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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
