From 6f5f52c43ffb81adee9400d883692edc7ffab32a Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 9 Oct 2013 11:03:02 +0100 Subject: [PATCH] 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 (cherry picked from commit 9b8f307c6ad002a17a0510513883d06395636793) Conflicts: src/qemu/qemu_command.c --- 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 e5203246a7..842e47101e 100644 --- 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 4835bd5003..9b12a2194e 100644 --- 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 55f4c2575e..b185a1d448 100644 --- 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 ae81512e71..fcd41cacd8 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 00ff69a54e..fca0e098d6 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 c619e0644f..e977f93ed7 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 eeb6d1e131..1acc8d9b0c 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(); -- GitLab