On 2016/01/08 14:55, Lauri Tirkkonen wrote:
> On Fri, Jan 08 2016 12:43:59 +0000, Stuart Henderson wrote:
> > On 2016/01/08 14:04, Lauri Tirkkonen wrote:
> > > On Wed, Dec 30 2015 11:26:48 +0200, Lauri Tirkkonen wrote:
> > > > >Synopsis: ftp(1) fails when HTTP redirected to a relative URI
> > > > >containing the string "://"
> > >
> > > > >Fix:
> > > > Patch attached.
> > >
> > > No takers?
> >
> > The problem is valid, but the diff doesn't go far enough, it needs
> > an additional check to avoid breaking currently-working cases.
> >
> > Before patching:
> >
> > $ ftp -Mo- https://junkpile.org/redir-to-colon
> > Trying 195.95.187.26...
> > Requesting https://junkpile.org/redir-to-colon
> > Redirected to https://junkpile.org/test:file.txt
> > Trying 195.95.187.26...
> > Requesting https://junkpile.org/test:file.txt
> > testing...
> > 11 bytes received in 0.00 seconds (44.02 KB/s)
> >
> > After:
> >
> > $ obj/ftp -Mo- https://junkpile.org/redir-to-colon
> > Trying 195.95.187.26...
> > Requesting https://junkpile.org/redir-to-colon
> > Redirected to test:file.txt
> > ftp: url_get: Invalid URL 'test:file.txt'
> >
> > This is a contrived test, but there are definitely URLs in the
> > wild containing : and it wouldn't be entirely unexpected to see
> > a relative redirect to them.
>
> I was following RFC 3986 which states that relative-path segments
> containing a colon cannot be used as the first segment [0].
>
> [0]: https://tools.ietf.org/html/rfc3986#section-4.2
Ah, thanks for pointing that out. In which case, I think we
should refer to the RFC (and use KNF style for the multi-line
comment). While there, I'd like to add an XXX comment that
we don't handle // redirects.
Any OKs to commit this?
Index: fetch.c
===================================================================
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.143
diff -u -p -r1.143 fetch.c
--- fetch.c 13 Oct 2015 08:53:43 -0000 1.143
+++ fetch.c 8 Jan 2016 13:13:42 -0000
@@ -833,10 +833,15 @@ again:
} else if (isredirect &&
strncasecmp(cp, LOCATION, sizeof(LOCATION) - 1) == 0) {
cp += sizeof(LOCATION) - 1;
- if (strstr(cp, "://") == NULL) {
+ /*
+ * If there is a colon before the first slash, this URI
+ * is not relative. RFC 3986 4.2
+ */
+ if (cp[strcspn(cp, ":/")] != ':') {
#ifdef SMALL
errx(1, "Relative redirect not supported");
#else /* SMALL */
+ /* XXX doesn't handle protocol-relative URIs */
if (*cp == '/') {
locbase = NULL;
cp++;
