[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] fsdev: fix virtfs-proxy-helper cwd
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH] fsdev: fix virtfs-proxy-helper cwd |
Date: |
Sat, 20 May 2017 09:29:28 +0200 |
On Fri, 19 May 2017 17:19:10 -0500
Eric Blake <address@hidden> wrote:
> On 05/19/2017 09:30 AM, Greg Kurz wrote:
> > Since chroot() doesn't change the current directory, it is indeed a good
> > practice to chdir() to the target directory and then then chroot(), or
> > to chroot() to the target directory and then chdir("/").
> >
> > The current code does neither of them actually. Let's go for the latter.
> >
> > This doesn't fix any security issue since all of this takes place before
> > the helper begins to process requests.
> >
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> > fsdev/virtfs-proxy-helper.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
>
> You are correct that failing to sanitize the current working directory
> alongside a chroot() can lead to escaped access outside of the new
> smaller root.
>
The funny thing is that the author seemed to care about calling chdir("/")
at some point but it doesn't really make sense to do this before chroot().
Passing the special fd value AT_FDCWD and a relative path to an "*at()"
syscall would allow to access to the entire filesystem.
> Aside: chdir() is annoying in multi-threaded apps - it is global state,
> rather than thread-local. A multi-threaded app should therefore either
> never change the current working directory, or else never rely on the
> current working directory.
> But if I'm not mistaken, virtfs-proxy-helper
> is an independent helper app, not qemu proper, so the use of
> chdir/chroot is not affecting other threads.
>
Yes, this is correct, we're ok with this helper.
> Reviewed-by: Eric Blake <address@hidden>
>
pgpyGw8895Xr6.pgp
Description: OpenPGP digital signature