[libvirt] Bug#732394: Implementation deficiency in virInitctlSetRunLevel
Daniel P. Berrange
berrange at redhat.com
Tue Jan 7 12:51:32 UTC 2014
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?bugs2394
>
> 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 > char *path > int ret > + struct stat attrs;
>
> memset(&req, 0, sizeof(req));
>
> @@ -139,7 +143,10 @@
> return -1;
> }
>
> - if ((fd > + if (lstat(path, &attrs) > + goto cleanup;
> +
> + if ((attrs.st_mode & S_IFIFO) && (fd > if (errno > ret > 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 :|
--
To UNSUBSCRIBE, email to debian-bugs-dist-REQUEST at lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster at lists.debian.org
More information about the libvir-list
mailing list