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>

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9be6f327e705c6123ca2ed3c21338ba0186146ca..f1f817c906b9d39c57565d4dd69aa38d5a61dc73 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1048,7 +1048,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 f088e8e51b70c151dfb38bf9137545b8edebdaeb..c51c4d5368a0eeb3cbd436b88b68ca94df9a8143 100644 (file)
--- 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 814f368e06b22bf15f54d0012f82bdd6707dc4f4..63e235dd52c8f1dd35e9ce4699ff3011f3cd5f43 100644 (file)
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7141,7 +7141,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");
 
@@ -7396,8 +7396,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
@@ -7847,7 +7847,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 2e50f8ccafaa1322d532d2ac667a6af657f801a9..b2ebefec3b7d294fe0148c56976bb8188b621e08 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 4780cf985fcf517189bf22eabe0423306a8d998e..08e9e21fc6e5cc291f11be232de2cc006d109f09 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();