8:33 a.m.
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.
11:15 a.m.
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(a)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
1:47 p.m.
Hi.
On Wed, 18 Dec 2013 18:15:17 +0100
Michal Privoznik <mprivozn(a)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
2:42 p.m.
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(a)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
3:06 p.m.
Hi.
On Wed, 18 Dec 2013 13:42:25 -0700
Eric Blake <eblake(a)redhat.com> wrote:
git send-email --in-reply-to='messageid(a)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
4:03 p.m.
On 12/18/2013 02:06 PM, Reco wrote:
Hi.
On Wed, 18 Dec 2013 13:42:25 -0700
Eric Blake <eblake(a)redhat.com> wrote:
> git send-email --in-reply-to='messageid(a)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
11:25 a.m.
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 :|
4046
days inactive
4046
days old
6 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik -
Reco