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

[Tim Ruehsen]

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 ;-)

Regards, Tim

signature.asc
Description: This is a digitally signed message part.