Date: Tue, 14 Dec 2021 21:34:30 +0200
From: Timo Teras <timo.teras@iki.fi>
To: Jan Hendrik Farr <git@jfarr.cc>
Cc: ~alpine/apk-tools@lists.alpinelinux.org
Subject: Re: [PATCH 1/1] libfetch: Allow special characters in http basic
 auth
Message-ID: <20211214213430.029b34c5@vostro>
In-Reply-To: <20211213212929.925-1-git@jfarr.cc>
References: <20211213212929.925-1-git@jfarr.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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