On Fri, 19 May 2017 17:19:10 -0500 Eric Blake <ebl...@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 <gr...@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 <ebl...@redhat.com>
>
pgpeEKkAwnbxN.pgp
Description: OpenPGP digital signature
