On Wed, 24 Aug 2016 16:00:24 +0100
Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 24 August 2016 at 15:29, Greg Kurz <gr...@kaod.org> 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
