Make virCommand env handling robust in setuid env

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)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 02b999b9513e689bd9220ad56bdc3a1ee350b8aa..91765f8682d9607d70072c4ca7d29fd0a4864c74 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1243,7 +1243,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 f92c613293c3584314df4477f2f057625c6271f4..e9b68d4e0743aca145763c978fb2b432e70d6810 100644 (file)
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -741,7 +741,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 f175a8ae3dd07a70a0fa99ce38be0a44f33f0f2f..e3c2d0cdafce6d527b1caf04cb2728b5c1946c4c 100644 (file)
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7126,7 +7126,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");
 
@@ -7371,8 +7371,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
@@ -7822,7 +7822,7 @@ qemuBuildCommandLine(virConnectPtr conn,
         virCommandAddArg(cmd, "-nographic");
 
         if (cfg->nogfxAllowHostAudio)
-            virCommandAddEnvPass(cmd, "QEMU_AUDIO_DRV");
+            virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
         else
             virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
     }

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index a2823efc3a0f9bb871f2276afef745fb5e9ad784..4a94331b6d2a8c75e48b8162cd51e9725c667050 100644 (file)
--- 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 7413269cb69171e65ea29894864655536b89758a..8dcf9e725255823818c79fdfeda136baec6b7881 100644 (file)
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1246,21 +1246,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 c619e0644facea8a213935dbed1a47c5c6957ec2..e977f93ed73b167e1eafc2315cc27a59479334ea 100644 (file)
--- 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 eeb6d1e131694eccc29c5b03ec7b641eb0f7de54..1acc8d9b0cfeb67db183f1deed2105abcd559285 100644 (file)
--- 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();