[libvirt] [PATCH 08/17] util: get rid of virGetEnv{Allow, Block}SUID functions
Daniel P. Berrangé
berrange at redhat.com
Thu Aug 1 15:00:10 UTC 2019
Now that 100% of libvirt code is forbidden in a SUID environment,
we no longer need to worry about whether env variables are
trustworthy or not. The virt-login-shell setuid program, which
does not link to any libvirt code, will purge all environment
variables, except $TERM, before invoking the virt-login-shell-helper
program which uses libvirt.
Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
---
cfg.mk | 9 ---------
src/libvirt-admin.c | 2 +-
src/libvirt.c | 2 +-
src/libvirt_private.syms | 2 --
src/network/leaseshelper.c | 14 ++++++-------
src/qemu/qemu_firmware.c | 2 +-
src/remote/remote_driver.c | 2 +-
src/rpc/virnetlibsshsession.c | 2 +-
src/rpc/virnettlscontext.c | 2 +-
src/util/virauth.c | 2 +-
src/util/vircommand.c | 2 +-
src/util/virfile.c | 4 ++--
src/util/virlease.c | 4 ++--
src/util/virlog.c | 6 +++---
src/util/virsystemd.c | 8 ++++----
src/util/virutil.c | 36 ++++-----------------------------
src/util/virutil.h | 3 ---
src/vbox/vbox_XPCOMCGlue.c | 2 +-
src/vbox/vbox_common.c | 2 +-
tools/virsh.c | 2 +-
tools/virt-login-shell-helper.c | 4 ++--
tools/vsh.c | 12 +++++------
22 files changed, 41 insertions(+), 83 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 9130b4560b..ec09550b49 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -855,12 +855,6 @@ sc_prohibit_unbounded_arrays_in_rpc:
halt='Arrays in XDR must have a upper limit set for <NNN>' \
$(_sc_search_regexp)
-sc_prohibit_getenv:
- @prohibit='\b(secure_)?getenv *\(' \
- exclude='exempt from syntax-check' \
- halt='Use virGetEnv{Allow,Block}SUID instead of getenv' \
- $(_sc_search_regexp)
-
sc_prohibit_atoi:
@prohibit='\bato(i|f|l|ll|q) *\(' \
halt='Use virStrToLong* instead of atoi, atol, atof, atoq, atoll' \
@@ -1316,9 +1310,6 @@ exclude_file_name_regexp--sc_prohibit_int_ijk = \
exclude_file_name_regexp--sc_prohibit_unsigned_pid = \
^(include/libvirt/.*\.h|src/(qemu/qemu_driver\.c|driver-hypervisor\.h|libvirt(-[a-z]*)?\.c|.*\.x|util/vir(polkit|systemd)\.c)|tests/virpolkittest\.c|tools/virsh-domain\.c)$$
-exclude_file_name_regexp--sc_prohibit_getenv = \
- ^tests/.*\.[ch]|tools/virt-login-shell\.c$$
-
exclude_file_name_regexp--sc_avoid_attribute_unused_in_header = \
^(src/util/virlog\.h|src/network/bridge_driver\.h)$$
diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
index 74dedf64d8..4ae50b188f 100644
--- a/src/libvirt-admin.c
+++ b/src/libvirt-admin.c
@@ -169,7 +169,7 @@ getSocketPath(virURIPtr uri)
static int
virAdmGetDefaultURI(virConfPtr conf, char **uristr)
{
- const char *defname = virGetEnvAllowSUID("LIBVIRT_ADMIN_DEFAULT_URI");
+ const char *defname = getenv("LIBVIRT_ADMIN_DEFAULT_URI");
if (defname && *defname) {
if (VIR_STRDUP(*uristr, defname) < 0)
return -1;
diff --git a/src/libvirt.c b/src/libvirt.c
index 161001bf48..768ad348c7 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -774,7 +774,7 @@ virConnectGetDefaultURI(virConfPtr conf,
char **name)
{
int ret = -1;
- const char *defname = virGetEnvBlockSUID("LIBVIRT_DEFAULT_URI");
+ const char *defname = getenv("LIBVIRT_DEFAULT_URI");
if (defname && *defname) {
VIR_DEBUG("Using LIBVIRT_DEFAULT_URI '%s'", defname);
if (VIR_STRDUP(*name, defname) < 0)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 983fe93d99..12c506a87a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3270,8 +3270,6 @@ virFormatIntDecimal;
virFormatIntPretty;
virGetDeviceID;
virGetDeviceUnprivSGIO;
-virGetEnvAllowSUID;
-virGetEnvBlockSUID;
virGetGroupID;
virGetGroupList;
virGetGroupName;
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
index 2a10fbf33a..481f29aa59 100644
--- a/src/network/leaseshelper.c
+++ b/src/network/leaseshelper.c
@@ -86,10 +86,10 @@ main(int argc, char **argv)
const char *ip = NULL;
const char *mac = NULL;
const char *leases_str = NULL;
- const char *iaid = virGetEnvAllowSUID("DNSMASQ_IAID");
- const char *clientid = virGetEnvAllowSUID("DNSMASQ_CLIENT_ID");
- const char *interface = virGetEnvAllowSUID("DNSMASQ_INTERFACE");
- const char *hostname = virGetEnvAllowSUID("DNSMASQ_SUPPLIED_HOSTNAME");
+ const char *iaid = getenv("DNSMASQ_IAID");
+ const char *clientid = getenv("DNSMASQ_CLIENT_ID");
+ const char *interface = getenv("DNSMASQ_INTERFACE");
+ const char *hostname = getenv("DNSMASQ_SUPPLIED_HOSTNAME");
char *server_duid = NULL;
int action = -1;
int pid_file_fd = -1;
@@ -131,7 +131,7 @@ main(int argc, char **argv)
* events for expired leases. So, libvirtd sets another env var for this
* purpose */
if (!interface &&
- !(interface = virGetEnvAllowSUID("VIR_BRIDGE_NAME")))
+ !(interface = getenv("VIR_BRIDGE_NAME")))
goto cleanup;
ip = argv[3];
@@ -148,11 +148,11 @@ main(int argc, char **argv)
/* Check if it is an IPv6 lease */
if (iaid) {
- mac = virGetEnvAllowSUID("DNSMASQ_MAC");
+ mac = getenv("DNSMASQ_MAC");
clientid = argv[2];
}
- if (VIR_STRDUP(server_duid, virGetEnvAllowSUID("DNSMASQ_SERVER_DUID")) < 0)
+ if (VIR_STRDUP(server_duid, getenv("DNSMASQ_SERVER_DUID")) < 0)
goto cleanup;
if (virAsprintf(&custom_lease_file,
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
index bf29b10b9a..983a7c83b2 100644
--- a/src/qemu/qemu_firmware.c
+++ b/src/qemu/qemu_firmware.c
@@ -986,7 +986,7 @@ qemuFirmwareFetchConfigs(char ***firmwares,
* much sense to parse files in root's home directory. It
* makes sense only for session daemon which runs under
* regular user. */
- if (VIR_STRDUP(xdgConfig, virGetEnvBlockSUID("XDG_CONFIG_HOME")) < 0)
+ if (VIR_STRDUP(xdgConfig, getenv("XDG_CONFIG_HOME")) < 0)
return -1;
if (!xdgConfig) {
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 5e6007d468..76ea49ed8e 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1297,7 +1297,7 @@ remoteConnectOpen(virConnectPtr conn,
struct private_data *priv;
int ret = VIR_DRV_OPEN_ERROR;
int rflags = 0;
- const char *autostart = virGetEnvBlockSUID("LIBVIRT_AUTOSTART");
+ const char *autostart = getenv("LIBVIRT_AUTOSTART");
char *driver = NULL;
char *transport = NULL;
diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
index 093ac29071..62cbe1e06a 100644
--- a/src/rpc/virnetlibsshsession.c
+++ b/src/rpc/virnetlibsshsession.c
@@ -172,7 +172,7 @@ virNetLibsshSessionOnceInit(void)
ssh_set_log_level(TRACE_LIBSSH);
#endif
- dbgLevelStr = virGetEnvAllowSUID("LIBVIRT_LIBSSH_DEBUG");
+ dbgLevelStr = getenv("LIBVIRT_LIBSSH_DEBUG");
if (dbgLevelStr &&
virStrToLong_i(dbgLevelStr, NULL, 10, &dbgLevel) >= 0)
ssh_set_log_level(dbgLevel);
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 4adc409c0b..416c8308ed 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -1439,7 +1439,7 @@ void virNetTLSSessionDispose(void *obj)
void virNetTLSInit(void)
{
const char *gnutlsdebug;
- if ((gnutlsdebug = virGetEnvAllowSUID("LIBVIRT_GNUTLS_DEBUG")) != NULL) {
+ if ((gnutlsdebug = getenv("LIBVIRT_GNUTLS_DEBUG")) != NULL) {
int val;
if (virStrToLong_i(gnutlsdebug, NULL, 10, &val) < 0)
val = 10;
diff --git a/src/util/virauth.c b/src/util/virauth.c
index e5994cbb7c..9de3996e92 100644
--- a/src/util/virauth.c
+++ b/src/util/virauth.c
@@ -42,7 +42,7 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
char **path)
{
size_t i;
- const char *authenv = virGetEnvBlockSUID("LIBVIRT_AUTH_FILE");
+ const char *authenv = getenv("LIBVIRT_AUTH_FILE");
VIR_AUTOFREE(char *) userdir = NULL;
*path = NULL;
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index ea9a9fd622..79e1e87964 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1424,7 +1424,7 @@ virCommandAddEnvPass(virCommandPtr cmd, const char *name)
if (!cmd || cmd->has_error)
return;
- value = virGetEnvAllowSUID(name);
+ value = getenv(name);
if (value)
virCommandAddEnvPair(cmd, name, value);
}
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 775192ff00..7667c16d27 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1674,7 +1674,7 @@ virFindFileInPath(const char *file)
}
/* copy PATH env so we can tweak it */
- origpath = virGetEnvBlockSUID("PATH");
+ origpath = getenv("PATH");
if (!origpath)
origpath = "/bin:/usr/bin";
@@ -1735,7 +1735,7 @@ virFileFindResourceFull(const char *filename,
const char *envname)
{
char *ret = NULL;
- const char *envval = envname ? virGetEnvBlockSUID(envname) : NULL;
+ const char *envval = envname ? getenv(envname) : NULL;
const char *path;
if (!prefix)
diff --git a/src/util/virlease.c b/src/util/virlease.c
index 93ca72e3af..87e9a3ce88 100644
--- a/src/util/virlease.c
+++ b/src/util/virlease.c
@@ -213,13 +213,13 @@ virLeaseNew(virJSONValuePtr *lease_ret,
const char *server_duid)
{
VIR_AUTOPTR(virJSONValue) lease_new = NULL;
- const char *exptime_tmp = virGetEnvAllowSUID("DNSMASQ_LEASE_EXPIRES");
+ const char *exptime_tmp = getenv("DNSMASQ_LEASE_EXPIRES");
long long expirytime = 0;
VIR_AUTOFREE(char *) exptime = NULL;
/* In case hostname is still unknown, use the last known one */
if (!hostname)
- hostname = virGetEnvAllowSUID("DNSMASQ_OLD_HOSTNAME");
+ hostname = getenv("DNSMASQ_OLD_HOSTNAME");
if (!mac)
return 0;
diff --git a/src/util/virlog.c b/src/util/virlog.c
index 6a2229ae2b..4c76fbc5a4 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -1308,13 +1308,13 @@ virLogSetFromEnv(void)
if (virLogInitialize() < 0)
return;
- debugEnv = virGetEnvAllowSUID("LIBVIRT_DEBUG");
+ debugEnv = getenv("LIBVIRT_DEBUG");
if (debugEnv && *debugEnv)
virLogSetDefaultPriority(virLogParseDefaultPriority(debugEnv));
- debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_FILTERS");
+ debugEnv = getenv("LIBVIRT_LOG_FILTERS");
if (debugEnv && *debugEnv)
virLogSetFilters(debugEnv);
- debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_OUTPUTS");
+ debugEnv = getenv("LIBVIRT_LOG_OUTPUTS");
if (debugEnv && *debugEnv)
virLogSetOutputs(debugEnv);
}
diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
index 1cb8874403..26b751311f 100644
--- a/src/util/virsystemd.c
+++ b/src/util/virsystemd.c
@@ -509,7 +509,7 @@ virSystemdNotifyStartup(void)
.msg_iovlen = 1,
};
- if (!(path = virGetEnvBlockSUID("NOTIFY_SOCKET"))) {
+ if (!(path = getenv("NOTIFY_SOCKET"))) {
VIR_DEBUG("Skipping systemd notify, not requested");
return;
}
@@ -798,7 +798,7 @@ virSystemdGetListenFDs(void)
VIR_DEBUG("Setting up networking from caller");
- if (!(pidstr = virGetEnvAllowSUID("LISTEN_PID"))) {
+ if (!(pidstr = getenv("LISTEN_PID"))) {
VIR_DEBUG("No LISTEN_PID from caller");
return 0;
}
@@ -814,7 +814,7 @@ virSystemdGetListenFDs(void)
return 0;
}
- if (!(fdstr = virGetEnvAllowSUID("LISTEN_FDS"))) {
+ if (!(fdstr = getenv("LISTEN_FDS"))) {
VIR_DEBUG("No LISTEN_FDS from caller");
return 0;
}
@@ -866,7 +866,7 @@ virSystemdActivationNew(virSystemdActivationMap *map,
if (!(act->fds = virHashCreate(10, virSystemdActivationEntryFree)))
goto error;
- fdnames = virGetEnvAllowSUID("LISTEN_FDNAMES");
+ fdnames = getenv("LISTEN_FDNAMES");
if (fdnames) {
if (virSystemdActivationInitFromNames(act, nfds, fdnames) < 0)
goto error;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 4e0dbe15c4..89d2cf011f 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -739,7 +739,7 @@ char *virGetUserShell(uid_t uid)
static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir)
{
- const char *path = virGetEnvBlockSUID(xdgenvname);
+ const char *path = getenv(xdgenvname);
char *ret = NULL;
char *home = NULL;
@@ -767,7 +767,7 @@ char *virGetUserCacheDirectory(void)
char *virGetUserRuntimeDirectory(void)
{
- const char *path = virGetEnvBlockSUID("XDG_RUNTIME_DIR");
+ const char *path = getenv("XDG_RUNTIME_DIR");
if (!path || !path[0]) {
return virGetUserCacheDirectory();
@@ -1137,7 +1137,7 @@ virGetUserDirectoryByUID(uid_t uid ATTRIBUTE_UNUSED)
const char *dir;
char *ret;
- dir = virGetEnvBlockSUID("HOME");
+ dir = getenv("HOME");
/* Only believe HOME if it is an absolute path and exists */
if (dir) {
@@ -1157,7 +1157,7 @@ virGetUserDirectoryByUID(uid_t uid ATTRIBUTE_UNUSED)
if (!dir)
/* USERPROFILE is probably the closest equivalent to $HOME? */
- dir = virGetEnvBlockSUID("USERPROFILE");
+ dir = getenv("USERPROFILE");
if (VIR_STRDUP(ret, dir) < 0)
return NULL;
@@ -1722,34 +1722,6 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
return rc;
}
-
-/**
- * virGetEnvBlockSUID:
- * @name: the environment variable name
- *
- * Obtain an environment variable which is unsafe to
- * use when running setuid. If running setuid, a NULL
- * value will be returned
- */
-const char *virGetEnvBlockSUID(const char *name)
-{
- return secure_getenv(name); /* exempt from syntax-check */
-}
-
-
-/**
- * virGetEnvAllowSUID:
- * @name: the environment variable name
- *
- * Obtain an environment variable which is safe to
- * use when running setuid. The value will be returned
- * even when running setuid
- */
-const char *virGetEnvAllowSUID(const char *name)
-{
- return getenv(name); /* exempt from syntax-check */
-}
-
static time_t selfLastChanged;
time_t virGetSelfLastChanged(void)
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 52d0c33773..b64a85f49e 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -141,9 +141,6 @@ char *virGetUnprivSGIOSysfsPath(const char *path,
int virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr);
-const char *virGetEnvBlockSUID(const char *name);
-const char *virGetEnvAllowSUID(const char *name);
-
time_t virGetSelfLastChanged(void);
void virUpdateSelfLastChanged(const char *path);
diff --git a/src/vbox/vbox_XPCOMCGlue.c b/src/vbox/vbox_XPCOMCGlue.c
index 2a054f02d6..72ae49b6b2 100644
--- a/src/vbox/vbox_XPCOMCGlue.c
+++ b/src/vbox/vbox_XPCOMCGlue.c
@@ -190,7 +190,7 @@ VBoxCGlueInit(unsigned int *version)
"/usr/local/lib/VirtualBox",
"/Applications/VirtualBox.app/Contents/MacOS"
};
- const char *home = virGetEnvBlockSUID("VBOX_APP_HOME");
+ const char *home = getenv("VBOX_APP_HOME");
/* If the user specifies the location, try only that. */
if (home != NULL) {
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 8a912da50c..6a4ef01e64 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3558,7 +3558,7 @@ vboxDumpDisplay(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
graphics->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP;
if (VIR_STRDUP(graphics->data.desktop.display,
- virGetEnvBlockSUID("DISPLAY")) < 0)
+ getenv("DISPLAY")) < 0)
goto cleanup;
}
diff --git a/tools/virsh.c b/tools/virsh.c
index f09ce1ec98..692a1ff16d 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -913,7 +913,7 @@ main(int argc, char **argv)
if (!ctl->connname)
ctl->connname = vshStrdup(ctl,
- virGetEnvBlockSUID("VIRSH_DEFAULT_CONNECT_URI"));
+ getenv("VIRSH_DEFAULT_CONNECT_URI"));
if (!ctl->imode) {
ret = vshCommandRun(ctl, ctl->cmd);
diff --git a/tools/virt-login-shell-helper.c b/tools/virt-login-shell-helper.c
index 8dc3bedaa0..d062c07a27 100644
--- a/tools/virt-login-shell-helper.c
+++ b/tools/virt-login-shell-helper.c
@@ -372,8 +372,8 @@ main(int argc, char **argv)
/* We're duping the string because the clearenv()
* call will shortly release the pointer we get
- * back from virGetEnvAllowSUID() right here */
- if (VIR_STRDUP(term, virGetEnvAllowSUID("TERM")) < 0)
+ * back from getenv() right here */
+ if (VIR_STRDUP(term, getenv("TERM")) < 0)
goto cleanup;
/* A fork is required to create new process in correct pid namespace. */
diff --git a/tools/vsh.c b/tools/vsh.c
index 5de082cb34..9bdd90e362 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2429,7 +2429,7 @@ vshEditWriteToTempFile(vshControl *ctl, const char *doc)
int fd;
char ebuf[1024];
- tmpdir = virGetEnvBlockSUID("TMPDIR");
+ tmpdir = getenv("TMPDIR");
if (!tmpdir) tmpdir = "/tmp";
if (virAsprintf(&ret, "%s/virshXXXXXX.xml", tmpdir) < 0) {
vshError(ctl, "%s", _("out of memory"));
@@ -2476,9 +2476,9 @@ vshEditFile(vshControl *ctl, const char *filename)
int outfd = STDOUT_FILENO;
int errfd = STDERR_FILENO;
- editor = virGetEnvBlockSUID("VISUAL");
+ editor = getenv("VISUAL");
if (!editor)
- editor = virGetEnvBlockSUID("EDITOR");
+ editor = getenv("EDITOR");
if (!editor)
editor = DEFAULT_EDITOR;
@@ -2956,7 +2956,7 @@ vshReadlineInit(vshControl *ctl)
goto cleanup;
/* Limit the total size of the history buffer */
- if ((histsize_str = virGetEnvBlockSUID(histsize_env))) {
+ if ((histsize_str = getenv(histsize_env))) {
if (virStrToLong_i(histsize_str, NULL, 10, &max_history) < 0) {
vshError(ctl, _("Bad $%s value."), histsize_env);
goto cleanup;
@@ -3072,7 +3072,7 @@ vshInitDebug(vshControl *ctl)
return -1;
/* log level not set from commandline, check env variable */
- debugEnv = virGetEnvAllowSUID(env);
+ debugEnv = getenv(env);
if (debugEnv) {
int debug;
if (virStrToLong_i(debugEnv, NULL, 10, &debug) < 0 ||
@@ -3091,7 +3091,7 @@ vshInitDebug(vshControl *ctl)
return -1;
/* log file not set from cmdline */
- debugEnv = virGetEnvBlockSUID(env);
+ debugEnv = getenv(env);
if (debugEnv && *debugEnv) {
ctl->logfile = vshStrdup(ctl, debugEnv);
vshOpenLogFile(ctl);
--
2.21.0
More information about the libvir-list
mailing list