10:50 a.m.
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