From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58673) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bccpi-0008Nb-Ux for qemu-devel@nongnu.org; Wed, 24 Aug 2016 14:23:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bccpe-0000ar-3Y for qemu-devel@nongnu.org; Wed, 24 Aug 2016 14:23:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46904) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bccpd-0000ak-RO for qemu-devel@nongnu.org; Wed, 24 Aug 2016 14:23:22 -0400 Date: Wed, 24 Aug 2016 21:23:20 +0300 From: "Michael S. Tsirkin" Message-ID: <20160824212312-mutt-send-email-mst@kernel.org> References: <147204811781.25757.13905475486785615296.stgit@bahia.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <147204811781.25757.13905475486785615296.stgit@bahia.lan> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] 9pfs: disallow / in path components List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: qemu-devel@nongnu.org, Peter Maydell , Felix Wilhelm , P J P , "Aneesh Kumar K.V" 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 gu= est > originated string to the export path. A malicious guest could forge a > relative path and access files outside the export path. >=20 > 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 pa= tch > tries to address the issue more globally, based on the official 9P spec= . >=20 > The walk request described in the 9P spec [1] clearly shows that the cl= ient > is supposed to send individual path components: the official linux clie= nt > never sends portions of path containing the / character for example. >=20 > Moreover, the 9P spec [2] also states that a system can decide to restr= ict > the set of supported characters used in path components, with an explic= it > mention "to remove slashes from name components". >=20 > This patch introduces a new name_has_illegal_characters() helper that l= ooks > 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. >=20 > If a client sends a path component with an illegal character, the reque= st > will fail and EINVAL is returned to the client. >=20 > For the sake of simplicity and consistency, the check is done at top-le= vel > for all affected 9P requests: >=20 > - xattrwalk > - xattrcreate > - mknod > - rename > - renameat > - unlinkat > - mkdir > - walk > - link > - symlink > - create > - lcreate >=20 > [1] http://man.cat-v.org/plan_9/5/walk > [2] http://man.cat-v.org/plan_9/5/intro >=20 > Reported-by: Felix Wilhelm > Suggested-by: Peter Maydell > Signed-off-by: Greg Kurz Acked-by: Michael S. Tsirkin > --- >=20 > 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: >=20 > [greg@vm66 host]$ mkdir ...foo > (then turning ...foo into ../foo in QEMU with GDB) > mkdir: cannot create directory =E2=80=98...foo=E2=80=99: Invalid argume= nt >=20 > I also could run the POSIX file system test suite from TUXERA: >=20 > http://www.tuxera.com/community/open-source-posix/ >=20 > and did not observe any regression with this patch. >=20 > hw/9pfs/9p.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++= ++++++++ > 1 file changed, 67 insertions(+) >=20 > 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, uint1= 6_t nwnames, V9fsQID *qids) > return offset; > } > =20 > +static bool name_has_illegal_characters(const char *name) > +{ > + return strchr(name, '/') !=3D 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 =3D -EINVAL; > + goto out_nofid; > + } > offset +=3D 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); > =20 > + if (name_has_illegal_characters(name.data)) { > + err =3D -EINVAL; > + goto out_nofid; > + } > + > fidp =3D get_fid(pdu, dfid); > if (fidp =3D=3D NULL) { > err =3D -ENOENT; > @@ -2077,6 +2091,11 @@ static void v9fs_create(void *opaque) > } > trace_v9fs_create(pdu->tag, pdu->id, fid, name.data, perm, mode); > =20 > + if (name_has_illegal_characters(name.data)) { > + err =3D -EINVAL; > + goto out_nofid; > + } > + > fidp =3D get_fid(pdu, fid); > if (fidp =3D=3D NULL) { > err =3D -EINVAL; > @@ -2242,6 +2261,11 @@ static void v9fs_symlink(void *opaque) > } > trace_v9fs_symlink(pdu->tag, pdu->id, dfid, name.data, symname.dat= a, gid); > =20 > + if (name_has_illegal_characters(symname.data)) { > + err =3D -EINVAL; > + goto out_nofid; > + } > + > dfidp =3D get_fid(pdu, dfid); > if (dfidp =3D=3D NULL) { > err =3D -EINVAL; > @@ -2316,6 +2340,11 @@ static void v9fs_link(void *opaque) > } > trace_v9fs_link(pdu->tag, pdu->id, dfid, oldfid, name.data); > =20 > + if (name_has_illegal_characters(name.data)) { > + err =3D -EINVAL; > + goto out_nofid; > + } > + > dfidp =3D get_fid(pdu, dfid); > if (dfidp =3D=3D NULL) { > err =3D -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 =3D -EINVAL; > + goto out_nofid; > + } > + > dfidp =3D get_fid(pdu, dfid); > if (dfidp =3D=3D NULL) { > err =3D -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 =3D -EINVAL; > + goto out_nofid; > + } > + > fidp =3D get_fid(pdu, fid); > if (fidp =3D=3D NULL) { > err =3D -ENOENT; > @@ -2616,6 +2657,12 @@ static void v9fs_renameat(void *opaque) > goto out_err; > } > =20 > + if (name_has_illegal_characters(old_name.data) || > + name_has_illegal_characters(new_name.data)) { > + err =3D -EINVAL; > + goto out_err; > + } > + > v9fs_path_write_lock(s); > err =3D 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); > =20 > + if (name_has_illegal_characters(name.data)) { > + err =3D -EINVAL; > + goto out_nofid; > + } > + > fidp =3D get_fid(pdu, fid); > if (fidp =3D=3D NULL) { > err =3D -ENOENT; > @@ -2977,6 +3029,11 @@ static void v9fs_mkdir(void *opaque) > } > trace_v9fs_mkdir(pdu->tag, pdu->id, fid, name.data, mode, gid); > =20 > + if (name_has_illegal_characters(name.data)) { > + err =3D -EINVAL; > + goto out_nofid; > + } > + > fidp =3D get_fid(pdu, fid); > if (fidp =3D=3D NULL) { > err =3D -ENOENT; > @@ -3020,6 +3077,11 @@ static void v9fs_xattrwalk(void *opaque) > } > trace_v9fs_xattrwalk(pdu->tag, pdu->id, fid, newfid, name.data); > =20 > + if (name_has_illegal_characters(name.data)) { > + err =3D -EINVAL; > + goto out_nofid; > + } > + > file_fidp =3D get_fid(pdu, fid); > if (file_fidp =3D=3D NULL) { > err =3D -ENOENT; > @@ -3126,6 +3188,11 @@ static void v9fs_xattrcreate(void *opaque) > } > trace_v9fs_xattrcreate(pdu->tag, pdu->id, fid, name.data, size, fl= ags); > =20 > + if (name_has_illegal_characters(name.data)) { > + err =3D -EINVAL; > + goto out_nofid; > + } > + > file_fidp =3D get_fid(pdu, fid); > if (file_fidp =3D=3D NULL) { > err =3D -EINVAL;