On Tue, 10 Jan 2017 08:38:27 -0600 Eric Blake <[email protected]> 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 <[email protected]> > > --- > > 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
pgptHd9EsQ5d2.pgp
Description: OpenPGP digital signature
