* [pve-devel] [PATCH proxmox-acme 1/3] acme client: fix #3536 untaint data returned from acme server
2021-08-06 15:44 [pve-devel] [PATCH proxmox-acme 0/3] update acme.sh and fix #3536 and #3546 Stoiko Ivanov
@ 2021-08-06 15:44 ` Stoiko Ivanov
2021-08-06 15:44 ` [pve-devel] [PATCH proxmox-acme 2/3] update to acme.sh dns plugins to 3.0.0 Stoiko Ivanov
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Stoiko Ivanov @ 2021-08-06 15:44 UTC (permalink / raw)
To: pve-devel
The data returned from the acme server (e.g. boulder) should be
considered tainted.
To see which places might need untainted I checked where $self->{ua}
was used (in $self->do) and a quick scan identified __get_result as
the consumer of the tainted data.
In all but one uses the data is decoded from json (which would die if the
result is not valid json).
The remaining use-case yields a certificate in PEM format (and is
handled at the caller of __get_result).
The issue is currently only visible if a proxy is set, because AFAICT
somewhere in SSLeay (or IO::Socket::SSL, which uses SSLeay) a taint
flag is not set on the return value.
A reproducer for the issue:
```
use strict;
use warnings;
use HTTP::Request;
use LWP::UserAgent;
$ENV{PATH} = "/usr/bin:/bin";
my $ua = LWP::UserAgent->new(env_proxy => 0);
my $request = HTTP::Request->new('GET', 'https://google.com/');
my $resp = $ua->request($request);
my $text = substr($resp->decoded_content, 0, 5);;
system("echo \"$text\""); # does work
$request = HTTP::Request->new('GET', 'http://neverssl.com/');
$resp = $ua->request($request);
$text = substr($resp->decoded_content, 0, 5);;
system("echo \"$text\""); # does not work
```
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
src/PVE/ACME.pm | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
index c5b8d66..265482d 100644
--- a/src/PVE/ACME.pm
+++ b/src/PVE/ACME.pm
@@ -69,7 +69,9 @@ sub tojs($;%) { # shortcut for to_json with utf8=>1
}
sub fromjs($) {
- return from_json($_[0]);
+ my ($data) = @_;
+ ($data) = ($data =~ /^(.*)$/s); # untaint from_json croaks on error anyways.
+ return from_json($data);
}
sub fatal($$;$$) {
@@ -449,7 +451,13 @@ sub get_certificate {
if !$order->{certificate};
my $r = $self->do(POST => $order->{certificate}, '');
- my $return = eval { __get_result($r, 200, 1); };
+ my $return = eval {
+ my $res = __get_result($r, 200, 1);
+ if ($res =~ /^(-----BEGIN CERTIFICATE-----)(.+)(-----END CERTIFICATE-----)$/s) { # untaint
+ return $1 . $2 . $3;
+ }
+ die "Server reply does not look like a PEM encoded certificate\n";
+ };
$self->fatal("POST of '$order->{certificate}' failed - $@", $r) if $@;
return $return;
}
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread* [pve-devel] [PATCH proxmox-acme 2/3] update to acme.sh dns plugins to 3.0.0
2021-08-06 15:44 [pve-devel] [PATCH proxmox-acme 0/3] update acme.sh and fix #3536 and #3546 Stoiko Ivanov
2021-08-06 15:44 ` [pve-devel] [PATCH proxmox-acme 1/3] acme client: fix #3536 untaint data returned from acme server Stoiko Ivanov
@ 2021-08-06 15:44 ` Stoiko Ivanov
2021-08-06 15:44 ` [pve-devel] [PATCH proxmox-acme 3/3] plugin-caller: pull in changes from upstream 3.0.0 Stoiko Ivanov
2021-08-11 10:29 ` [pve-devel] applied: [PATCH proxmox-acme 0/3] update acme.sh and fix #3536 and #3546 Fabian Grünbichler
3 siblings, 0 replies; 5+ messages in thread
From: Stoiko Ivanov @ 2021-08-06 15:44 UTC (permalink / raw)
To: pve-devel
fixes #3546
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
src/Makefile | 2 ++
src/acme.sh | 2 +-
src/dns-challenge-schema.json | 2 ++
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/Makefile b/src/Makefile
index 16166fe..2503683 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -14,6 +14,7 @@ ACME_SOURCES = \
dnsapi/dns_aurora.sh \
dnsapi/dns_autodns.sh \
dnsapi/dns_aws.sh \
+ dnsapi/dns_azion.sh \
dnsapi/dns_azure.sh \
dnsapi/dns_cf.sh \
dnsapi/dns_clouddns.sh \
@@ -93,6 +94,7 @@ ACME_SOURCES = \
dnsapi/dns_nsone.sh \
dnsapi/dns_nsupdate.sh \
dnsapi/dns_nw.sh \
+ dnsapi/dns_oci.sh \
dnsapi/dns_one.sh \
dnsapi/dns_online.sh \
dnsapi/dns_openprovider.sh \
diff --git a/src/acme.sh b/src/acme.sh
index 9293bcf..d84da5b 160000
--- a/src/acme.sh
+++ b/src/acme.sh
@@ -1 +1 @@
-Subproject commit 9293bcfb1cd5a56c6cede3f5f46af8529ee99624
+Subproject commit d84da5bdbf2ea190977ad66669173e3256380d2a
diff --git a/src/dns-challenge-schema.json b/src/dns-challenge-schema.json
index 559077f..ddfe2b2 100644
--- a/src/dns-challenge-schema.json
+++ b/src/dns-challenge-schema.json
@@ -90,6 +90,7 @@
},
"name" : "Amazon Route53 (AWS)"
},
+ "azion" : {},
"azure" : {},
"cf" : {
"description" : "Either provide global account key and email, or CF API token and Account ID.",
@@ -238,6 +239,7 @@
"nsone" : {},
"nsupdate" : {},
"nw" : {},
+ "oci" : {},
"one" : {},
"online" : {},
"openprovider" : {},
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread* [pve-devel] [PATCH proxmox-acme 3/3] plugin-caller: pull in changes from upstream 3.0.0
2021-08-06 15:44 [pve-devel] [PATCH proxmox-acme 0/3] update acme.sh and fix #3536 and #3546 Stoiko Ivanov
2021-08-06 15:44 ` [pve-devel] [PATCH proxmox-acme 1/3] acme client: fix #3536 untaint data returned from acme server Stoiko Ivanov
2021-08-06 15:44 ` [pve-devel] [PATCH proxmox-acme 2/3] update to acme.sh dns plugins to 3.0.0 Stoiko Ivanov
@ 2021-08-06 15:44 ` Stoiko Ivanov
2021-08-11 10:29 ` [pve-devel] applied: [PATCH proxmox-acme 0/3] update acme.sh and fix #3536 and #3546 Fabian Grünbichler
3 siblings, 0 replies; 5+ messages in thread
From: Stoiko Ivanov @ 2021-08-06 15:44 UTC (permalink / raw)
To: pve-devel
Commits ae3dda0f8fc3071495cd1e8dff0fe4a339febb1c and
d70b759cb9c5b413cce92e65e841a54a65813962
implementing retrying get and post requests seem worth pulling in.
From a quick look through the diff the remaining changes (between
2.9.0 and 3.0.0) should not be relevant for us
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
src/proxmox-acme | 62 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 60 insertions(+), 2 deletions(-)
diff --git a/src/proxmox-acme b/src/proxmox-acme
index 6cc7b5f..a00d23a 100644
--- a/src/proxmox-acme
+++ b/src/proxmox-acme
@@ -222,6 +222,8 @@ _resethttp() {
:
}
+_HTTP_MAX_RETRY=8
+
# body url [needbase64] [POST|PUT|DELETE] [ContentType]
_post() {
body="$1"
@@ -229,6 +231,33 @@ _post() {
needbase64="$3"
httpmethod="$4"
_postContentType="$5"
+ _sleep_retry_sec=1
+ _http_retry_times=0
+ _hcode=0
+ while [ "${_http_retry_times}" -le "$_HTTP_MAX_RETRY" ]; do
+ [ "$_http_retry_times" = "$_HTTP_MAX_RETRY" ]
+ _lastHCode="$?"
+ _debug "Retrying post"
+ _post_impl "$body" "$_post_url" "$needbase64" "$httpmethod" "$_postContentType" "$_lastHCode"
+ _hcode="$?"
+ _debug _hcode "$_hcode"
+ if [ "$_hcode" = "0" ]; then
+ break
+ fi
+ _http_retry_times=$(_math $_http_retry_times + 1)
+ _sleep $_sleep_retry_sec
+ done
+ return $_hcode
+}
+
+# body url [needbase64] [POST|PUT|DELETE] [ContentType] [displayError]
+_post_impl() {
+ body="$1"
+ _post_url="$2"
+ needbase64="$3"
+ httpmethod="$4"
+ _postContentType="$5"
+ displayError="$6"
if [ -z "$httpmethod" ]; then
httpmethod="POST"
@@ -272,7 +301,9 @@ _post() {
fi
_ret="$?"
if [ "$_ret" != "0" ]; then
- _err "Please refer to https://curl.haxx.se/libcurl/c/libcurl-errors.html for error code: $_ret"
+ if [ -z "$displayError" ] || [ "$displayError" = "0" ]; then
+ _err "Please refer to https://curl.haxx.se/libcurl/c/libcurl-errors.html for error code: $_ret"
+ fi
fi
printf "%s" "$response"
return $_ret
@@ -283,6 +314,31 @@ _get() {
url="$1"
onlyheader="$2"
t="$3"
+ _sleep_retry_sec=1
+ _http_retry_times=0
+ _hcode=0
+ while [ "${_http_retry_times}" -le "$_HTTP_MAX_RETRY" ]; do
+ [ "$_http_retry_times" = "$_HTTP_MAX_RETRY" ]
+ _lastHCode="$?"
+ _debug "Retrying GET"
+ _get_impl "$url" "$onlyheader" "$t" "$_lastHCode"
+ _hcode="$?"
+ _debug _hcode "$_hcode"
+ if [ "$_hcode" = "0" ]; then
+ break
+ fi
+ _http_retry_times=$(_math $_http_retry_times + 1)
+ _sleep $_sleep_retry_sec
+ done
+ return $_hcode
+}
+
+# url getheader timeout displayError
+_get_impl() {
+ url="$1"
+ onlyheader="$2"
+ t="$3"
+ displayError="$4"
_CURL="curl -L --silent --dump-header $HTTP_HEADER -g "
if [ "$HTTPS_INSECURE" ]; then
@@ -298,7 +354,9 @@ _get() {
fi
ret=$?
if [ "$ret" != "0" ]; then
- _err "Please refer to https://curl.haxx.se/libcurl/c/libcurl-errors.html for error code: $ret"
+ if [ -z "$displayError" ] || [ "$displayError" = "0" ]; then
+ _err "Please refer to https://curl.haxx.se/libcurl/c/libcurl-errors.html for error code: $ret"
+ fi
fi
return $ret
}
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread* [pve-devel] applied: [PATCH proxmox-acme 0/3] update acme.sh and fix #3536 and #3546
2021-08-06 15:44 [pve-devel] [PATCH proxmox-acme 0/3] update acme.sh and fix #3536 and #3546 Stoiko Ivanov
` (2 preceding siblings ...)
2021-08-06 15:44 ` [pve-devel] [PATCH proxmox-acme 3/3] plugin-caller: pull in changes from upstream 3.0.0 Stoiko Ivanov
@ 2021-08-11 10:29 ` Fabian Grünbichler
3 siblings, 0 replies; 5+ messages in thread
From: Fabian Grünbichler @ 2021-08-11 10:29 UTC (permalink / raw)
To: Proxmox VE development discussion
series, thanks!
On August 6, 2021 5:44 pm, Stoiko Ivanov wrote:
> This patchset started out as attempt to add comfortable proxy-handling to
> our acme client(s) and to address #3536 and #3546, but in it's current form
> only fixes the two issues.
>
> patch 1/3 is independent of the others and enables users to interact with an
> ACME provider via proxy on PVE on the commandline (by exporting the
> https_proxy environment variable).
>
> the remaining patches simply update the acme.sh submodule, add the 2 new
> dns-plugins to our schema.json file and and port over retrying GET and POST
> requests from acme.sh.
>
> Tested on my PVE-node with a domain of mine and the powerdns api:
> * setting https_proxy (and having a squid configured on a guest) does not
> cause the `pvenode acme cert renew -force` to abort due to taint-checks
> * the content type of the PATCH requests is application/json insted of
> application/x-www-form-urlencoded
>
> Stoiko Ivanov (3):
> acme client: fix #3536 untaint data returned from acme server
> update to acme.sh dns plugins to 3.0.0
> plugin-caller: pull in changes from upstream 3.0.0
>
> src/Makefile | 2 ++
> src/PVE/ACME.pm | 12 +++++--
> src/acme.sh | 2 +-
> src/dns-challenge-schema.json | 2 ++
> src/proxmox-acme | 62 +++++++++++++++++++++++++++++++++--
> 5 files changed, 75 insertions(+), 5 deletions(-)
>
> --
> 2.30.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread