CVE-2013-6456: Avoid unsafe use of /proc/$PID/root in LXC shutdown/reboot code

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=732394Signed-off-by: NEric Blake <eblake@redhat.com>

src/lxc/lxc_driver.c

@@ -3336,13 +3336,21 @@ 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;
 
@@ -3369,16 +3377,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",
@@ -3404,7 +3410,6 @@ lxcDomainShutdownFlags(virDomainPtr dom,
     ret = 0;
 
 cleanup:
-    VIR_FREE(vroot);
     if (vm)
         virObjectUnlock(vm);
     return ret;
@@ -3416,13 +3421,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;
 
@@ -3449,16 +3454,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",
@@ -3484,7 +3487,6 @@ lxcDomainReboot(virDomainPtr dom,
     ret = 0;
 
 cleanup:
-    VIR_FREE(vroot);
     if (vm)
         virObjectUnlock(vm);
     return ret;

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;
 }

src/util/virinitctl.h

 /*
  * virinitctl.h: API for talking to init systems via initctl
  *
- * Copyright (C) 2012 Red Hat, Inc.
+ * Copyright (C) 2012-2014 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