From 9b8f307c6ad002a17a0510513883d06395636793 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 --- src/libvirt_private.syms | 3 ++- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_command.c | 8 +++---- src/rpc/virnetsocket.c | 16 ++++++------- src/util/vircommand.c | 50 +++++++++++++++++++++++++++++++--------- src/util/vircommand.h | 8 +++++-- tests/commandtest.c | 8 +++---- 7 files changed, 64 insertions(+), 31 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9be6f327e7..f1f817c906 100644 --- 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 f088e8e51b..c51c4d5368 100644 --- 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 814f368e06..63e235dd52 100644 --- 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 2e50f8ccaf..b2ebefec3b 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 7413269cb6..8dcf9e7252 100644 --- 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 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 4780cf985f..08e9e21fc6 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