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: NDaniel P. Berrange <berrange@redhat.com>

src/libvirt_private.syms

@@ -1048,7 +1048,8 @@ virCommandAddArgSet;
 virCommandAddEnvBuffer;
 virCommandAddEnvFormat;
 virCommandAddEnvPair;
-virCommandAddEnvPass;
+virCommandAddEnvPassAllowSUID;
+virCommandAddEnvPassBlockSUID;
 virCommandAddEnvPassCommon;
 virCommandAddEnvString;
 virCommandAllowCap;

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());

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");
     }

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)

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);
 }
 
 /**

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);
 

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();