Date: Wed, 15 Dec 2021 08:22:20 +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: <20211215082220.71dbc443@vostro>
In-Reply-To: <e4a2e0eb-762f-41e7-92ce-0c58023f280d@www.fastmail.com>
References: <20211213212929.925-1-git@jfarr.cc>
	<20211214213430.029b34c5@vostro>
	<e4a2e0eb-762f-41e7-92ce-0c58023f280d@www.fastmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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