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

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

[Greg Kurz]

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(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 54f7ad1c48f0..4c4238f62e53 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -1129,14 +1129,14 @@ int main(int argc, char **argv)
         }
     }
 
-    if (chdir("/") < 0) {
-        do_perror("chdir");
-        goto error;
-    }
     if (chroot(rpath) < 0) {
         do_perror("chroot");
         goto error;
     }
+    if (chdir("/") < 0) {
+        do_perror("chdir");
+        goto error;
+    }
 
     get_version = false;
 #ifdef FS_IOC_GETVERSION

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

[Eric Blake]

[-- 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 --]

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

[Greg Kurz]

[-- 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 --]