From 4799286cbbf12b5d2734ae79d0580d92b8d3b100 Mon Sep 17 00:00:00 2001

Message-Id: <4799286cbbf12b5d2734ae79d0580d92b8d3b100.1383321465.git.jdenemar@redhat.com>

From: "Daniel P. Berrange" <berrange@redhat.com>

Date: Wed, 30 Oct 2013 17:01:50 +0000

Subject: [PATCH] Make virCommand env handling robust in setuid env

For

  https://bugzilla.redhat.com/show_bug.cgi?id=1015247

When running setuid, we must be careful about what env vars

we allow commands to inherit from us. Replace the

virCommandAddEnvPass function with two new ones which do

filtering

  virCommandAddEnvPassAllowSUID

  virCommandAddEnvPassBlockSUID

And make virCommandAddEnvPassCommon use the appropriate

ones

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

(cherry picked from commit 9b8f307c6ad002a17a0510513883d06395636793)

Conflicts:

	src/qemu/qemu_command.c

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/libvirt_private.syms |  3 ++-

 src/lxc/lxc_process.c    |  2 +-

 src/qemu/qemu_command.c  |  6 +++---

 src/rpc/virnetsocket.c   | 16 ++++++++--------

 src/util/vircommand.c    | 50 +++++++++++++++++++++++++++++++++++++-----------

 src/util/vircommand.h    |  8 ++++++--

 tests/commandtest.c      |  8 ++++----

 7 files changed, 63 insertions(+), 30 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms

index 374a332..ca6275c 100644

--- a/src/libvirt_private.syms

+++ b/src/libvirt_private.syms

@@ -1237,7 +1237,8 @@ virCommandAddArgSet;

 virCommandAddEnvBuffer;

 virCommandAddEnvFormat;

 virCommandAddEnvPair;

-virCommandAddEnvPass;

+virCommandAddEnvPassAllowSUID;

+virCommandAddEnvPassBlockSUID;

 virCommandAddEnvPassCommon;

 virCommandAddEnvString;

 virCommandAllowCap;

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c

index a5f1f35..a88fb4d 100644

--- a/src/lxc/lxc_process.c

+++ b/src/lxc/lxc_process.c

@@ -740,7 +740,7 @@ virLXCProcessBuildControllerCmd(virLXCDriverPtr driver,

     cmd = virCommandNew(vm->def->emulator);

     /* The controller may call ip command, so we have to retain PATH. */

-    virCommandAddEnvPass(cmd, "PATH");

+    virCommandAddEnvPassBlockSUID(cmd, "PATH", "/bin:/usr/bin");

     virCommandAddEnvFormat(cmd, "LIBVIRT_DEBUG=%d",

                            virLogGetDefaultPriority());

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c

index 78f07c8..0292b72 100644

--- a/src/qemu/qemu_command.c

+++ b/src/qemu/qemu_command.c

@@ -6997,7 +6997,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,

      * security issues and might not work when using VNC.

      */

     if (cfg->vncAllowHostAudio)

-        virCommandAddEnvPass(cmd, "QEMU_AUDIO_DRV");

+        virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);

     else

         virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");

@@ -7242,8 +7242,8 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,

          * use QEMU's host audio drivers, possibly SDL too

          * User can set these two before starting libvirtd

          */

-        virCommandAddEnvPass(cmd, "QEMU_AUDIO_DRV");

-        virCommandAddEnvPass(cmd, "SDL_AUDIODRIVER");

+        virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);

+        virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL);

         /* New QEMU has this flag to let us explicitly ask for

          * SDL graphics. This is better than relying on the

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c

index ae81512..fcd41ca 100644

--- a/src/rpc/virnetsocket.c

+++ b/src/rpc/virnetsocket.c

@@ -127,9 +127,9 @@ static int virNetSocketForkDaemon(const char *binary)

                                              NULL);

     virCommandAddEnvPassCommon(cmd);

-    virCommandAddEnvPass(cmd, "XDG_CACHE_HOME");

-    virCommandAddEnvPass(cmd, "XDG_CONFIG_HOME");

-    virCommandAddEnvPass(cmd, "XDG_RUNTIME_DIR");

+    virCommandAddEnvPassBlockSUID(cmd, "XDG_CACHE_HOME", NULL);

+    virCommandAddEnvPassBlockSUID(cmd, "XDG_CONFIG_HOME", NULL);

+    virCommandAddEnvPassBlockSUID(cmd, "XDG_RUNTIME_DIR", NULL);

     virCommandClearCaps(cmd);

     virCommandDaemonize(cmd);

     ret = virCommandRun(cmd, NULL);

@@ -680,11 +680,11 @@ int virNetSocketNewConnectSSH(const char *nodename,

     cmd = virCommandNew(binary ? binary : "ssh");

     virCommandAddEnvPassCommon(cmd);

-    virCommandAddEnvPass(cmd, "KRB5CCNAME");

-    virCommandAddEnvPass(cmd, "SSH_AUTH_SOCK");

-    virCommandAddEnvPass(cmd, "SSH_ASKPASS");

-    virCommandAddEnvPass(cmd, "DISPLAY");

-    virCommandAddEnvPass(cmd, "XAUTHORITY");

+    virCommandAddEnvPassBlockSUID(cmd, "KRB5CCNAME", NULL);

+    virCommandAddEnvPassBlockSUID(cmd, "SSH_AUTH_SOCK", NULL);

+    virCommandAddEnvPassBlockSUID(cmd, "SSH_ASKPASS", NULL);

+    virCommandAddEnvPassBlockSUID(cmd, "DISPLAY", NULL);

+    virCommandAddEnvPassBlockSUID(cmd, "XAUTHORITY", NULL);

     virCommandClearCaps(cmd);

     if (service)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c

index 00ff69a..fca0e09 100644

--- a/src/util/vircommand.c

+++ b/src/util/vircommand.c

@@ -1247,21 +1247,49 @@ virCommandAddEnvBuffer(virCommandPtr cmd, virBufferPtr buf)

 /**

- * virCommandAddEnvPass:

+ * virCommandAddEnvPassAllowSUID:

  * @cmd: the command to modify

  * @name: the name to look up in current environment

  *

  * Pass an environment variable to the child

  * using current process' value

+ *

+ * Allow to be passed even if setuid

+ */

+void

+virCommandAddEnvPassAllowSUID(virCommandPtr cmd, const char *name)

+{

+    const char *value;

+    if (!cmd || cmd->has_error)

+        return;

+

+    value = virGetEnvAllowSUID(name);

+    if (value)

+        virCommandAddEnvPair(cmd, name, value);

+}

+

+

+/**

+ * virCommandAddEnvPassBlockSUID:

+ * @cmd: the command to modify

+ * @name: the name to look up in current environment

+ * @defvalue: value to return if running setuid, may be NULL

+ *

+ * Pass an environment variable to the child

+ * using current process' value.

+ *

+ * Do not pass if running setuid

  */

 void

-virCommandAddEnvPass(virCommandPtr cmd, const char *name)

+virCommandAddEnvPassBlockSUID(virCommandPtr cmd, const char *name, const char *defvalue)

 {

-    char *value;

+    const char *value;

     if (!cmd || cmd->has_error)

         return;

-    value = getenv(name);

+    value = virGetEnvBlockSUID(name);

+    if (!value)

+        value = defvalue;

     if (value)

         virCommandAddEnvPair(cmd, name, value);

 }

@@ -1286,13 +1314,13 @@ virCommandAddEnvPassCommon(virCommandPtr cmd)

     virCommandAddEnvPair(cmd, "LC_ALL", "C");

-    virCommandAddEnvPass(cmd, "LD_PRELOAD");

-    virCommandAddEnvPass(cmd, "LD_LIBRARY_PATH");

-    virCommandAddEnvPass(cmd, "PATH");

-    virCommandAddEnvPass(cmd, "HOME");

-    virCommandAddEnvPass(cmd, "USER");

-    virCommandAddEnvPass(cmd, "LOGNAME");

-    virCommandAddEnvPass(cmd, "TMPDIR");

+    virCommandAddEnvPassBlockSUID(cmd, "LD_PRELOAD", NULL);

+    virCommandAddEnvPassBlockSUID(cmd, "LD_LIBRARY_PATH", NULL);

+    virCommandAddEnvPassBlockSUID(cmd, "PATH", "/bin:/usr/bin");

+    virCommandAddEnvPassBlockSUID(cmd, "HOME", NULL);

+    virCommandAddEnvPassAllowSUID(cmd, "USER");

+    virCommandAddEnvPassAllowSUID(cmd, "LOGNAME");

+    virCommandAddEnvPassBlockSUID(cmd, "TMPDIR", NULL);

 }

 /**

diff --git a/src/util/vircommand.h b/src/util/vircommand.h

index c619e06..e977f93 100644

--- a/src/util/vircommand.h

+++ b/src/util/vircommand.h

@@ -99,8 +99,12 @@ void virCommandAddEnvString(virCommandPtr cmd,

 void virCommandAddEnvBuffer(virCommandPtr cmd,

                             virBufferPtr buf);

-void virCommandAddEnvPass(virCommandPtr cmd,

-                          const char *name) ATTRIBUTE_NONNULL(2);

+void virCommandAddEnvPassBlockSUID(virCommandPtr cmd,

+                                   const char *name,

+                                   const char *defvalue) ATTRIBUTE_NONNULL(2);

+

+void virCommandAddEnvPassAllowSUID(virCommandPtr cmd,

+                                   const char *name) ATTRIBUTE_NONNULL(2);

 void virCommandAddEnvPassCommon(virCommandPtr cmd);

diff --git a/tests/commandtest.c b/tests/commandtest.c

index eeb6d1e..1acc8d9 100644

--- a/tests/commandtest.c

+++ b/tests/commandtest.c

@@ -294,8 +294,8 @@ static int test6(const void *unused ATTRIBUTE_UNUSED)

 {

     virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");

-    virCommandAddEnvPass(cmd, "DISPLAY");

-    virCommandAddEnvPass(cmd, "DOESNOTEXIST");

+    virCommandAddEnvPassBlockSUID(cmd, "DISPLAY", NULL);

+    virCommandAddEnvPassBlockSUID(cmd, "DOESNOTEXIST", NULL);

     if (virCommandRun(cmd, NULL) < 0) {

         virErrorPtr err = virGetLastError();

@@ -319,8 +319,8 @@ static int test7(const void *unused ATTRIBUTE_UNUSED)

     virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");

     virCommandAddEnvPassCommon(cmd);

-    virCommandAddEnvPass(cmd, "DISPLAY");

-    virCommandAddEnvPass(cmd, "DOESNOTEXIST");

+    virCommandAddEnvPassBlockSUID(cmd, "DISPLAY", NULL);

+    virCommandAddEnvPassBlockSUID(cmd, "DOESNOTEXIST", NULL);

     if (virCommandRun(cmd, NULL) < 0) {

         virErrorPtr err = virGetLastError();

-- 

1.8.4.2