Re: [Bug-wget] [PATCH 09/25] Enforce Metalink file name verification, strip directory if necessary

[Matthew White]

On Tue, 13 Sep 2016 16:21:37 +0200
Tim Ruehsen <address@hidden> wrote:

> On Tuesday, September 13, 2016 4:08:30 PM CEST Matthew White wrote:
> > On Tue, 13 Sep 2016 09:29:27 +0200
> > 
> > Tim Ruehsen <address@hidden> wrote:
> > > On Tuesday, September 13, 2016 5:13:10 AM CEST Matthew White wrote:
> > > > On Mon, 12 Sep 2016 21:20:54 +0200
> > > > 
> > > > Tim Rühsen <address@hidden> wrote:
> > > > > On Montag, 12. September 2016 20:18:30 CEST Eli Zaretskii wrote:
> > > > > > > From: Tim Ruehsen <address@hidden>
> > > > > > > Date: Mon, 12 Sep 2016 13:00:32 +0200
> > > > > > > 
> > > > > > > > +  char *basename = name;
> > > > > > > > +
> > > > > > > > +  while ((name = strstr (basename, "/")))
> > > > > > > > +    basename = name + 1;
> > > > > > > 
> > > > > > > Could you use strrchr() ? something like
> > > > > > > 
> > > > > > > char *basename = strrchr (name, '/');
> > > > > > > 
> > > > > > > if (basename)
> > > > > > > 
> > > > > > >   basename += 1;
> > > > > > > 
> > > > > > > else
> > > > > > > 
> > > > > > >   basename = name;
> > > > > > 
> > > > > > I think we want to use ISSEP, no?  Otherwise Windows file names with
> > > > > > backslashes will misfire.
> > > > > 
> > > > > Good point. What about device names ?
> > > > > 
> > > > > So maybe base_name() from Gnulib module 'dirname' is the right choice
> > > > > !?
> > > > > See https://www.gnu.org/software/gnulib/manual/html_node/basename.html
> > > > 
> > > > What if Gnulib's base_name() returns "./<basename>"?
> > > > 
> > > > libmetalink's metalink_check_safe_path() rejects relative paths:
> > > > https://tools.ietf.org/html/rfc5854#section-4.1.2.1
> > > > 
> > > > Also, basename is used to point to an existing memory location,
> > > > base_name()
> > > > instead allocates new space. This is not a biggy, but we should keep it
> > > > in
> > > > mind to amend properly.
> > > > 
> > > > lib/basename.c (base_name)
> > > > --------------------------
> > > > 
> > > >   /* On systems with drive letters, "a/b:c" must return "./b:c" rather
> > > >   
> > > >      than "b:c" to avoid confusion with a drive letter.  On systems
> > > >      with pure POSIX semantics, this is not an issue.  */
> > > > 
> > > > --------------------------
> > > > 
> > > > Suggestions?
> > > 
> > > ISSEP is "homebrewed" and incomplete but doesn't need a memory allocation.
> > > base_name() is "complete" (the macros check more than just WINDOWS) and we
> > > automatically get improvements from upstream - but it calls malloc().
> > > 
> > > There is also last_component() which returns a pointer to the basename
> > > within your filename. This is basically what you do.
> > > 
> > > Anyways, this last component (basename) may still hold a device prefix -
> > > you have to check that with either HAS_DEVICE() (only defined in certain
> > > environments, needs to guarded by #ifdef) or by FILE_SYSTEM_PREFIX_LEN()
> > > which gives 2 if your basename has a leading device prefix. And you
> > > should do this check in a loop to catch names like 'C:D:xxx'. If you
> > > don't do, we likely get a CVE assigned ;-)
> > 
> > Thanks Tim! Yours is a super smart solution!
> > 
> > I rewrote src/metalink.c (get_metalink_basename) to skip prefix drive
> > letters on the basename (the declaration of last_component() is in
> > src/metalink.h):
> > 
> > #include "dosname.h"
> > 
> > char *
> > get_metalink_basename (char *name)
> > {
> >   char *basename;
> > 
> >   if (!name)
> >     return NULL;
> > 
> >   basename = last_component (name);
> > 
> >   while (FILE_SYSTEM_PREFIX_LEN (basename))
> >     basename += 2;
> > 
> >   return metalink_check_safe_path (basename) ? basename : NULL;
> > }
> > 
> > [make syntax-check is ok, make check-valgrind is ok, contrib/check-hard is
> > ok]
> > 
> > WDYT?
> 
> Looks great !
> 
> To be absolutely future ready, don't assume FILE_SYSTEM_PREFIX_LEN to return 
> 0 
> or 2 (even if the current code is like that). 
> 
> while ((n = FILE_SYSTEM_PREFIX_LEN (basename)))
>   basename += n;
> 
> and you are on the safe side.

Thanks, you're beyond me! Fix implemented.

> 
> Regards, Tim

Regards,
Matthew

-- 
Matthew White <address@hidden>

pgpXY48iUoul4.pgp
Description: PGP signature