Jan Hendrik Farr: 2 libfetch: Allow special characters in http basic auth libfetch: Allow special characters in http basic auth 2 files changed, 124 insertions(+), 25 deletions(-)
Hi,
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.
Hey! On Wed, 15 Dec 2021 00:21:34 +0100 "Jan Hendrik Farr" <git@jfarr.cc> wrote:
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:
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.alpinelinux.org/~alpine/apk-tools/patches/3807/mbox | git am -3Learn more about email & git
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.
Timo Teras <timo.teras@iki.fi>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);
Timo Teras <timo.teras@iki.fi>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);
Timo Teras <timo.teras@iki.fi>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; } }
Timo Teras <timo.teras@iki.fi>Other than the above, it looks good. Thanks for working on this! Timo
-- 2.34.1
Timo Teras <timo.teras@iki.fi>Hi, On Mon, 13 Dec 2021 21:29:29 +0000 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
Timo Teras <timo.teras@iki.fi>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: