On Wed, Dec 18, 2013 at 06:33:21PM +0400, Reco wrote: > Hello, list. > > I was pointed here by maintainer of libvirt package in Debian, Guido > Günther. For the sake of completeness, the original bug report can be > viewed at this link: > > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394 > > To sum up the bug report, current implementation of > virInitctlSetRunLevel function (src/util/virinitctl.c) lacks any sanity > checks before writing to container's /dev/initctl. In the absence of > such checks, libvirtd can be easily tricked to write runlevel check > request to an arbitrary main hosts' file (including > hosts' /run/initctl, as described in the bug report). All it takes is > one symlink in place of containers' /dev/initctl. > > I've checked current libvirtd's git, and it seems to me that the > problem is still here. > > Attached to this letter is a patch which tries to mitigate the issue by > checking whenever container's /dev/initctl is a pipe actually. > > Sincerely yours, Reco > > PS I'm not subscribed to this list, in case of further questions please > CC me. > --- a/src/util/virinitctl.c 2013-12-18 11:13:10.078432196 +0400 > +++ b/src/util/virinitctl.c 2013-12-18 11:26:50.000000000 +0400 > @@ -24,7 +24,10 @@ > #include <config.h> > > #include <sys/param.h> > +#include <sys/types.h> > +#include <sys/stat.h> > #include <fcntl.h> > +#include <unistd.h> > > #include "internal.h" > #include "virinitctl.h" > @@ -122,6 +125,7 @@ > int fd = -1; > char *path = NULL; > int ret = -1; > + struct stat attrs; > > memset(&req, 0, sizeof(req)); > > @@ -139,7 +143,10 @@ > return -1; > } > > - if ((fd = open(path, O_WRONLY|O_NONBLOCK|O_CLOEXEC|O_NOCTTY)) < 0) { > + if (lstat(path, &attrs) == -1) > + goto cleanup; > + > + if ((attrs.st_mode & S_IFIFO) && (fd = open(path, O_WRONLY|O_NONBLOCK|O_CLOEXEC|O_NOCTTY)) < 0) { > if (errno == ENOENT) { > ret = 0; > goto cleanup Hmm, using lstat sets up a race condition though. I would suggest we use O_NOFOLLOW with open() but that only works for the final component of the path - so if /dev is a symlink in the guest it'll still cause problems. There are also a few other places where we use /proc/$PID/root/dev for hotplug where we mknod. If the guest setup a bad /dev symlink it could cause us problems. I think we may actually have to instead rely on forking a child which does setns(/proc/$PID/ns/mnt) to make the changes safely in the container namespace. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list