[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCHv2 7/7] lxc: security fix for virInitctlSetRunLevel



Use helper virProcessRunInMountNamespace in lxcDomainShutdownFlags and
lxcDomainReboot.  Otherwise, a malicious guest could use symlinks
to force the host to manipulate the wrong file in the host's namespace.

Idea by Dan Berrange, based on an initial report by Reco
<recoverym4n gmail com> at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394

Signed-off-by: Eric Blake <eblake redhat com>
---
 src/lxc/lxc_driver.c  | 38 ++++++++++++++++++++------------------
 src/util/virinitctl.c | 28 +++++++++++-----------------
 src/util/virinitctl.h |  5 ++---
 3 files changed, 33 insertions(+), 38 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index e5298d1..30d65c8 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -2694,12 +2694,20 @@ lxcConnectListAllDomains(virConnectPtr conn,


 static int
+lxcDomainInitctlCallback(pid_t pid ATTRIBUTE_UNUSED,
+                         void *opaque)
+{
+    int *command = opaque;
+    return virInitctlSetRunLevel(*command);
+}
+
+
+static int
 lxcDomainShutdownFlags(virDomainPtr dom,
                        unsigned int flags)
 {
     virLXCDomainObjPrivatePtr priv;
     virDomainObjPtr vm;
-    char *vroot = NULL;
     int ret = -1;
     int rc;

@@ -2726,16 +2734,14 @@ lxcDomainShutdownFlags(virDomainPtr dom,
         goto cleanup;
     }

-    if (virAsprintf(&vroot, "/proc/%llu/root",
-                    (unsigned long long)priv->initpid) < 0)
-        goto cleanup;
-
     if (flags == 0 ||
         (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) {
-        if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF,
-                                        vroot)) < 0) {
+        int command = VIR_INITCTL_RUNLEVEL_POWEROFF;
+
+        if ((rc = virProcessRunInMountNamespace(priv->initpid,
+                                                lxcDomainInitctlCallback,
+                                                &command)) < 0)
             goto cleanup;
-        }
         if (rc == 0 && flags != 0 &&
             ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) {
             virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
@@ -2761,7 +2767,6 @@ lxcDomainShutdownFlags(virDomainPtr dom,
     ret = 0;

 cleanup:
-    VIR_FREE(vroot);
     if (vm)
         virObjectUnlock(vm);
     return ret;
@@ -2773,13 +2778,13 @@ lxcDomainShutdown(virDomainPtr dom)
     return lxcDomainShutdownFlags(dom, 0);
 }

+
 static int
 lxcDomainReboot(virDomainPtr dom,
                 unsigned int flags)
 {
     virLXCDomainObjPrivatePtr priv;
     virDomainObjPtr vm;
-    char *vroot = NULL;
     int ret = -1;
     int rc;

@@ -2806,16 +2811,14 @@ lxcDomainReboot(virDomainPtr dom,
         goto cleanup;
     }

-    if (virAsprintf(&vroot, "/proc/%llu/root",
-                    (unsigned long long)priv->initpid) < 0)
-        goto cleanup;
-
     if (flags == 0 ||
         (flags & VIR_DOMAIN_REBOOT_INITCTL)) {
-        if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT,
-                                        vroot)) < 0) {
+        int command = VIR_INITCTL_RUNLEVEL_REBOOT;
+
+        if ((rc = virProcessRunInMountNamespace(priv->initpid,
+                                                lxcDomainInitctlCallback,
+                                                &command)) < 0)
             goto cleanup;
-        }
         if (rc == 0 && flags != 0 &&
             ((flags & ~VIR_DOMAIN_SHUTDOWN_INITCTL) == 0)) {
             virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
@@ -2841,7 +2844,6 @@ lxcDomainReboot(virDomainPtr dom,
     ret = 0;

 cleanup:
-    VIR_FREE(vroot);
     if (vm)
         virObjectUnlock(vm);
     return ret;
diff --git a/src/util/virinitctl.c b/src/util/virinitctl.c
index 64bc23a..029dbb9 100644
--- a/src/util/virinitctl.c
+++ b/src/util/virinitctl.c
@@ -1,7 +1,7 @@
 /*
  * virinitctl.c: API for talking to init systems via initctl
  *
- * Copyright (C) 2012 Red Hat, Inc.
+ * Copyright (C) 2012-2013 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -111,16 +111,18 @@ struct virInitctlRequest {
 # endif

 /*
- * Send a message to init to change the runlevel
+ * Send a message to init to change the runlevel. This function is
+ * asynchronous-signal-safe (thus safe to use after fork of a
+ * multithreaded parent) - which is good, because it should only be
+ * used after forking and entering correct namespace.
  *
  * Returns 1 on success, 0 if initctl does not exist, -1 on error
  */
-int virInitctlSetRunLevel(virInitctlRunLevel level,
-                          const char *vroot)
+int
+virInitctlSetRunLevel(virInitctlRunLevel level)
 {
     struct virInitctlRequest req;
     int fd = -1;
-    char *path = NULL;
     int ret = -1;

     memset(&req, 0, sizeof(req));
@@ -131,36 +133,28 @@ int virInitctlSetRunLevel(virInitctlRunLevel level,
     /* Yes it is an 'int' field, but wants a numeric character. Go figure */
     req.runlevel = '0' + level;

-    if (vroot) {
-        if (virAsprintf(&path, "%s/%s", vroot, VIR_INITCTL_FIFO) < 0)
-            return -1;
-    } else {
-        if (VIR_STRDUP(path, VIR_INITCTL_FIFO) < 0)
-            return -1;
-    }
-
-    if ((fd = open(path, O_WRONLY|O_NONBLOCK|O_CLOEXEC|O_NOCTTY)) < 0) {
+    if ((fd = open(VIR_INITCTL_FIFO,
+                   O_WRONLY|O_NONBLOCK|O_CLOEXEC|O_NOCTTY)) < 0) {
         if (errno == ENOENT) {
             ret = 0;
             goto cleanup;
         }
         virReportSystemError(errno,
                              _("Cannot open init control %s"),
-                             path);
+                             VIR_INITCTL_FIFO);
         goto cleanup;
     }

     if (safewrite(fd, &req, sizeof(req)) != sizeof(req)) {
         virReportSystemError(errno,
                              _("Failed to send request to init control %s"),
-                             path);
+                             VIR_INITCTL_FIFO);
         goto cleanup;
     }

     ret = 1;

 cleanup:
-    VIR_FREE(path);
     VIR_FORCE_CLOSE(fd);
     return ret;
 }
diff --git a/src/util/virinitctl.h b/src/util/virinitctl.h
index 862539f..18052c0 100644
--- a/src/util/virinitctl.h
+++ b/src/util/virinitctl.h
@@ -1,7 +1,7 @@
 /*
  * virinitctl.h: API for talking to init systems via initctl
  *
- * Copyright (C) 2012 Red Hat, Inc.
+ * Copyright (C) 2012-2013 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -37,7 +37,6 @@ enum virInitctlRunLevel {
     VIR_INITCTL_RUNLEVEL_LAST
 };

-int virInitctlSetRunLevel(virInitctlRunLevel level,
-                          const char *vroot);
+int virInitctlSetRunLevel(virInitctlRunLevel level);

 #endif
-- 
1.8.4.2


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]