Re: [Qemu-devel] [PATCH] fsdev: fix virtfs-proxy-helper cwd

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

On Fri, 19 May 2017 17:19:10 -0500
Eric Blake <eblake@redhat.com> 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 <groug@kaod.org>
> > ---
> >  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 <eblake@redhat.com>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]