From df0fd4153043befd4815e18f3cfca72ae29baf24 Mon Sep 17 00:00:00 2001 From: "Todd C. Miller" Date: Wed, 6 Aug 2014 16:45:57 -0600 Subject: [PATCH] Add explicit support for matching the full environment string (name=value). Bash functions may now be preserved for full matches, but not for name-only matches. --- NEWS | 5 ++ doc/sudoers.cat | 28 ++++++--- doc/sudoers.man.in | 36 ++++++++++-- doc/sudoers.mdoc.in | 33 +++++++++-- plugins/sudoers/env.c | 128 +++++++++++++++++++++--------------------- 5 files changed, 149 insertions(+), 81 deletions(-) diff --git a/NEWS b/NEWS index b128bb526..fef3e09d9 100644 --- a/NEWS +++ b/NEWS @@ -32,6 +32,11 @@ What's new in Sudo 1.8.11 * Sudo and its associated programs now link against a shared version of libsudo_util. + * It is now possible to match an environment variable's value as + well as its name using env_keep and env_check. This can be used + to preserve bash functions which would otherwise be removed from + the environment. + What's new in Sudo 1.8.10p3? * Fixed expansion of %p in the prompt for "sudo -l" when rootpw, diff --git a/doc/sudoers.cat b/doc/sudoers.cat index 16663e4c5..330af8587 100644 --- a/doc/sudoers.cat +++ b/doc/sudoers.cat @@ -122,19 +122,31 @@ DDEESSCCRRIIPPTTIIOONN PATH, HOME, MAIL, SHELL, LOGNAME, USER, USERNAME and SUDO_* variables in addition to variables from the invoking process permitted by the _e_n_v___c_h_e_c_k and _e_n_v___k_e_e_p options. This is effectively a whitelist for - environment variables. + environment variables. Environment variables with a value beginning with + () are removed unless both the name and value parts are matched by + _e_n_v___k_e_e_p or _e_n_v___c_h_e_c_k, as they could be interpreted as bbaasshh functions. + Prior to version 1.8.11, such variables were always removed. If, however, the _e_n_v___r_e_s_e_t option is disabled, any variables not explicitly denied by the _e_n_v___c_h_e_c_k and _e_n_v___d_e_l_e_t_e options are inherited from the invoking process. In this case, _e_n_v___c_h_e_c_k and _e_n_v___d_e_l_e_t_e behave - like a blacklist. Since it is not possible to blacklist all potentially - dangerous environment variables, use of the default _e_n_v___r_e_s_e_t behavior is + like a blacklist. Environment variables with a value beginning with () + are always removed, even if they do not match one of the blacklists. + Since it is not possible to blacklist all potentially dangerous + environment variables, use of the default _e_n_v___r_e_s_e_t behavior is encouraged. - In all cases, environment variables with a value beginning with () are - removed as they could be interpreted as bbaasshh functions. The list of - environment variables that ssuuddoo allows or denies is contained in the - output of ``sudo -V'' when run as root. + By default, environment variables are matched by name. However, if the + pattern includes an equal sign (`='), both the variables name and value + must match. For example, a bbaasshh function could be matched as follows: + + env_keep += "my_func=()*" + + Without the ``=()*'' suffix, this would not match, as bbaasshh functions are + not preserved by default. + + The list of environment variables that ssuuddoo allows or denies is contained + in the output of ``sudo -V'' when run as root. Note that the dynamic linker on most operating systems will remove variables that can control dynamic linking from the environment of setuid @@ -2331,4 +2343,4 @@ DDIISSCCLLAAIIMMEERR file distributed with ssuuddoo or http://www.sudo.ws/sudo/license.html for complete details. -Sudo 1.8.11 July 16, 2014 Sudo 1.8.11 +Sudo 1.8.11b1 July 16, 2014 Sudo 1.8.11b1 diff --git a/doc/sudoers.man.in b/doc/sudoers.man.in index eddcb4ba6..d3595b406 100644 --- a/doc/sudoers.man.in +++ b/doc/sudoers.man.in @@ -304,6 +304,16 @@ and options. This is effectively a whitelist for environment variables. +Environment variables with a value beginning with +\fR()\fR +are removed unless both the name and value parts are matched by +\fIenv_keep\fR +or +\fIenv_check\fR, +as they could be interpreted as +\fBbash\fR +functions. +Prior to version 1.8.11, such variables were always removed. .PP If, however, the \fIenv_reset\fR @@ -319,17 +329,35 @@ In this case, and \fIenv_delete\fR behave like a blacklist. +Environment variables with a value beginning with +\fR()\fR +are always removed, even if they do not match one of the blacklists. Since it is not possible to blacklist all potentially dangerous environment variables, use of the default \fIenv_reset\fR behavior is encouraged. .PP -In all cases, environment variables with a value beginning with -\fR()\fR -are removed as they could be interpreted as +By default, environment variables are matched by name. +However, if the pattern includes an equal sign +(\(oq=\&\(cq), +both the variables name and value must match. +For example, a \fBbash\fR -functions. +function could be matched as follows: +.nf +.sp +.RS 4n +env_keep += "my_func=()*" +.RE +.fi +.PP +Without the +\(lq\fR=()*\fR\(rq +suffix, this would not match, as +\fBbash\fR +functions are not preserved by default. +.PP The list of environment variables that \fBsudo\fR allows or denies is diff --git a/doc/sudoers.mdoc.in b/doc/sudoers.mdoc.in index 4450cdda8..80b041746 100644 --- a/doc/sudoers.mdoc.in +++ b/doc/sudoers.mdoc.in @@ -293,6 +293,16 @@ and options. This is effectively a whitelist for environment variables. +Environment variables with a value beginning with +.Li () +are removed unless both the name and value parts are matched by +.Em env_keep +or +.Em env_check , +as they could be interpreted as +.Sy bash +functions. +Prior to version 1.8.11, such variables were always removed. .Pp If, however, the .Em env_reset @@ -308,17 +318,32 @@ In this case, and .Em env_delete behave like a blacklist. +Environment variables with a value beginning with +.Li () +are always removed, even if they do not match one of the blacklists. Since it is not possible to blacklist all potentially dangerous environment variables, use of the default .Em env_reset behavior is encouraged. .Pp -In all cases, environment variables with a value beginning with -.Li () -are removed as they could be interpreted as +By default, environment variables are matched by name. +However, if the pattern includes an equal sign +.Pq Ql =\& , +both the variables name and value must match. +For example, a .Sy bash -functions. +function could be matched as follows: +.Bd -literal -offset 4n +env_keep += "my_func=()*" +.Ed +.Pp +Without the +.Dq Li =()* +suffix, this would not match, as +.Sy bash +functions are not preserved by default. +.Pp The list of environment variables that .Nm sudo allows or denies is diff --git a/plugins/sudoers/env.c b/plugins/sudoers/env.c index 5ba854326..ddc4738c8 100644 --- a/plugins/sudoers/env.c +++ b/plugins/sudoers/env.c @@ -536,29 +536,32 @@ sudo_getenv(const char *name) } /* - * Check the env_delete blacklist. + * Check for var against patterns in the specified environment list. * Returns true if the variable was found, else false. */ static bool -matches_env_delete(const char *var) +matches_env_list(const char *var, struct list_members *list, bool *full_match) { struct list_member *cur; - size_t len; - bool iswild; bool match = false; - debug_decl(matches_env_delete, SUDO_DEBUG_ENV) + debug_decl(matches_env_list, SUDO_DEBUG_ENV) - /* Skip anything listed in env_delete. */ - SLIST_FOREACH(cur, &def_env_delete, entries) { - len = strlen(cur->value); - /* Deal with '*' wildcard */ + SLIST_FOREACH(cur, list, entries) { + size_t sep_pos, len = strlen(cur->value); + bool iswild = false; + + /* Locate position of the '=' separator in var=value. */ + sep_pos = strcspn(var, "="); + + /* Deal with '*' wildcard at the end of the pattern. */ if (cur->value[len - 1] == '*') { len--; iswild = true; - } else - iswild = false; + } if (strncmp(cur->value, var, len) == 0 && - (iswild || var[len] == '=')) { + (iswild || len == sep_pos || var[len] == '\0')) { + /* If we matched past the '=', count as a full match. */ + *full_match = len > sep_pos + 1; match = true; break; } @@ -566,33 +569,36 @@ matches_env_delete(const char *var) debug_return_bool(match); } +/* + * Check the env_delete blacklist. + * Returns true if the variable was found, else false. + */ +static bool +matches_env_delete(const char *var) +{ + bool full_match; /* unused */ + debug_decl(matches_env_delete, SUDO_DEBUG_ENV) + + /* Skip anything listed in env_delete. */ + debug_return_bool(matches_env_list(var, &def_env_delete, &full_match)); +} + /* * Apply the env_check list. * Returns true if the variable is allowed, false if denied * or -1 if no match. */ static int -matches_env_check(const char *var) +matches_env_check(const char *var, bool *full_match) { - struct list_member *cur; - size_t len; - bool iswild; int keepit = -1; debug_decl(matches_env_check, SUDO_DEBUG_ENV) - SLIST_FOREACH(cur, &def_env_check, entries) { - len = strlen(cur->value); - /* Deal with '*' wildcard */ - if (cur->value[len - 1] == '*') { - len--; - iswild = true; - } else - iswild = false; - if (strncmp(cur->value, var, len) == 0 && - (iswild || var[len] == '=')) { - keepit = !strpbrk(var, "/%"); - break; - } + /* Skip anything listed in env_check that includes '/' or '%'. */ + if (matches_env_list(var, &def_env_check, full_match)) { + const char *val = strchr(var, '='); + if (val != NULL) + keepit = !strpbrk(++val, "/%"); } debug_return_bool(keepit); } @@ -602,34 +608,17 @@ matches_env_check(const char *var) * Returns true if the variable is allowed else false. */ static bool -matches_env_keep(const char *var) +matches_env_keep(const char *var, bool *full_match) { - struct list_member *cur; - size_t len; - bool iswild, keepit = false; + bool keepit = false; debug_decl(matches_env_keep, SUDO_DEBUG_ENV) /* Preserve SHELL variable for "sudo -s". */ if (ISSET(sudo_mode, MODE_SHELL) && strncmp(var, "SHELL=", 6) == 0) { keepit = true; - goto done; - } - - SLIST_FOREACH(cur, &def_env_keep, entries) { - len = strlen(cur->value); - /* Deal with '*' wildcard */ - if (cur->value[len - 1] == '*') { - len--; - iswild = true; - } else - iswild = false; - if (strncmp(cur->value, var, len) == 0 && - (iswild || var[len] == '=')) { - keepit = true; - break; - } + } else if (matches_env_list(var, &def_env_keep, full_match)) { + keepit = true; } -done: debug_return_bool(keepit); } @@ -640,13 +629,24 @@ done: static bool env_should_delete(const char *var) { + const char *cp; int delete_it; + bool full_match = false; debug_decl(env_should_delete, SUDO_DEBUG_ENV); + /* Skip variables with values beginning with () (bash functions) */ + if ((cp = strchr(var, '=')) != NULL) { + if (strncmp(cp, "=() ", 3) == 0) { + delete_it = true; + goto done; + } + } + delete_it = matches_env_delete(var); if (!delete_it) - delete_it = matches_env_check(var) == false; + delete_it = matches_env_check(var, &full_match) == false; +done: sudo_debug_printf(SUDO_DEBUG_INFO, "delete %s: %s", var, delete_it ? "YES" : "NO"); debug_return_bool(delete_it); @@ -660,12 +660,21 @@ static bool env_should_keep(const char *var) { int keepit; + bool full_match = false; + const char *cp; debug_decl(env_should_keep, SUDO_DEBUG_ENV) - keepit = matches_env_check(var); + keepit = matches_env_check(var, &full_match); if (keepit == -1) - keepit = matches_env_keep(var); + keepit = matches_env_keep(var, &full_match); + /* Skip bash functions unless we matched on the value as well as name. */ + if (keepit && !full_match) { + if ((cp = strchr(var, '=')) != NULL) { + if (strncmp(cp, "=() ", 3) == 0) + keepit = false; + } + } sudo_debug_printf(SUDO_DEBUG_INFO, "keep %s: %s", var, keepit ? "YES" : "NO"); debug_return_bool(keepit == true); @@ -685,6 +694,7 @@ env_merge(char * const envp[]) debug_decl(env_merge, SUDO_DEBUG_ENV) for (ep = envp; *ep != NULL; ep++) { + /* XXX - avoid checking value here too */ if (sudo_putenv(*ep, true, !env_should_keep(*ep)) == -1) { /* XXX cannot undo on failure */ rval = false; @@ -800,12 +810,6 @@ rebuild_env(void) for (ep = old_envp; *ep; ep++) { bool keepit; - /* Skip variables with values beginning with () (bash functions) */ - if ((cp = strchr(*ep, '=')) != NULL) { - if (strncmp(cp, "=() ", 3) == 0) - continue; - } - /* * Look up the variable in the env_check and env_keep lists. */ @@ -880,12 +884,6 @@ rebuild_env(void) * env_check. */ for (ep = old_envp; *ep; ep++) { - /* Skip variables with values beginning with () (bash functions) */ - if ((cp = strchr(*ep, '=')) != NULL) { - if (strncmp(cp, "=() ", 3) == 0) - continue; - } - /* Add variable unless it matches a black list. */ if (!env_should_delete(*ep)) { if (strncmp(*ep, "SUDO_PS1=", 9) == 0) -- 2.49.0