[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCH 08/17] util: get rid of virGetEnv{Allow, Block}SUID functions



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]