[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/5] 9p: forbid illegal path names
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/5] 9p: forbid illegal path names |
Date: |
Sun, 28 Aug 2016 15:11:20 +0200 |
On Fri, 26 Aug 2016 13:33:28 -0500
Eric Blake <address@hidden> wrote:
> On 08/26/2016 10:07 AM, Greg Kurz wrote:
> > Empty path components don't make sense 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
> > EINVAL 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 <address@hidden>
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> > hw/9pfs/9p.c | 56
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 56 insertions(+)
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index b6b02b46a9da..dba11773699b 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 == NULL || strchr(name, '/') != NULL;
>
> Is anyone actually passing NULL? And the commit message says you are
> blacklisting the empty string "", but that is not what you did here.
> Depending on whether callers can even pass NULL, you may be able to just
> s/name == NULL/!*name/
>
Oops you're right, v9fs_iov_vunmarshal() always allocates and sets str->data,
even
for empty strings... so I guess this cannot be call with NULL and your
suggestion
is the way to go.
pgpedwu5wK7S9.pgp
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 0/5] 9P security fixes, Greg Kurz, 2016/08/26
- [Qemu-devel] [PATCH v2 1/5] 9p: forbid illegal path names, Greg Kurz, 2016/08/26
- [Qemu-devel] [PATCH v2 2/5] 9p: disallow the NUL character in all strings, Greg Kurz, 2016/08/26
- [Qemu-devel] [PATCH v2 3/5] 9p: forbid . and .. in file names, Greg Kurz, 2016/08/26
- [Qemu-devel] [PATCH v2 4/5] 9p: handle walk of ".." in the root directory, Greg Kurz, 2016/08/26
- [Qemu-devel] [PATCH v2 5/5] 9p: forbid empty extension string, Greg Kurz, 2016/08/26