[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.
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@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
Hi. On Wed, 18 Dec 2013 18:15:17 +0100 Michal Privoznik <mprivozn@redhat.com> wrote:
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.
I've adjusted the patch to your suggestion.
Missing ';' at EOL ^^^ - I guess you missed that during copy-paste.
OOPS. My mistake.
Looking forward to v2.
Attached. I've tried following Daniel suggestions too.
BTW: consider using git when sending patches. I couldn't really apply this patch via 'git am'.
I cannot figure out how to force 'git send-email' to reply on a mail. It seems to insist to send a new one. Reco
On 12/18/2013 12:47 PM, Reco wrote:
BTW: consider using git when sending patches. I couldn't really apply this patch via 'git am'.
I cannot figure out how to force 'git send-email' to reply on a mail.
git send-email --in-reply-to='messageid@example.com' for the correct value of messageid. But generally posting a new thread is better than threading your v2 to your v1 anyways, as top-level threads are easier to spot than a patch nested deep in reply to another message. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hi. On Wed, 18 Dec 2013 13:42:25 -0700 Eric Blake <eblake@redhat.com> wrote:
git send-email --in-reply-to='messageid@example.com'
for the correct value of messageid. But generally posting a new thread is better than threading your v2 to your v1 anyways, as top-level threads are easier to spot than a patch nested deep in reply to another message.
Thanks. Should I resend the patch the proper way again? Reco
On 12/18/2013 02:06 PM, Reco wrote:
Hi.
On Wed, 18 Dec 2013 13:42:25 -0700 Eric Blake <eblake@redhat.com> wrote:
git send-email --in-reply-to='messageid@example.com'
for the correct value of messageid. But generally posting a new thread is better than threading your v2 to your v1 anyways, as top-level threads are easier to spot than a patch nested deep in reply to another message.
Thanks. Should I resend the patch the proper way again?
Reco
We also prefer that patches have authorship attributed to a legal name, rather than a nickname. But rather than resend your patch, I think we should pay attention to Dan's advice - that any solution we come up with that doesn't involve forking a child process, then resetting that child into the correct namespace, is prone to races and therefore does not solve the problem at hand (namely, that symlinks created in the guest namespace must NOT be followed in the parent in host namespace). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik -
Reco