From 30d4034b0903d7828dddd874a035cdc002ecc5be Mon Sep 17 00:00:00 2001

Message-Id: <30d4034b0903d7828dddd874a035cdc002ecc5be@dist-git>

From: Eric Blake <eblake@redhat.com>

Date: Tue, 18 Feb 2014 15:45:39 -0700

Subject: [PATCH] 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=732394

Signed-off-by: Eric Blake <eblake@redhat.com>

(cherry picked from commit aebbcdd33c8c18891f0bdbbf8924599a28152c9c)

Signed-off-by: Jiri Denemark <jdenemar@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 33169c8..cf45d8d 100644

--- a/src/lxc/lxc_driver.c

+++ b/src/lxc/lxc_driver.c

@@ -2742,12 +2742,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;

@@ -2774,16 +2782,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",

@@ -2809,7 +2815,6 @@ lxcDomainShutdownFlags(virDomainPtr dom,

     ret = 0;

 cleanup:

-    VIR_FREE(vroot);

     if (vm)

         virObjectUnlock(vm);

     return ret;

@@ -2821,13 +2826,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;

@@ -2854,16 +2859,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",

@@ -2889,7 +2892,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..4deeb19 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..6aeb1a6 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-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

-- 

1.9.0