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

[Michal Privoznik]

On 18.12.2013 15:33, 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.
> 
> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

[Pasting the patch here]

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

While the part above looks okay, I think we should report error if the /dev/initctl is not a pipe. Moreover, with your approach, if it's really not a pipe, we don't call open() and proceed to safewrite(-1, ...) at line 153 which will report EBADF error.

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

Missing ';' at EOL ^^^ - I guess you missed that during copy-paste.

Looking forward to v2.
BTW: consider using git when sending patches. I couldn't really apply this patch via 'git am'.

http://libvirt.org/hacking.html#patches

Michal