Re: [Qemu-devel] [PATCH] 9pfs: disallow / in path components

[Greg Kurz]

On Wed, 24 Aug 2016 16:00:24 +0100
Peter Maydell <address@hidden> wrote:

> On 24 August 2016 at 15:29, Greg Kurz <address@hidden> wrote:
> > At various places in 9pfs, full paths are created by concatenating a guest
> > originated string to the export path. A malicious guest could forge a
> > relative path and access files outside the export path.
> >
> > A tentative fix was sent recently by Prasad J Pandit, but it was only
> > focused on the local backend and did not get a positive review. This patch
> > tries to address the issue more globally, based on the official 9P spec.
> >
> > The walk request described in the 9P spec [1] clearly shows that the client
> > is supposed to send individual path components: the official linux client
> > never sends portions of path containing the / character for example.
> >
> > Moreover, the 9P spec [2] also states that a system can decide to restrict
> > the set of supported characters used in path components, with an explicit
> > mention "to remove slashes from name components".
> >
> > This patch introduces a new name_has_illegal_characters() helper that looks
> > for such unwanted characters in strings sent by the client. Since 9pfs is
> > only supported on linux hosts, only the / character is checked at the
> > moment. When support for other hosts (AKA. win32) is added, other chars
> > may need to be blacklisted as well.  
> 
> Do we also need ".." and "." to be illegal names (for at least most
> operations)?
> 
> thanks
> -- PMM

I understand how ".." could be an issue, but I don't for "."... can you
please elaborate ?

The spec says:

          Directories are created by create with DMDIR set in the per-
          missions argument (see stat(5)). The members of a directory
          can be found with read(5). All directories must support
          walks to the directory .. (dot-dot) meaning parent direc-
          tory, although by convention directories contain no explicit
          entry for .. or . (dot).  The parent of the root directory
          of a server's tree is itself.

So I don't think we should boldly make ".." an illegal name, but
rather ignore it. Pretty much like doing chdir("..") when the current
directory is /.

All operations except walk take an existing fid and a single path component.
A possible fix would be to convert ".." to "" when the fid points to the root
of the export path. Makes sense ?

Since the walk operation takes an array of path components, we would need
to do extend the above check to each component.

Cheers.

--
Greg