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

[Eric Blake <eblake@redhat.com>]

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

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.

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.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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