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

Mateusz Guzik mguzik at redhat.com
Tue Dec 1 16:50:54 UTC 2015


On Tue, Dec 01, 2015 at 05:05:50PM +0100, Mateusz Guzik wrote:
> On Tue, Dec 01, 2015 at 04:16:57PM +0100, Pino Toscano wrote:
> > On Tuesday 01 December 2015 15:59:56 Mateusz Guzik wrote:
> > > I would argue that /dev has to be at least partially populated for anything
> > > that gets executed in the chroot. At the very least special nodes like null,
> > > zero and {u,}random are needed.
> > 
> > We do not assume anything about guests, which could be Linux with a
> > static /dev (rare these days), but also non-Linux guests, including
> > Windows.
> > 
> 
> I have to confess to ignorance about this project, I only found it after
> seeing a suspicious commit in dnf.
> 
> However, your description does not look right. The code seems to assume
> it is possible to execve binaries in a chroot, which for practical
> purposes means linux binaries. Further, do_command sets up
> /etc/resolv.conf for the "guest".
> 
> My argument is that executing linux binaries in an environment without
> properly populated /dev is a recipe for trouble and will one day come
> back with weird failures or improperly constructed images.
> 

I realized said programs typically require at least /proc, which
prompted me to look around a little bit more.

do_command apart from setting up /etc/resolv.conf calls bind_mount,
which will bind several filesystems into the chroot. The list includes
/dev.

I cannot test it, but it would seem relevant directories in chroot are
populated just fine, which would suggest that /dev/null is there when
the chrooted child is trying to open it and it is closed after exec due
to the O_CLOEXEC flag.

If chroot's /dev is indeed empty, the bind_mount actions failed or were
undone in some way, which would be a bug.

As a side note bind_mount only notes the failure to bind something, as
opposed to bailing out. Seems like a dangerous behaviour - whatever is
executing in there may need this data and weirdly error much later if,
say, /sys was not provided.

Similar remark goes to:

ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC));

It's such a basic part of the setup that a failure here indicates
something is wrong. The code should return an error instead of
proceeding to execute the binary.

> > > 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.
> > 
> > That could be another way to make the command* functions in the daemon
> > safer, which wouldn't solve the issue of this email thread: even if you
> > fork and then chroot, the guest has nothing in /dev, so you cannot
> > open /dev/null at that point. This means you still need to carry on an
> > open /dev/null from the appliance.
> > 
> > > Assuming populated /dev is problematic/not feasible, at the very least
> > > the open(/dev/null) should be performed in the child just prior to
> > > chroot.
> > 
> > See above.
> > 
> 
> The issue would be solved in a less hackish manner. If the process is
> not chrooted, it can open /dev/null. So you fork, open /dev/null and
> only then proceed to chroot.
> 
> But as noted earlier, unpopulated /dev seems to be the real bug here.
> 
> > > Current patch seems to work around shortcomings of the current API.
> > > 
> > > Side content:
> > >       if (flag_copy_stdin) close (flag_copy_fd);
> > >       waitpid (pid, NULL, 0);
> > >       return -1;
> > 
> > This is done only in the main "while (quit < 2)" loop, and only on
> > error there, which will return with -1.
> > 
> > > but some lines below there is:
> > >   if (flag_copy_stdin && close (flag_copy_fd) == -1) {
> > >     perror ("close");
> > >     return -1;
> > >   }
> > > 
> > >   /* Get the exit status of the command. */
> > >   if (waitpid (pid, &r, 0) != pid) {
> > >     perror ("waitpid");
> > >     return -1;
> > >   }
> > > 
> > > 
> > > close() does not return an error unless extraterrestial circumstances occur,
> > > and even then the fd is no longer in use by the process. As such, I would argue
> > > checking for errors here is not necessary. Note that prior sample does not check.
> > 
> > The code for flag_copy_stdin & flag_copy_fd is unrelated to the bit
> > quoted above, see above for the explanation.
> > 
> 
> The point was that cleanup is duplicated and inconsistent.
> 

-- 
Mateusz Guzik




More information about the Libguestfs mailing list