| Message ID | 148405873540.9522.2224634666374633695.stgit@bahia.lab.toulouse-stg.fr.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 01/10/2017 08:32 AM, Greg Kurz wrote: > It really does not make sense for the 9P server to open anything else but > a regular file or a directory. > > Malicious code in a guest could for example create a named pipe, associate > it to a valid fid and pass it to the server in a RLOPEN message. This would > cause QEMU to hang in open(), waiting for someone to open the other end of > the pipe. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index fa58877570f6..edd7b97270e3 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -1462,7 +1462,7 @@ static void coroutine_fn v9fs_open(void *opaque) > goto out; > } > err += offset; > - } else { > + } else if (S_ISREG(stbuf.st_mode)) { > if (s->proto_version == V9FS_PROTO_2000L) { TOCTTOU race. You are checking the stat() results and only then calling open(), rather than calling open() first and validating fstat(). That means the guest can STILL cause you to open() a pipe by changing the file type in between the stat and the open. I think you need to rework this patch to open() first, then validate (closing the fd if necessary); the open can be done with O_NONBLOCK to avoid hanging on a pipe. Yes, that's more annoying, but that's life with TOCTTOU races.
On 01/10/2017 08:38 AM, Eric Blake wrote: > I think you need to rework this patch to open() first, then validate > (closing the fd if necessary); the open can be done with O_NONBLOCK to > avoid hanging on a pipe. And O_NOCTTY to avoid becoming the controlling process of a terminal.
On Tue, 10 Jan 2017 08:38:27 -0600 Eric Blake <eblake@redhat.com> wrote: > On 01/10/2017 08:32 AM, Greg Kurz wrote: > > It really does not make sense for the 9P server to open anything else but > > a regular file or a directory. > > > > Malicious code in a guest could for example create a named pipe, associate > > it to a valid fid and pass it to the server in a RLOPEN message. This would > > cause QEMU to hang in open(), waiting for someone to open the other end of > > the pipe. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index fa58877570f6..edd7b97270e3 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -1462,7 +1462,7 @@ static void coroutine_fn v9fs_open(void *opaque) > > goto out; > > } > > err += offset; > > - } else { > > + } else if (S_ISREG(stbuf.st_mode)) { > > if (s->proto_version == V9FS_PROTO_2000L) { > > TOCTTOU race. You are checking the stat() results and only then calling > open(), rather than calling open() first and validating fstat(). That > means the guest can STILL cause you to open() a pipe by changing the > file type in between the stat and the open. > You're right. And of course, this TOCTTOU race also affects the file versus directory choice: we can end up with a fid with type P9_FID_FILE whereas the underlying fd points to a directory... not sure how the rest of the code copes with that. :-\ Side note: support for older versions of the protocol greatly contribute to the overall obfuscation of the code... I wonder if we have non-V9FS_PROTO_2000L users, and how I could deprecate the damn thing... > I think you need to rework this patch to open() first, then validate > (closing the fd if necessary); the open can be done with O_NONBLOCK to > avoid hanging on a pipe. Yes, that's more annoying, but that's life > with TOCTTOU races. > Yeah... and even if linux doesn't implement O_NONBLOCK behaviour for files, for completeness we should revert this with fcntl(). The fix should hence be done at the backend level. Good news, at least: the directory branch will eventually call opendir()->open(O_DIRECTORY) and thus doesn't need fixing. Also the fixing of the regular file case will also take care of the P9_FID_FILE/directory discrepancy mentioned above. Thanks for the advice anyway. :) -- Greg
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index fa58877570f6..edd7b97270e3 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1462,7 +1462,7 @@ static void coroutine_fn v9fs_open(void *opaque) goto out; } err += offset; - } else { + } else if (S_ISREG(stbuf.st_mode)) { if (s->proto_version == V9FS_PROTO_2000L) { flags = get_dotl_openflags(s, mode); } else { @@ -1494,6 +1494,9 @@ static void coroutine_fn v9fs_open(void *opaque) goto out; } err += offset; + } else { + err = -EINVAL; + goto out; } trace_v9fs_open_return(pdu->tag, pdu->id, qid.type, qid.version, qid.path, iounit);
It really does not make sense for the 9P server to open anything else but a regular file or a directory. Malicious code in a guest could for example create a named pipe, associate it to a valid fid and pass it to the server in a RLOPEN message. This would cause QEMU to hang in open(), waiting for someone to open the other end of the pipe. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)