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