Message ID | 149520423435.7495.2756710723268429379.stgit@bahia.lan |
---|---|
State | New |
Headers | show |
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>
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> >
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
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(-)