[Qemu-devel] [PATCH v4 0/3] 9pfs security fixes

[Qemu-devel] [PATCH v4 0/3] 9pfs security fixes

[Greg Kurz]

As reported by Felix Wilhelm, 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 series
tries to address the issue more globally, based on the official 9P spec.

I wasn't running the TUXERA test suite correctly and overlooked a failure
with symbolic links (thanks Aneesh for your assistance). This v4 is basically
the same as v3 with a change in patch 1/3.

---

Greg Kurz (3):
      9pfs: forbid illegal path names
      9pfs: forbid . and .. in file names
      9pfs: handle walk of ".." in the root directory


 hw/9pfs/9p.c |  147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 hw/9pfs/9p.h |    1 
 2 files changed, 139 insertions(+), 9 deletions(-)

--
Greg

[Qemu-devel] [PATCH v4 1/3] 9pfs: forbid illegal path names

[Greg Kurz]

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(+)

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;

[Qemu-devel] [PATCH v4 2/3] 9pfs: forbid . and .. in file names

[Greg Kurz]

According to the 9P spec http://man.cat-v.org/plan_9/5/open about the
create request:

The names . and .. are special; it is illegal to create files with these
names.

This patch causes the create and lcreate requests to fail with EINVAL if
the file name is either "." or "..".

Even if it isn't explicitly written in the spec, this patch extends the
checking to all requests that may cause a directory entry to be created:

    - mknod
    - rename
    - renameat
    - mkdir
    - link
    - symlink

The unlinkat request also gets patched for consistency (even if
rmdir("foo/..") is expected to fail according to POSIX.1-2001).

The various error values come from the linux manual pages.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Greg Kurz <groug@kaod.org>

v3: - rename and renameat now return EISDIR instead of EBUSY
---
 hw/9pfs/9p.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 385269ea0ac3..51c6f9883bf8 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1497,6 +1497,11 @@ static void v9fs_lcreate(void *opaque)
         goto out_nofid;
     }
 
+    if (!strcmp(".", name.data) || !strcmp("..", name.data)) {
+        err = -EEXIST;
+        goto out_nofid;
+    }
+
     fidp = get_fid(pdu, dfid);
     if (fidp == NULL) {
         err = -ENOENT;
@@ -2096,6 +2101,11 @@ static void v9fs_create(void *opaque)
         goto out_nofid;
     }
 
+    if (!strcmp(".", name.data) || !strcmp("..", name.data)) {
+        err = -EEXIST;
+        goto out_nofid;
+    }
+
     fidp = get_fid(pdu, fid);
     if (fidp == NULL) {
         err = -EINVAL;
@@ -2266,6 +2276,11 @@ static void v9fs_symlink(void *opaque)
         goto out_nofid;
     }
 
+    if (!strcmp(".", name.data) || !strcmp("..", name.data)) {
+        err = -EEXIST;
+        goto out_nofid;
+    }
+
     dfidp = get_fid(pdu, dfid);
     if (dfidp == NULL) {
         err = -EINVAL;
@@ -2345,6 +2360,11 @@ static void v9fs_link(void *opaque)
         goto out_nofid;
     }
 
+    if (!strcmp(".", name.data) || !strcmp("..", name.data)) {
+        err = -EEXIST;
+        goto out_nofid;
+    }
+
     dfidp = get_fid(pdu, dfid);
     if (dfidp == NULL) {
         err = -ENOENT;
@@ -2433,6 +2453,16 @@ static void v9fs_unlinkat(void *opaque)
         goto out_nofid;
     }
 
+    if (!strcmp(".", name.data)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
+    if (!strcmp("..", name.data)) {
+        err = -ENOTEMPTY;
+        goto out_nofid;
+    }
+
     dfidp = get_fid(pdu, dfid);
     if (dfidp == NULL) {
         err = -EINVAL;
@@ -2545,6 +2575,11 @@ static void v9fs_rename(void *opaque)
         goto out_nofid;
     }
 
+    if (!strcmp(".", name.data) || !strcmp("..", name.data)) {
+        err = -EISDIR;
+        goto out_nofid;
+    }
+
     fidp = get_fid(pdu, fid);
     if (fidp == NULL) {
         err = -ENOENT;
@@ -2662,6 +2697,12 @@ static void v9fs_renameat(void *opaque)
         goto out_err;
     }
 
+    if (!strcmp(".", old_name.data) || !strcmp("..", old_name.data) ||
+        !strcmp(".", new_name.data) || !strcmp("..", new_name.data)) {
+        err = -EISDIR;
+        goto out_err;
+    }
+
     v9fs_path_write_lock(s);
     err = v9fs_complete_renameat(pdu, olddirfid,
                                  &old_name, newdirfid, &new_name);
@@ -2877,6 +2918,11 @@ static void v9fs_mknod(void *opaque)
         goto out_nofid;
     }
 
+    if (!strcmp(".", name.data) || !strcmp("..", name.data)) {
+        err = -EEXIST;
+        goto out_nofid;
+    }
+
     fidp = get_fid(pdu, fid);
     if (fidp == NULL) {
         err = -ENOENT;
@@ -3033,6 +3079,11 @@ static void v9fs_mkdir(void *opaque)
         goto out_nofid;
     }
 
+    if (!strcmp(".", name.data) || !strcmp("..", name.data)) {
+        err = -EEXIST;
+        goto out_nofid;
+    }
+
     fidp = get_fid(pdu, fid);
     if (fidp == NULL) {
         err = -ENOENT;

Re: [Qemu-devel] [PATCH v4 1/3] 9pfs: forbid illegal path names

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1959 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 2/3] 9pfs: forbid . and .. in file names

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

On 08/30/2016 12:13 PM, Greg Kurz wrote:
> According to the 9P spec http://man.cat-v.org/plan_9/5/open about the
> create request:
> 
> The names . and .. are special; it is illegal to create files with these
> names.
> 
> This patch causes the create and lcreate requests to fail with EINVAL if
> the file name is either "." or "..".
> 
> Even if it isn't explicitly written in the spec, this patch extends the
> checking to all requests that may cause a directory entry to be created:
> 
>     - mknod
>     - rename
>     - renameat
>     - mkdir
>     - link
>     - symlink
> 
> The unlinkat request also gets patched for consistency (even if
> rmdir("foo/..") is expected to fail according to POSIX.1-2001).
> 
> The various error values come from the linux manual pages.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> v3: - rename and renameat now return EISDIR instead of EBUSY

The v3 comment could occur after the '---' separator.

> ---
>  hw/9pfs/9p.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)

Maintainer can touch that up, then add
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 0/3] 9pfs security fixes

[Michael S. Tsirkin]

On Tue, Aug 30, 2016 at 07:10:47PM +0200, Greg Kurz wrote:
> As reported by Felix Wilhelm, 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 series
> tries to address the issue more globally, based on the official 9P spec.
> 
> I wasn't running the TUXERA test suite correctly and overlooked a failure
> with symbolic links (thanks Aneesh for your assistance). This v4 is basically
> the same as v3 with a change in patch 1/3.

Looks good to me.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>



> ---
> 
> Greg Kurz (3):
>       9pfs: forbid illegal path names
>       9pfs: forbid . and .. in file names
>       9pfs: handle walk of ".." in the root directory
> 
> 
>  hw/9pfs/9p.c |  147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  hw/9pfs/9p.h |    1 
>  2 files changed, 139 insertions(+), 9 deletions(-)
> 
> --
> Greg

Re: [Qemu-devel] [PATCH v4 0/3] 9pfs security fixes

[Peter Maydell]

On 30 August 2016 at 18:10, Greg Kurz <groug@kaod.org> wrote:
> As reported by Felix Wilhelm, 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 series
> tries to address the issue more globally, based on the official 9P spec.
>
> I wasn't running the TUXERA test suite correctly and overlooked a failure
> with symbolic links (thanks Aneesh for your assistance). This v4 is basically
> the same as v3 with a change in patch 1/3.
>
> ---
>
> Greg Kurz (3):
>       9pfs: forbid illegal path names
>       9pfs: forbid . and .. in file names
>       9pfs: handle walk of ".." in the root directory

I see the cover letter and patches 1 and 2 in my email client
and in patchwork. Where is patch 3? (If it's identical to the v3
patch 3 I can get that...)

thanks
-- PMM

[Qemu-devel] [PATCH v4 3/3] 9pfs: handle walk of ".." in the root directory

[Greg Kurz]

The 9P spec at http://man.cat-v.org/plan_9/5/intro says:

All directories must support walks to the directory .. (dot-dot) meaning
parent directory, although by convention directories contain no explicit
entry for .. or . (dot).  The parent of the root directory of a server's
tree is itself.

This means that a client cannot walk further than the root directory
exported by the server. In other words, if the client wants to walk
"/.." or "/foo/../..", the server should answer like the request was
to walk "/".

This patch just does that:
- we cache the QID of the root directory at attach time
- during the walk we compare the QID of each path component with the root
  QID to detect if we're in a "/.." situation
- if so, we skip the current component and go to the next one

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Eric Blake <eblake@redhat.com>

V3: - fixed typo in changelog
    - added Eric's R-b tag
---
 hw/9pfs/9p.c |   40 +++++++++++++++++++++++++++++++---------
 hw/9pfs/9p.h |    1 +
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 51c6f9883bf8..dfe293d11d1c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1010,6 +1010,7 @@ static void v9fs_attach(void *opaque)
         goto out;
     }
     err += offset;
+    memcpy(&s->root_qid, &qid, sizeof(qid));
     trace_v9fs_attach_return(pdu->tag, pdu->id,
                              qid.type, qid.version, qid.path);
     /*
@@ -1261,6 +1262,14 @@ static bool name_is_illegal(const char *name)
     return !*name || strchr(name, '/') != NULL;
 }
 
+static bool not_same_qid(const V9fsQID *qid1, const V9fsQID *qid2)
+{
+    return
+        qid1->type != qid2->type ||
+        qid1->version != qid2->version ||
+        qid1->path != qid2->path;
+}
+
 static void v9fs_walk(void *opaque)
 {
     int name_idx;
@@ -1276,6 +1285,7 @@ static void v9fs_walk(void *opaque)
     V9fsFidState *newfidp = NULL;
     V9fsPDU *pdu = opaque;
     V9fsState *s = pdu->s;
+    V9fsQID qid;
 
     err = pdu_unmarshal(pdu, offset, "ddw", &fid, &newfid, &nwnames);
     if (err < 0) {
@@ -1309,6 +1319,12 @@ static void v9fs_walk(void *opaque)
         err = -ENOENT;
         goto out_nofid;
     }
+
+    err = fid_to_qid(pdu, fidp, &qid);
+    if (err < 0) {
+        goto out;
+    }
+
     v9fs_path_init(&dpath);
     v9fs_path_init(&path);
     /*
@@ -1318,16 +1334,22 @@ static void v9fs_walk(void *opaque)
     v9fs_path_copy(&dpath, &fidp->path);
     v9fs_path_copy(&path, &fidp->path);
     for (name_idx = 0; name_idx < nwnames; name_idx++) {
-        err = v9fs_co_name_to_path(pdu, &dpath, wnames[name_idx].data, &path);
-        if (err < 0) {
-            goto out;
-        }
-        err = v9fs_co_lstat(pdu, &path, &stbuf);
-        if (err < 0) {
-            goto out;
+        if (not_same_qid(&pdu->s->root_qid, &qid) ||
+            strcmp("..", wnames[name_idx].data)) {
+            err = v9fs_co_name_to_path(pdu, &dpath, wnames[name_idx].data,
+                                       &path);
+            if (err < 0) {
+                goto out;
+            }
+
+            err = v9fs_co_lstat(pdu, &path, &stbuf);
+            if (err < 0) {
+                goto out;
+            }
+            stat_to_qid(&stbuf, &qid);
+            v9fs_path_copy(&dpath, &path);
         }
-        stat_to_qid(&stbuf, &qids[name_idx]);
-        v9fs_path_copy(&dpath, &path);
+        memcpy(&qids[name_idx], &qid, sizeof(qid));
     }
     if (fid == newfid) {
         BUG_ON(fidp->fid_type != P9_FID_NONE);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index b4f757ab5449..a38603398ef5 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -236,6 +236,7 @@ typedef struct V9fsState
     int32_t root_fid;
     Error *migration_blocker;
     V9fsConf fsconf;
+    V9fsQID root_qid;
 } V9fsState;
 
 /* 9p2000.L open flags */

Re: [Qemu-devel] [PATCH v4 2/3] 9pfs: forbid . and .. in file names

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

On Tue, 30 Aug 2016 13:06:31 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 08/30/2016 12:13 PM, Greg Kurz wrote:
> > According to the 9P spec http://man.cat-v.org/plan_9/5/open about the
> > create request:
> > 
> > The names . and .. are special; it is illegal to create files with these
> > names.
> > 
> > This patch causes the create and lcreate requests to fail with EINVAL if
> > the file name is either "." or "..".
> > 
> > Even if it isn't explicitly written in the spec, this patch extends the
> > checking to all requests that may cause a directory entry to be created:
> > 
> >     - mknod
> >     - rename
> >     - renameat
> >     - mkdir
> >     - link
> >     - symlink
> > 
> > The unlinkat request also gets patched for consistency (even if
> > rmdir("foo/..") is expected to fail according to POSIX.1-2001).
> > 
> > The various error values come from the linux manual pages.
> > 
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > 
> > v3: - rename and renameat now return EISDIR instead of EBUSY  
> 
> The v3 comment could occur after the '---' separator.
> 

Yes of course. Sorry for the other patches as well :)

> > ---
> >  hw/9pfs/9p.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)  
> 
> Maintainer can touch that up, then add
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v4 1/3] 9pfs: forbid illegal path names

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v4 0/3] 9pfs security fixes

[Peter Maydell]

On 30 August 2016 at 14:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 August 2016 at 18:10, Greg Kurz <groug@kaod.org> wrote:
>> As reported by Felix Wilhelm, 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 series
>> tries to address the issue more globally, based on the official 9P spec.
>>
>> I wasn't running the TUXERA test suite correctly and overlooked a failure
>> with symbolic links (thanks Aneesh for your assistance). This v4 is basically
>> the same as v3 with a change in patch 1/3.
>>
>> ---
>>
>> Greg Kurz (3):
>>       9pfs: forbid illegal path names
>>       9pfs: forbid . and .. in file names
>>       9pfs: handle walk of ".." in the root directory
>
> I see the cover letter and patches 1 and 2 in my email client
> and in patchwork. Where is patch 3? (If it's identical to the v3
> patch 3 I can get that...)

Ah, it just arrived. Applied all to master, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v4 1/3] 9pfs: forbid illegal path names

[Aneesh Kumar K.V]

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

Re: [Qemu-devel] [PATCH v4 0/3] 9pfs security fixes

[Greg Kurz]

On Tue, 30 Aug 2016 15:39:13 -0400
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 30 August 2016 at 14:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 30 August 2016 at 18:10, Greg Kurz <groug@kaod.org> wrote:  
> >> As reported by Felix Wilhelm, 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 series
> >> tries to address the issue more globally, based on the official 9P spec.
> >>
> >> I wasn't running the TUXERA test suite correctly and overlooked a failure
> >> with symbolic links (thanks Aneesh for your assistance). This v4 is basically
> >> the same as v3 with a change in patch 1/3.
> >>
> >> ---
> >>
> >> Greg Kurz (3):
> >>       9pfs: forbid illegal path names
> >>       9pfs: forbid . and .. in file names
> >>       9pfs: handle walk of ".." in the root directory  
> >
> > I see the cover letter and patches 1 and 2 in my email client
> > and in patchwork. Where is patch 3? (If it's identical to the v3
> > patch 3 I can get that...)  
> 
> Ah, it just arrived. Applied all to master, thanks.
> 
> -- PMM
> 

FWIW, this also applies to 2.6.1.

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH v4 3/3] 9pfs: handle walk of ".." in the root directory

[Paolo Bonzini]

On 30/08/2016 20:40, Greg Kurz wrote:
> +
> +    err = fid_to_qid(pdu, fidp, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
> +
>      v9fs_path_init(&dpath);
>      v9fs_path_init(&path);

The "out" label can now be reached without having initialized dpath and
path.  This upsets Coverity (and might also cause a segfault, indeed).

The simplest fix is to move the v9fs_path_init before the fid_to_qid call.

Paolo

>      /*
> @@ -1318,16 +1334,22 @@ static void v9fs_walk(void *opaque)
>      v9fs_path_copy(&dpath, &fidp->path);
>      v9fs_path_copy(&path, &fidp->path);
>      for (name_idx = 0; name_idx < nwnames; name_idx++) {
> -        err = v9fs_co_name_to_path(pdu, &dpath, wnames[name_idx].data, &path);
> -        if (err < 0) {
> -            goto out;
> -        }
> -        err = v9fs_co_lstat(pdu, &path, &stbuf);
> -        if (err < 0) {
> -            goto out;
> +        if (not_same_qid(&pdu->s->root_qid, &qid) ||
> +            strcmp("..", wnames[name_idx].data)) {
> +            err = v9fs_co_name_to_path(pdu, &dpath, wnames[name_idx].data,
> +                                       &path);
> +            if (err < 0) {
> +                goto out;
> +            }
> +
> +            err = v9fs_co_lstat(pdu, &path, &stbuf);
> +            if (err < 0) {
> +                goto out;
> +            }
> +            stat_to_qid(&stbuf, &qid);
> +            v9fs_path_copy(&dpath, &path);
>          }
> -        stat_to_qid(&stbuf, &qids[name_idx]);
> -        v9fs_path_copy(&dpath, &path);
> +        memcpy(&qids[name_idx], &qid, sizeof(qid));
>      }
>      if (fid == newfid) {
>          BUG_ON(fidp->fid_type != P9_FID_NONE);
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index b4f757ab5449..a38603398ef5 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -236,6 +236,7 @@ typedef struct V9fsState
>      int32_t root_fid;
>      Error *migration_blocker;
>      V9fsConf fsconf;
> +    V9fsQID root_qid;
>  } V9fsState;
>  
>  /* 9p2000.L open flags */
> 
> 
>