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

Greg Kurz posted 1 patch 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/149520423435.7495.2756710723268429379.stgit@bahia.lan
Test checkpatch passed
Test docker passed
Test s390x passed
fsdev/virtfs-proxy-helper.c |    8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
[Qemu-devel] [PATCH] fsdev: fix virtfs-proxy-helper cwd
Posted by Greg Kurz 6 years, 10 months ago
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
Posted by Eric Blake 6 years, 10 months ago
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

Re: [Qemu-devel] [PATCH] fsdev: fix virtfs-proxy-helper cwd
Posted by Greg Kurz 6 years, 10 months ago
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>
>