[Pkg-libvirt-maintainers] Bug#732394: [libvirt] Implementation deficiency in virInitctlSetRunLevel

[Daniel P. Berrange]

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 :|