| Message ID | 147204811781.25757.13905475486785615296.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24 August 2016 at 15:29, Greg Kurz <groug@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
On Wed, Aug 24, 2016 at 04:00:24PM +0100, Peter Maydell wrote: > On 24 August 2016 at 15:29, Greg Kurz <groug@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 agree, and I think this implies name_is_legal would be a better function name.
On Wed, 24 Aug 2016 16:00:24 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 24 August 2016 at 15:29, Greg Kurz <groug@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
On Wed, 24 Aug 2016 18:46:10 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Aug 24, 2016 at 04:00:24PM +0100, Peter Maydell wrote: > > On 24 August 2016 at 15:29, Greg Kurz <groug@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 agree, and I think this implies name_is_legal would be a better function name. > No I think this is a different issue that calls for a followup patch (see my other mail).
On Wed, Aug 24, 2016 at 06:41:45PM +0200, Greg Kurz wrote: > On Wed, 24 Aug 2016 18:46:10 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Aug 24, 2016 at 04:00:24PM +0100, Peter Maydell wrote: > > > On 24 August 2016 at 15:29, Greg Kurz <groug@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 agree, and I think this implies name_is_legal would be a better function name. > > > > No I think this is a different issue that calls for a followup patch (see my > other mail). OK, that's fine.
On Wed, Aug 24, 2016 at 04:29:07PM +0200, Greg Kurz 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. > > If a client sends a path component with an illegal character, the request > will fail and EINVAL is returned to the client. > > For the sake of simplicity and consistency, the check is done at top-level > for all affected 9P requests: > > - xattrwalk > - xattrcreate > - mknod > - rename > - renameat > - unlinkat > - mkdir > - walk > - link > - symlink > - create > - lcreate > > [1] http://man.cat-v.org/plan_9/5/walk > [2] http://man.cat-v.org/plan_9/5/intro > > Reported-by: Felix Wilhelm <fwilhelm@ernw.de> > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Greg Kurz <groug@kaod.org> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Since the linux client does not send / in path components and I don't > have enough time to write an appropriate qtest, I choosed to do manual > testing of the mkdir request with GDB: > > [greg@vm66 host]$ mkdir ...foo > (then turning ...foo into ../foo in QEMU with GDB) > mkdir: cannot create directory ‘...foo’: Invalid argument > > I also could run the POSIX file system test suite from TUXERA: > > http://www.tuxera.com/community/open-source-posix/ > > and did not observe any regression with this patch. > > hw/9pfs/9p.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 67 insertions(+) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index b6b02b46a9da..1c008814509c 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -1256,6 +1256,11 @@ static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t nwnames, V9fsQID *qids) > return offset; > } > > +static bool name_has_illegal_characters(const char *name) > +{ > + return strchr(name, '/') != NULL; > +} > + > static void v9fs_walk(void *opaque) > { > int name_idx; > @@ -1289,6 +1294,10 @@ static void v9fs_walk(void *opaque) > if (err < 0) { > goto out_nofid; > } > + if (name_has_illegal_characters(wnames[i].data)) { > + err = -EINVAL; > + goto out_nofid; > + } > offset += err; > } > } else if (nwnames > P9_MAXWELEM) { > @@ -1483,6 +1492,11 @@ static void v9fs_lcreate(void *opaque) > } > trace_v9fs_lcreate(pdu->tag, pdu->id, dfid, flags, mode, gid); > > + if (name_has_illegal_characters(name.data)) { > + err = -EINVAL; > + goto out_nofid; > + } > + > fidp = get_fid(pdu, dfid); > if (fidp == NULL) { > err = -ENOENT; > @@ -2077,6 +2091,11 @@ static void v9fs_create(void *opaque) > } > trace_v9fs_create(pdu->tag, pdu->id, fid, name.data, perm, mode); > > + if (name_has_illegal_characters(name.data)) { > + err = -EINVAL; > + goto out_nofid; > + } > + > fidp = get_fid(pdu, fid); > if (fidp == NULL) { > err = -EINVAL; > @@ -2242,6 +2261,11 @@ static void v9fs_symlink(void *opaque) > } > trace_v9fs_symlink(pdu->tag, pdu->id, dfid, name.data, symname.data, gid); > > + if (name_has_illegal_characters(symname.data)) { > + err = -EINVAL; > + goto out_nofid; > + } > + > dfidp = get_fid(pdu, dfid); > if (dfidp == NULL) { > err = -EINVAL; > @@ -2316,6 +2340,11 @@ static void v9fs_link(void *opaque) > } > trace_v9fs_link(pdu->tag, pdu->id, dfid, oldfid, name.data); > > + if (name_has_illegal_characters(name.data)) { > + err = -EINVAL; > + goto out_nofid; > + } > + > dfidp = get_fid(pdu, dfid); > if (dfidp == NULL) { > err = -ENOENT; > @@ -2398,6 +2427,12 @@ static void v9fs_unlinkat(void *opaque) > if (err < 0) { > goto out_nofid; > } > + > + if (name_has_illegal_characters(name.data)) { > + err = -EINVAL; > + goto out_nofid; > + } > + > dfidp = get_fid(pdu, dfid); > if (dfidp == NULL) { > err = -EINVAL; > @@ -2504,6 +2539,12 @@ static void v9fs_rename(void *opaque) > if (err < 0) { > goto out_nofid; > } > + > + if (name_has_illegal_characters(name.data)) { > + err = -EINVAL; > + goto out_nofid; > + } > + > fidp = get_fid(pdu, fid); > if (fidp == NULL) { > err = -ENOENT; > @@ -2616,6 +2657,12 @@ static void v9fs_renameat(void *opaque) > goto out_err; > } > > + if (name_has_illegal_characters(old_name.data) || > + name_has_illegal_characters(new_name.data)) { > + err = -EINVAL; > + goto out_err; > + } > + > v9fs_path_write_lock(s); > err = v9fs_complete_renameat(pdu, olddirfid, > &old_name, newdirfid, &new_name); > @@ -2826,6 +2873,11 @@ static void v9fs_mknod(void *opaque) > } > trace_v9fs_mknod(pdu->tag, pdu->id, fid, mode, major, minor); > > + if (name_has_illegal_characters(name.data)) { > + err = -EINVAL; > + goto out_nofid; > + } > + > fidp = get_fid(pdu, fid); > if (fidp == NULL) { > err = -ENOENT; > @@ -2977,6 +3029,11 @@ static void v9fs_mkdir(void *opaque) > } > trace_v9fs_mkdir(pdu->tag, pdu->id, fid, name.data, mode, gid); > > + if (name_has_illegal_characters(name.data)) { > + err = -EINVAL; > + goto out_nofid; > + } > + > fidp = get_fid(pdu, fid); > if (fidp == NULL) { > err = -ENOENT; > @@ -3020,6 +3077,11 @@ static void v9fs_xattrwalk(void *opaque) > } > trace_v9fs_xattrwalk(pdu->tag, pdu->id, fid, newfid, name.data); > > + if (name_has_illegal_characters(name.data)) { > + err = -EINVAL; > + goto out_nofid; > + } > + > file_fidp = get_fid(pdu, fid); > if (file_fidp == NULL) { > err = -ENOENT; > @@ -3126,6 +3188,11 @@ static void v9fs_xattrcreate(void *opaque) > } > trace_v9fs_xattrcreate(pdu->tag, pdu->id, fid, name.data, size, flags); > > + if (name_has_illegal_characters(name.data)) { > + err = -EINVAL; > + goto out_nofid; > + } > + > file_fidp = get_fid(pdu, fid); > if (file_fidp == NULL) { > err = -EINVAL;
On 24 August 2016 at 17:40, Greg Kurz <groug@kaod.org> wrote: > On Wed, 24 Aug 2016 16:00:24 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: >> Do we also need ".." and "." to be illegal names (for at least most >> operations)? > I understand how ".." could be an issue, but I don't for "."... can you > please elaborate ? If you try to create, open, etc a file named "." then it won't create a file named "."; it'll do something wrong instead. So we shouldn't permit attempts to treat "." as an ordinary filename. (Basically the rationale is that for Linux the constraints on file and pathnames are only * no NULs * no / * "." is special * ".." is special * can't be the empty string We should reflect that in our error checking.) > 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. Yes, walk is the one special case that needs to handle "." and ".." (because for this operation they have a defined meaning that doesn't mean "just pass them through to the libc functions"). > 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 ? What did you want the empty string to mean? (We should probably also define the empty string as an illegal name). thanks -- PMM
On Wed, 24 Aug 2016 20:23:22 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 24 August 2016 at 17:40, Greg Kurz <groug@kaod.org> wrote: > > On Wed, 24 Aug 2016 16:00:24 +0100 > > Peter Maydell <peter.maydell@linaro.org> wrote: > >> Do we also need ".." and "." to be illegal names (for at least most > >> operations)? > > > I understand how ".." could be an issue, but I don't for "."... can you > > please elaborate ? > > If you try to create, open, etc a file named "." then it won't create a > file named "."; it'll do something wrong instead. So we shouldn't > permit attempts to treat "." as an ordinary filename. FWIW, on UNIX filesystems, "." cannot be created, nor deleted, nor symlinked to: mkdir(".", 0777) = -1 EEXIST (File exists) open(".", O_WRONLY|O_CREAT|O_TRUNC, 0666) = -1 EISDIR (Is a directory) rmdir(".") = -1 EINVAL (Invalid argument) symlink("foo", ".") = -1 EEXIST (File exists) I get the very same behaviour with the 9pfs local backend in QEMU (using the linux client and turning strings into "." with GDB). So I still don't get what "something wrong" refers to... but I'm not opposed to handling "." and ".." differently (see below). > > (Basically the rationale is that for Linux the constraints on > file and pathnames are only > * no NULs I have a separate patch for this as the spec forbids NUL for all strings, not only file and pathnames, as said in http://man.cat-v.org/plan_9/5/intro. > * no / > * "." is special > * ".." is special Indeed, that's what http://man.cat-v.org/plan_9/5/open says about the create request: The names . and .. are special; it is illegal to create files with these names. I hence agree these should be checked in the middle layer for create and lcreate. And even if they are not described in the spec, it probably also makes sense for all other operations except walk. I'll make it a separate patch since it is conceptually different than blacklisting illegal characters like /, which applies to all requests, including walk. > * can't be the empty string I could not find it in the spec but it makes sense indeed. Maybe this should be generalized as I could not find a valid use of the empty string in the 9P spec. > We should reflect that in our error checking.) > > > 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. > > Yes, walk is the one special case that needs to handle "." and ".." > (because for this operation they have a defined meaning that doesn't > mean "just pass them through to the libc functions"). > > > 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 ? > > What did you want the empty string to mean? (We should probably > also define the empty string as an illegal name). > You can ignore this remark since ".." and "." will be specifically ignored for all operations except walk. > thanks > -- PMM Thanks for your valuable help, Peter ! I'll try to come up with a new series ASAP. But since I'm still on holidays until the end of the week, don't feel forced to hold 2.7 for this if you don't hear from me. Cheers. -- Greg
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index b6b02b46a9da..1c008814509c 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1256,6 +1256,11 @@ static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t nwnames, V9fsQID *qids) return offset; } +static bool name_has_illegal_characters(const char *name) +{ + return strchr(name, '/') != NULL; +} + static void v9fs_walk(void *opaque) { int name_idx; @@ -1289,6 +1294,10 @@ static void v9fs_walk(void *opaque) if (err < 0) { goto out_nofid; } + if (name_has_illegal_characters(wnames[i].data)) { + err = -EINVAL; + goto out_nofid; + } offset += err; } } else if (nwnames > P9_MAXWELEM) { @@ -1483,6 +1492,11 @@ static void v9fs_lcreate(void *opaque) } trace_v9fs_lcreate(pdu->tag, pdu->id, dfid, flags, mode, gid); + if (name_has_illegal_characters(name.data)) { + err = -EINVAL; + goto out_nofid; + } + fidp = get_fid(pdu, dfid); if (fidp == NULL) { err = -ENOENT; @@ -2077,6 +2091,11 @@ static void v9fs_create(void *opaque) } trace_v9fs_create(pdu->tag, pdu->id, fid, name.data, perm, mode); + if (name_has_illegal_characters(name.data)) { + err = -EINVAL; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -EINVAL; @@ -2242,6 +2261,11 @@ static void v9fs_symlink(void *opaque) } trace_v9fs_symlink(pdu->tag, pdu->id, dfid, name.data, symname.data, gid); + if (name_has_illegal_characters(symname.data)) { + err = -EINVAL; + goto out_nofid; + } + dfidp = get_fid(pdu, dfid); if (dfidp == NULL) { err = -EINVAL; @@ -2316,6 +2340,11 @@ static void v9fs_link(void *opaque) } trace_v9fs_link(pdu->tag, pdu->id, dfid, oldfid, name.data); + if (name_has_illegal_characters(name.data)) { + err = -EINVAL; + goto out_nofid; + } + dfidp = get_fid(pdu, dfid); if (dfidp == NULL) { err = -ENOENT; @@ -2398,6 +2427,12 @@ static void v9fs_unlinkat(void *opaque) if (err < 0) { goto out_nofid; } + + if (name_has_illegal_characters(name.data)) { + err = -EINVAL; + goto out_nofid; + } + dfidp = get_fid(pdu, dfid); if (dfidp == NULL) { err = -EINVAL; @@ -2504,6 +2539,12 @@ static void v9fs_rename(void *opaque) if (err < 0) { goto out_nofid; } + + if (name_has_illegal_characters(name.data)) { + err = -EINVAL; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT; @@ -2616,6 +2657,12 @@ static void v9fs_renameat(void *opaque) goto out_err; } + if (name_has_illegal_characters(old_name.data) || + name_has_illegal_characters(new_name.data)) { + err = -EINVAL; + goto out_err; + } + v9fs_path_write_lock(s); err = v9fs_complete_renameat(pdu, olddirfid, &old_name, newdirfid, &new_name); @@ -2826,6 +2873,11 @@ static void v9fs_mknod(void *opaque) } trace_v9fs_mknod(pdu->tag, pdu->id, fid, mode, major, minor); + if (name_has_illegal_characters(name.data)) { + err = -EINVAL; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT; @@ -2977,6 +3029,11 @@ static void v9fs_mkdir(void *opaque) } trace_v9fs_mkdir(pdu->tag, pdu->id, fid, name.data, mode, gid); + if (name_has_illegal_characters(name.data)) { + err = -EINVAL; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT; @@ -3020,6 +3077,11 @@ static void v9fs_xattrwalk(void *opaque) } trace_v9fs_xattrwalk(pdu->tag, pdu->id, fid, newfid, name.data); + if (name_has_illegal_characters(name.data)) { + err = -EINVAL; + goto out_nofid; + } + file_fidp = get_fid(pdu, fid); if (file_fidp == NULL) { err = -ENOENT; @@ -3126,6 +3188,11 @@ static void v9fs_xattrcreate(void *opaque) } trace_v9fs_xattrcreate(pdu->tag, pdu->id, fid, name.data, size, flags); + if (name_has_illegal_characters(name.data)) { + err = -EINVAL; + goto out_nofid; + } + file_fidp = get_fid(pdu, fid); if (file_fidp == NULL) { err = -EINVAL;
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. If a client sends a path component with an illegal character, the request will fail and EINVAL is returned to the client. For the sake of simplicity and consistency, the check is done at top-level for all affected 9P requests: - xattrwalk - xattrcreate - mknod - rename - renameat - unlinkat - mkdir - walk - link - symlink - create - lcreate [1] http://man.cat-v.org/plan_9/5/walk [2] http://man.cat-v.org/plan_9/5/intro Reported-by: Felix Wilhelm <fwilhelm@ernw.de> Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Greg Kurz <groug@kaod.org> --- Since the linux client does not send / in path components and I don't have enough time to write an appropriate qtest, I choosed to do manual testing of the mkdir request with GDB: [greg@vm66 host]$ mkdir ...foo (then turning ...foo into ../foo in QEMU with GDB) mkdir: cannot create directory ‘...foo’: Invalid argument I also could run the POSIX file system test suite from TUXERA: http://www.tuxera.com/community/open-source-posix/ and did not observe any regression with this patch. hw/9pfs/9p.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+)