Re: [libvirt] Implementation deficiency in virInitctlSetRunLevel

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