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)

Conflicts:
	src/qemu/qemu_command.c

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e5203246a789886da25b298aaff6f7d0ababc065..842e47101e07d93fc9058aa3d56c1cfac25e33ee 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1238,7 +1238,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 4835bd50036b74d2dedd3e6a97822aa2d70f483d..9b12a2194e148ad9633f6dab1f28e8597ceb261a 100644 (file)
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -733,7 +733,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 55f4c2575ebe1a4f69193ff297f13fc55a807e95..b185a1d44824fb7a14596a00b49bdd681d8a1232 100644 (file)
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6917,7 +6917,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");
 
@@ -7162,8 +7162,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 ae81512e71139a4628f3f1a9fda4cbaef5c92f2f..fcd41cacd82d4eb3523f3f4a9552c26ca07597c4 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 00ff69a54e7803ac4cff6237f5c612585e182566..fca0e098d6d8f25a339325bee86e9167bcd30fa7 100644 (file)
--- 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 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();