[libvirt] [PATCH 09/14] Avoid unsafe use of /proc/$PID/root in LXC shutdown/reboot code

Daniel P. Berrange berrange at redhat.com
Fri Feb 7 15:33:07 UTC 2014


From: Eric Blake <eblake at redhat.com>

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 at gmail.com> at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394

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

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 51ca98c..96987cd 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3307,12 +3307,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;
 
@@ -3339,16 +3347,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",
@@ -3374,7 +3380,6 @@ lxcDomainShutdownFlags(virDomainPtr dom,
     ret = 0;
 
 cleanup:
-    VIR_FREE(vroot);
     if (vm)
         virObjectUnlock(vm);
     return ret;
@@ -3386,13 +3391,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;
 
@@ -3419,16 +3424,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",
@@ -3454,7 +3457,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 a85cc9c..579268f 100644
--- a/src/util/virinitctl.c
+++ b/src/util/virinitctl.c
@@ -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.5.3




More information about the libvir-list mailing list