| Message ID | 147257705420.28515.6347449121724165834.stgit@bahia.lab.toulouse-stg.fr.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 08/30/2016 12:11 PM, Greg Kurz wrote: > Empty path components don't make sense for most commands and may cause > undefined behavior, depending on the backend. > > Also, 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_is_illegal() helper that checks the > names sent by the client are not empty and don't contain unwanted chars. > 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 an illegal path component, the request will fail and > ENOENT is returned to the client. > > [1] http://man.cat-v.org/plan_9/5/walk > [2] http://man.cat-v.org/plan_9/5/intro > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > v4: dropped the checking of the symbolic link target name: because a target > can be a full path and thus contain '/' and linux already complains if > it is an empty string. When the symlink gets dereferenced, slashes are > interpreted as the usual path component separator. Can a symlink to "/foo" be used to escape the root (by being absolute instead of relative)? However, if the answer to that question requires more code, I'm fine with it being a separate patch. So for this email, Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue, 30 Aug 2016 13:03:40 -0500 Eric Blake <eblake@redhat.com> wrote: > On 08/30/2016 12:11 PM, Greg Kurz wrote: > > Empty path components don't make sense for most commands and may cause > > undefined behavior, depending on the backend. > > > > Also, 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_is_illegal() helper that checks the > > names sent by the client are not empty and don't contain unwanted chars. > > 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 an illegal path component, the request will fail and > > ENOENT is returned to the client. > > > > [1] http://man.cat-v.org/plan_9/5/walk > > [2] http://man.cat-v.org/plan_9/5/intro > > > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > v4: dropped the checking of the symbolic link target name: because a target > > can be a full path and thus contain '/' and linux already complains if > > it is an empty string. When the symlink gets dereferenced, slashes are > > interpreted as the usual path component separator. > > Can a symlink to "/foo" be used to escape the root (by being absolute No it can't because the target isn't a actually a file name but a string that will be translated to a path when the link is dereferenced. And all other requests with a file name argument that could have some unwanted effect don't allow '/' in file names. > instead of relative)? However, if the answer to that question requires > more code, I'm fine with it being a separate patch. So for this email, > > Reviewed-by: Eric Blake <eblake@redhat.com> >
Eric Blake <eblake@redhat.com> writes: > [ Unknown signature status ] > On 08/30/2016 12:11 PM, Greg Kurz wrote: >> Empty path components don't make sense for most commands and may cause >> undefined behavior, depending on the backend. >> >> Also, 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_is_illegal() helper that checks the >> names sent by the client are not empty and don't contain unwanted chars. >> 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 an illegal path component, the request will fail and >> ENOENT is returned to the client. >> >> [1] http://man.cat-v.org/plan_9/5/walk >> [2] http://man.cat-v.org/plan_9/5/intro >> >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Greg Kurz <groug@kaod.org> >> --- >> v4: dropped the checking of the symbolic link target name: because a target >> can be a full path and thus contain '/' and linux already complains if >> it is an empty string. When the symlink gets dereferenced, slashes are >> interpreted as the usual path component separator. > > Can a symlink to "/foo" be used to escape the root (by being absolute > instead of relative)? However, if the answer to that question requires > more code, I'm fine with it being a separate patch. So for this email, We resolve "/foo" on the client side. So this is ok. -aneesh
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index b6b02b46a9da..385269ea0ac3 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_is_illegal(const char *name) +{ + return !*name || 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_is_illegal(wnames[i].data)) { + err = -ENOENT; + 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_is_illegal(name.data)) { + err = -ENOENT; + 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_is_illegal(name.data)) { + err = -ENOENT; + 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_is_illegal(name.data)) { + err = -ENOENT; + 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_is_illegal(name.data)) { + err = -ENOENT; + 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_is_illegal(name.data)) { + err = -ENOENT; + 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_is_illegal(name.data)) { + err = -ENOENT; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT; @@ -2616,6 +2657,11 @@ static void v9fs_renameat(void *opaque) goto out_err; } + if (name_is_illegal(old_name.data) || name_is_illegal(new_name.data)) { + err = -ENOENT; + goto out_err; + } + v9fs_path_write_lock(s); err = v9fs_complete_renameat(pdu, olddirfid, &old_name, newdirfid, &new_name); @@ -2826,6 +2872,11 @@ static void v9fs_mknod(void *opaque) } trace_v9fs_mknod(pdu->tag, pdu->id, fid, mode, major, minor); + if (name_is_illegal(name.data)) { + err = -ENOENT; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT; @@ -2977,6 +3028,11 @@ static void v9fs_mkdir(void *opaque) } trace_v9fs_mkdir(pdu->tag, pdu->id, fid, name.data, mode, gid); + if (name_is_illegal(name.data)) { + err = -ENOENT; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT;
Empty path components don't make sense for most commands and may cause undefined behavior, depending on the backend. Also, 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_is_illegal() helper that checks the names sent by the client are not empty and don't contain unwanted chars. 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 an illegal path component, the request will fail and ENOENT is returned to the client. [1] http://man.cat-v.org/plan_9/5/walk [2] http://man.cat-v.org/plan_9/5/intro Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Greg Kurz <groug@kaod.org> --- v4: dropped the checking of the symbolic link target name: because a target can be a full path and thus contain '/' and linux already complains if it is an empty string. When the symlink gets dereferenced, slashes are interpreted as the usual path component separator. --- hw/9pfs/9p.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)