Libguestfs - Dec 2015 - Re: [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)

On Tue, Dec 01, 2015 at 03:59:56PM +0100, Mateusz Guzik
wrote:
> CHROOT_IN/OUT around commandvf are definitely problematic. chroot should be
> done in the child, which also removes the need to chroot out in the
> parent.
The CHROOT_IN/OUT business does need to be rewritten.  Every
instance where we currently do something like:

  CHROOT_IN;
  r = stat (fd, &statbuf);
  CHROOT_OUT

[https://github.com/libguestfs/libguestfs/blob/master/daemon/stat.c#L93-L95]

should instead be forking a subprocess, chrooting in the subprocess,
and doing the system call in the subprocess.

The problem which makes it not so easy is that instead of using a nice
local variable, we would have to pass back the result from a
subprocess to the parent process (the pair (r, statbuf) in the above
example).

So that means .. a pipe, and serializing the result down the pipe.

The good news is that since this all runs on the same machine in the
same compiled program, it's quite acceptable to dump a C struct into
the pipe.

But it's still a lot of work .. patches welcome of course.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Tue, Dec 01, 2015 at 04:58:11PM +0000, Richard W.M. Jones
wrote:
> On Tue, Dec 01, 2015 at 03:59:56PM +0100, Mateusz Guzik wrote:
> > CHROOT_IN/OUT around commandvf are definitely problematic. chroot
should be
> > done in the child, which also removes the need to chroot out in the
> > parent.
> 
> The CHROOT_IN/OUT business does need to be rewritten.  Every
> instance where we currently do something like:
> 
>   CHROOT_IN;
>   r = stat (fd, &statbuf);
>   CHROOT_OUT
> 
Ouch. Fortunately commandvf change should be easy.
>
[https://github.com/libguestfs/libguestfs/blob/master/daemon/stat.c#L93-L95]
> 
> should instead be forking a subprocess, chrooting in the subprocess,
> and doing the system call in the subprocess.
> 
> The problem which makes it not so easy is that instead of using a nice
> local variable, we would have to pass back the result from a
> subprocess to the parent process (the pair (r, statbuf) in the above
> example).
> 
> So that means .. a pipe, and serializing the result down the pipe.
> 
> The good news is that since this all runs on the same machine in the
> same compiled program, it's quite acceptable to dump a C struct into
> the pipe.
> 
> But it's still a lot of work .. patches welcome of course.
> 
This looks very problematic.

CHROOT_OUT is mere chroot ("."), which suggests that that cwd for
virt-builder is "/". This means anything using aforementioned
construct
has to use absolute paths, otherwise it looks names up against the real
"/". For current code it would make sense to somewhow check if all
passed paths are absolute (if not by code inspection, one can try to
cook up a systemtap script to verify such behaviour in-between chroots).

As for a solution, forking off a process which chroots is definitely on
the right track. However, I would argue what's really needed here is the
following: at the start, a container is created, the child gets in. Execs,
file system operations, just about everything has to be a request sent
to the child over e.g. a unix socket, but pipes could work too.

Filesystems in the container would not be fully populated (see: /dev/mem
& friends), but only have 'container-friendly' files.

That's definitely a lot of work and I'm not up to the task.

Regardless, commandvf vs chroot usage can be improved without said
significant work.

-- 
Mateusz Guzik

On Tue, Dec 01, 2015 at 06:29:01PM +0100, Mateusz Guzik
wrote:
> CHROOT_OUT is mere chroot ("."), which suggests that that cwd for
> virt-builder is "/". This means anything using aforementioned
construct
> has to use absolute paths, otherwise it looks names up against the real
> "/". For current code it would make sense to somewhow check if
all
> passed paths are absolute (if not by code inspection, one can try to
> cook up a systemtap script to verify such behaviour in-between chroots).
> 
> As for a solution, forking off a process which chroots is definitely on
> the right track. However, I would argue what's really needed here is
the
> following: at the start, a container is created, the child gets in. Execs,
> file system operations, just about everything has to be a request sent
> to the child over e.g. a unix socket, but pipes could work too.
I think you're hinting at a security issue, but I don't think there is
one in virt-builder, since firstly we trust the templates and the
command line (hence there is no "attacker" in the virt-builder
scenario), and secondly everything runs inside a virtual machine, so
this putative virt-builder attacker can only take over the appliance,
and would be stopped by the protections we place around the appliance
(sVirt and so on).

However it certainly is worth hardening the way we run commands.  A
container approach has another advantage too: that any processes
started up by (eg) dnf/yum are "captured" in the container and can be
conveniently killed off when the command exits.  This is in fact an
existing source of bugs (https://bugzilla.redhat.com/1195881).
> Filesystems in the container would not be fully populated (see: /dev/mem
> & friends), but only have 'container-friendly' files.
> 
> That's definitely a lot of work and I'm not up to the task.
Right - patches welcome!
> Regardless, commandvf vs chroot usage can be improved without said
> significant work.
Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v