Hi,
On Mon, 13 Dec 2021 21:29:29 +0000
Jan Hendrik Farr <git@jfarr.cc> wrote:
> Currently, special characters in the username or password are not> handled correctly (when set in $http_proxy and $https_proxy). They> should be percent encoded in the environment variables then decoded> by libfetch and reencoded using base64. This implementation is mainly> taken from the current FreeBSD source and adapted to the apk-tools> version of libfetch.
Thanks! There is a related bug
https://gitlab.alpinelinux.org/alpine/apk-tools/-/issues/10775
If this get's fixed, add to commit message:
ref #10775
> ---> libfetch/fetch.c | 69> +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 57> insertions(+), 12 deletions(-)> > diff --git a/libfetch/fetch.c b/libfetch/fetch.c> index 45c92aa..ced8411 100644> --- a/libfetch/fetch.c> +++ b/libfetch/fetch.c> @@ -353,6 +353,54 @@ fetchCopyURL(const struct url *src)> return dst;> }> > +/*> + * Return value of the given hex digit.> + */> +static int> +fetch_hexval(char ch)> +{> +> + if (ch >= '0' && ch <= '9')> + return (ch - '0');> + else if (ch >= 'a' && ch <= 'f')> + return (ch - 'a' + 10);> + else if (ch >= 'A' && ch <= 'F')> + return (ch - 'A' + 10);> + return (-1);> +}> +> +/*> + * Decode percent-encoded URL component from src into dst, stopping> at end> + * of string, or at @ or : separators. Returns a pointer to the> unhandled> + * part of the input string (null terminator, @, or :). No> terminator is> + * written to dst (it is the caller's responsibility).> + */> +static const char *> +fetch_pctdecode(char *dst, const char *src, size_t dlen)> +{> + int d1, d2;> + char c;> + const char *s;> +> + for (s = src; *s != '\0' && *s != '@' && *s != ':'; s++) {> + if (s[0] == '%' && (d1 = fetch_hexval(s[1])) >= 0 &&> + (d2 = fetch_hexval(s[2])) >= 0 && (d1 > 0 || d2 > 0)) {> + c = d1 << 4 | d2;> + s += 2;> + } else if (s[0] == '%') {> + /* Invalid escape sequence. */> + return (NULL);> + } else {> + c = *s;> + }> + if (dlen-- > 0)> + *dst++ = c;> + else> + return (NULL);
minor readability issue; i'd prefer something like:
if (!dlen)
return NULL;
dlen--;
*dst++ = c;
To make the error path and normal path more obvious.
> + }> + return (s);> +}> +> /*> * Split an URL into components. URL syntax is:> * [method:/][/[user[:pwd]@]host[:port]/][document]> @@ -428,22 +476,19 @@ find_user:> p = strpbrk(URL, "/@");> if (p != NULL && *p == '@') {> /* username */> - for (q = URL, i = 0; (*q != ':') && (*q != '@');> q++) {> - if (i >= URL_USERLEN) {> - url_seterr(URL_BAD_AUTH);> - goto ouch;> - }> - u->user[i++] = *q;> + q = URL;> + q = fetch_pctdecode(u->user, q, URL_USERLEN);> + if (q == NULL) {> + url_seterr(URL_BAD_AUTH);> + goto ouch;> }> > /* password */> if (*q == ':') {> - for (q++, i = 0; (*q != '@'); q++) {> - if (i >= URL_PWDLEN) {> - url_seterr(URL_BAD_AUTH);> - goto ouch;> - }> - u->pwd[i++] = *q;> + q = fetch_pctdecode(u->pwd, q + 1,> URL_PWDLEN);
This will terminate also with ':' in the string now. And will handle
the rest of the URL incorrectly, and break password with ':'.
Is the upstream file somewhere?
> + if (q == NULL) {> + url_seterr(URL_BAD_AUTH);> + goto ouch;> }> }>
Other than the above, it looks good. Thanks for working on this!
Timo
Hi,
> Thanks! There is a related bug> https://gitlab.alpinelinux.org/alpine/apk-tools/-/issues/10775>> If this get's fixed, add to commit message:> ref #10775>
Looks like this patch should fix that issue.
> Is the upstream file somewhere?
Yes. As I understand, libfetch.c was originally taken from NetBSD (with certain bits taken from FreeBSD): https://github.com/NetBSD/src/blob/trunk/external/bsd/fetch/dist/libfetch/fetch.c
That file has no fix for this and has the same issue. I took this fix from the FreeBSD version: https://cgit.freebsd.org/src/tree/lib/libfetch/fetch.c
I took the functions fetch_hexval and fetch_pctdecode (both exact copies) from that file and integrated them into fetchParseURL.
> minor readability issue; i'd prefer something like:> if (!dlen)> return NULL;> dlen--;> *dst++ = c;>> To make the error path and normal path more obvious.
Since fetch_pctdecode is an exact copy from FreeBSD I would prefer to keep it unchanged.
> This will terminate also with ':' in the string now. And will handle> the rest of the URL incorrectly, and break password with ':'.
Usernames used for http basic authentication are not allowed to contain ":" according RFC7617 section 2.1 (https://datatracker.ietf.org/doc/html/rfc7617#section-2.1). They are allowed to contain other special characters though and the password is allowed to contain special characters including ":". The characters ":", "@", or "/" present in the username or password must be encoded using percent encoding per RFC1738 section 3.1 (https://datatracker.ietf.org/doc/html/rfc1738#section-3.1). So the only ":" present in this part of the string will be the separator between username and password.
Example:
username: foo@123/bar
password: pass1234:hello:
A correct proxy setting (or other URL) using these credentials should be encoded by the user as:
http://foo%40123%2Fbar:pass1234%3Ahello%3A@example.com
libfetch should then decode the percent encoding to obtain the username and password in plain text, append them using a ":", and then encode the result using base64 to be used during http basic authentication:
result: Zm9vQDEyMy9iYXI6cGFzczEyMzQ6aGVsbG86
The decoding of the percent encoding is added in this patch and the appending of username and password and base64 encoding is already correctly handled in http.c.
Would you prefer for me to open a merge request on your Gitlab instead? If yes: Which branch should I open the request for?
With kind regards,
Jan
Hi,
sorry, forgot to wrap my lines. Wrapped version:
> Thanks! There is a related bug> https://gitlab.alpinelinux.org/alpine/apk-tools/-/issues/10775>> If this get's fixed, add to commit message: ref #10775>
Looks like this patch should fix that issue.
> Is the upstream file somewhere?
Yes. As I understand, libfetch.c was originally taken from NetBSD (with
certain bits taken from FreeBSD):
https://github.com/NetBSD/src/blob/trunk/external/bsd/fetch/dist/libfetch/fetch.c
That file has no fix for this and has the same issue. I took this fix
from the FreeBSD version:
https://cgit.freebsd.org/src/tree/lib/libfetch/fetch.c
I took the functions fetch_hexval and fetch_pctdecode (both exact
copies) from that file and integrated them into fetchParseURL.
> minor readability issue; i'd prefer something like:> if (!dlen)> return NULL;> dlen--;> *dst++ = c;>> To make the error path and normal path more obvious.
Since fetch_pctdecode is an exact copy from FreeBSD I would prefer to
keep it unchanged.
> This will terminate also with ':' in the string now. And will handle> the rest of the URL incorrectly, and break password with ':'.
Usernames used for http basic authentication are not allowed to contain
":" according RFC7617 section 2.1
(https://datatracker.ietf.org/doc/html/rfc7617#section-2.1). They are
allowed to contain other special characters though and the password is
allowed to contain special characters including ":". The characters ":",
"@", or "/" present in the username or password must be encoded using
percent encoding per RFC1738 section 3.1
(https://datatracker.ietf.org/doc/html/rfc1738#section-3.1). So the only
":" present in this part of the string will be the separator between
username and password.
Example:
username: foo@123/bar
password: pass1234:hello:
A correct proxy setting (or other URL) using these credentials should be
encoded by the user as:
http://foo%40123%2Fbar:pass1234%3Ahello%3A@example.com
libfetch should then decode the percent encoding to obtain the username
and password in plain text, append them using a ":", and then encode the
result using base64 to be used during http basic authentication:
result: Zm9vQDEyMy9iYXI6cGFzczEyMzQ6aGVsbG86
The decoding of the percent encoding is added in this patch and the
appending of username and password and base64 encoding is already
correctly handled in http.c.
Would you prefer for me to open a merge request on your Gitlab instead?
If yes: Which branch should I open the request for?
With kind regards, Jan
Hey!
On Wed, 15 Dec 2021 00:21:34 +0100
"Jan Hendrik Farr" <git@jfarr.cc> wrote:
> sorry, forgot to wrap my lines. Wrapped version:> > > Thanks! There is a related bug> > https://gitlab.alpinelinux.org/alpine/apk-tools/-/issues/10775> >> > If this get's fixed, add to commit message: ref #10775> > Looks like this patch should fix that issue.
Great!
> > Is the upstream file somewhere? > > Yes. As I understand, libfetch.c was originally taken from NetBSD> (with certain bits taken from FreeBSD):> https://github.com/NetBSD/src/blob/trunk/external/bsd/fetch/dist/libfetch/fetch.c> > That file has no fix for this and has the same issue. I took this fix> from the FreeBSD version:> https://cgit.freebsd.org/src/tree/lib/libfetch/fetch.c> > I took the functions fetch_hexval and fetch_pctdecode (both exact> copies) from that file and integrated them into fetchParseURL.
Thanks, would be good to mention the source reference in the commit
message too.
> > minor readability issue; i'd prefer something like:> > if (!dlen)> > return NULL;> > dlen--;> > *dst++ = c;> >> > To make the error path and normal path more obvious. > > Since fetch_pctdecode is an exact copy from FreeBSD I would prefer to> keep it unchanged.
libfetch is a bit of oddball. I was really hoping to not need do much
maintenance on it. But turns out there's been quite a bit of things to
fix on it. I'm tempted to run it through formatting utility and make it
a proper fork. But for the time being I'm still not commited. Ideally
I'd like to rewrite it to use the apk internal buffering, but that's a
bit bigger project for which I don't have yet time for. So I'm ok
either way with this for now (seeing it's direct upstream copy).
> > This will terminate also with ':' in the string now. And will handle> > the rest of the URL incorrectly, and break password with ':'. > > Usernames used for http basic authentication are not allowed to> contain ":" according RFC7617 section 2.1> (https://datatracker.ietf.org/doc/html/rfc7617#section-2.1). They are> allowed to contain other special characters though and the password is> allowed to contain special characters including ":". The characters> ":", "@", or "/" present in the username or password must be encoded> using percent encoding per RFC1738 section 3.1> (https://datatracker.ietf.org/doc/html/rfc1738#section-3.1). So the> only ":" present in this part of the string will be the separator> between username and password.
Yes. I understand it's not valid. I was trying to point out that if
user provides this invalidly formed data it now does completely wrong
thing. Previously it used to accept ':' as literal for password which
sounded acceptable.
The new code would treat stuff from ':' forward as URL by rest of the
code. As minimum after the password decoding call, there should be
check, so basically as minimum:
+ q = fetch_pctdecode(u->pwd, q + 1, URL_PWDLEN);
+ if (q == NULL || *q != '@') {
+ url_seterr(URL_BAD_AUTH);
This way user gets sane error in case he attempted something
non-standard that used to work; instead of getting incorrect behaviour.
> Would you prefer for me to open a merge request on your Gitlab> instead? If yes: Which branch should I open the request for?
I am happy both ways. Since the requested changes are commit message
and the one extra "if" condition, I can also commit this with the edits.
Timo
Hi,
this version will allow unencoded colons in the password. I modified
fetch_pctdecode to allow the callee to specify at what characters it
should stop. For the username it stops at ":" or "@" and for the
password just at "@". I prefer this behavior over giving people an error.
While it will allow some invalid credentials this is actually the same
behavior as curl + this won't brake current setups.
With kind regards, Jan
Currently, special characters in the username or password are not
handled correctly (when set in $http_proxy and $https_proxy). They
should be percent encoded in the environment variables then decoded
by libfetch and reencoded using base64. This implementation is mainly
taken from the current FreeBSD source and adapted to the apk-tools
version of libfetch.
---
libfetch/fetch.c | 80 ++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 67 insertions(+), 13 deletions(-)
diff --git a/libfetch/fetch.c b/libfetch/fetch.c
index 45c92aa..526ba86 100644
--- a/libfetch/fetch.c+++ b/libfetch/fetch.c
@@ -354,7 +354,58 @@ fetchCopyURL(const struct url *src)
}
/*
- * Split an URL into components. URL syntax is:+ * Return value of the given hex digit.+ */+static int+fetch_hexval(char ch)+{++ if (ch >= '0' && ch <= '9')+ return (ch - '0');+ else if (ch >= 'a' && ch <= 'f')+ return (ch - 'a' + 10);+ else if (ch >= 'A' && ch <= 'F')+ return (ch - 'A' + 10);+ return (-1);+}++/*+ * Decode percent-encoded URL component from src into dst, stopping at end+ * of string or one of the characters contained in brk. Returns a pointer+ * to the unhandled part of the input string (null terminator, specified+ * character). No terminator is written to dst (it is the caller's+ * responsibility).+ */+static const char *+fetch_pctdecode(char *dst, const char *src, const char *brk, size_t dlen)+{+ int d1, d2;+ char c;+ const char *s;++++ for (s = src; *s != '\0' && !strchr(brk, *s); s++) {+ if (s[0] == '%' && (d1 = fetch_hexval(s[1])) >= 0 &&+ (d2 = fetch_hexval(s[2])) >= 0 && (d1 > 0 || d2 > 0)) {+ c = d1 << 4 | d2;+ s += 2;+ } else if (s[0] == '%') {+ /* Invalid escape sequence. */+ return (NULL);+ } else {+ c = *s;+ }+ if (!dlen)+ return NULL;+ dlen--;+ *dst++ = c;+ }+ return (s);+}++/*+ * Split a URL into components. URL syntax is: * [method:/][/[user[:pwd]@]host[:port]/][document]
* This almost, but not quite, RFC1738 URL syntax.
*/
@@ -428,25 +479,28 @@ find_user:
p = strpbrk(URL, "/@");
if (p != NULL && *p == '@') {
/* username */
- for (q = URL, i = 0; (*q != ':') && (*q != '@'); q++) {- if (i >= URL_USERLEN) {- url_seterr(URL_BAD_AUTH);- goto ouch;- }- u->user[i++] = *q;+ q = URL;+ q = fetch_pctdecode(u->user, q, ":@", URL_USERLEN);+ if (q == NULL) {+ url_seterr(URL_BAD_AUTH);+ goto ouch; }
/* password */
if (*q == ':') {
- for (q++, i = 0; (*q != '@'); q++) {- if (i >= URL_PWDLEN) {- url_seterr(URL_BAD_AUTH);- goto ouch;- }- u->pwd[i++] = *q;+ q = fetch_pctdecode(u->pwd, q + 1, "@", URL_PWDLEN);++ if (q == NULL) {+ url_seterr(URL_BAD_AUTH);+ goto ouch; }
}
+ if (*q != '@') {+ url_seterr(URL_BAD_AUTH);+ goto ouch;+ }+ p++;
} else {
p = URL;
--
2.34.1
Hi!
Committed with whitespace fixes. Somehow tabs were converted to spaces
in the patch you sent and it did not apply cleanly.
Thank you!
Timo
On Thu, 16 Dec 2021 23:34:38 +0100
"Jan Hendrik Farr" <git@jfarr.cc> wrote:
> Hi,> > this version will allow unencoded colons in the password. I modified> fetch_pctdecode to allow the callee to specify at what characters it> should stop. For the username it stops at ":" or "@" and for the> password just at "@". I prefer this behavior over giving people an> error. While it will allow some invalid credentials this is actually> the same behavior as curl + this won't brake current setups.> > With kind regards, Jan> > > > > > > Currently, special characters in the username or password are not> handled correctly (when set in $http_proxy and $https_proxy). They> should be percent encoded in the environment variables then decoded> by libfetch and reencoded using base64. This implementation is mainly> taken from the current FreeBSD source and adapted to the apk-tools> version of libfetch.> ---> libfetch/fetch.c | 80> ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 67> insertions(+), 13 deletions(-)> > diff --git a/libfetch/fetch.c b/libfetch/fetch.c> index 45c92aa..526ba86 100644> --- a/libfetch/fetch.c> +++ b/libfetch/fetch.c> @@ -354,7 +354,58 @@ fetchCopyURL(const struct url *src)> }> > /*> - * Split an URL into components. URL syntax is:> + * Return value of the given hex digit.> + */> +static int> +fetch_hexval(char ch)> +{> +> + if (ch >= '0' && ch <= '9')> + return (ch - '0');> + else if (ch >= 'a' && ch <= 'f')> + return (ch - 'a' + 10);> + else if (ch >= 'A' && ch <= 'F')> + return (ch - 'A' + 10);> + return (-1);> +}> +> +/*> + * Decode percent-encoded URL component from src into dst, stopping> at end> + * of string or one of the characters contained in brk. Returns a> pointer> + * to the unhandled part of the input string (null terminator,> specified> + * character). No terminator is written to dst (it is the caller's> + * responsibility).> + */> +static const char *> +fetch_pctdecode(char *dst, const char *src, const char *brk, size_t> dlen) +{> + int d1, d2;> + char c;> + const char *s;> +> +> +> + for (s = src; *s != '\0' && !strchr(brk, *s); s++) {> + if (s[0] == '%' && (d1 = fetch_hexval(s[1])) >= 0 &&> + (d2 = fetch_hexval(s[2])) >= 0 && (d1 > 0 || d2 >> 0)) {> + c = d1 << 4 | d2;> + s += 2;> + } else if (s[0] == '%') {> + /* Invalid escape sequence. */> + return (NULL);> + } else {> + c = *s;> + }> + if (!dlen)> + return NULL;> + dlen--;> + *dst++ = c;> + }> + return (s);> +}> +> +/*> + * Split a URL into components. URL syntax is:> * [method:/][/[user[:pwd]@]host[:port]/][document]> * This almost, but not quite, RFC1738 URL syntax.> */> @@ -428,25 +479,28 @@ find_user:> p = strpbrk(URL, "/@");> if (p != NULL && *p == '@') {> /* username */> - for (q = URL, i = 0; (*q != ':') && (*q != '@'); q++)> {> - if (i >= URL_USERLEN) {> - url_seterr(URL_BAD_AUTH);> - goto ouch;> - }> - u->user[i++] = *q;> + q = URL;> + q = fetch_pctdecode(u->user, q, ":@", URL_USERLEN);> + if (q == NULL) {> + url_seterr(URL_BAD_AUTH);> + goto ouch;> }> > /* password */> if (*q == ':') {> - for (q++, i = 0; (*q != '@'); q++) {> - if (i >= URL_PWDLEN) {> - url_seterr(URL_BAD_AUTH);> - goto ouch;> - }> - u->pwd[i++] = *q;> + q = fetch_pctdecode(u->pwd, q + 1, "@",> URL_PWDLEN); +> + if (q == NULL) {> + url_seterr(URL_BAD_AUTH);> + goto ouch;> }> }> > + if (*q != '@') {> + url_seterr(URL_BAD_AUTH);> + goto ouch;> + }> +> p++;> } else {> p = URL;> --> 2.34.1