Allow user to set environment variables on the command line as long

as they are allowed by env_keep and env_check.  Ie: apply the same
restrictions as normal environment variables.
TODO: deal with secure_path

diff --git a/env.c b/env.c
index bfc1dde5a9e8f4761fef202b5a2b641935cb68d1..f831ea298f3f3fa8996e677e879ca501890a3362 100644 (file)
--- a/env.c
+++ b/env.c
@@ -286,6 +286,95 @@ insert_env(str, e, dupcheck)
     *nep = NULL;
 }
 
+/*
+ * Check the env_delete blacklist.
+ * Returns TRUE if the variable was found, else false.
+ */
+static int
+matches_env_delete(var)
+    const char *var;
+{
+    struct list_member *cur;
+    size_t len;
+    int iswild, match = FALSE;
+
+    /* Skip anything listed in env_delete. */
+    for (cur = def_env_delete; cur; cur = cur->next) {
+       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] == '=')) {
+           match = TRUE;
+           break;
+       }
+    }
+    return(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(var)
+    const char *var;
+{
+    struct list_member *cur;
+    size_t len;
+    int iswild, keepit = -1;
+
+    for (cur = def_env_check; cur; cur = cur->next) {
+       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;
+       }
+    }
+    return(keepit);
+}
+
+/*
+ * Check the env_keep list.
+ * Returns TRUE if the variable is allowed else FALSE.
+ */
+static int
+matches_env_keep(var)
+    const char *var;
+{
+    struct list_member *cur;
+    size_t len;
+    int iswild, keepit = FALSE;
+
+    for (cur = def_env_keep; cur; cur = cur->next) {
+       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;
+       }
+    }
+    return(keepit);
+}
+
 /*
  * Build a new environment and ether clear potentially dangerous
  * variables from the old one or start with a clean slate.
@@ -299,9 +388,8 @@ rebuild_env(envp, sudo_mode, noexec)
 {
     struct list_member *cur;
     struct environment env;
-    size_t len;
     char **ep, *cp, *ps1;
-    unsigned int okvar, iswild, didvar;
+    unsigned int didvar;
 
     /*
      * Either clean out the environment or reset to a safe default.
@@ -312,7 +400,7 @@ rebuild_env(envp, sudo_mode, noexec)
     if (def_env_reset) {
        /* Pull in vars we want to keep from the old environment. */
        for (ep = envp; *ep; ep++) {
-           int keepit = -1;
+           int keepit;
 
            /* Skip variables with values beginning with () (bash functions) */
            if ((cp = strchr(*ep, '=')) != NULL) {
@@ -320,40 +408,14 @@ rebuild_env(envp, sudo_mode, noexec)
                    continue;
            }
 
-           /* Check certain variables for '%' and '/' characters. */
-           for (cur = def_env_check; cur; cur = cur->next) {
-               len = strlen(cur->value);
-               /* Deal with '*' wildcard */
-               if (cur->value[len - 1] == '*') {
-                   len--;
-                   iswild = TRUE;
-               } else
-                   iswild = FALSE;
-               if (strncmp(cur->value, *ep, len) == 0 &&
-                   (iswild || (*ep)[len] == '=')) {
-                   keepit = !strpbrk(*ep, "/%");
-                   break;
-               }
-           }
-
-           if (keepit == -1) {
-               for (cur = def_env_keep; cur; cur = cur->next) {
-                   len = strlen(cur->value);
-                   /* Deal with '*' wildcard */
-                   if (cur->value[len - 1] == '*') {
-                       len--;
-                       iswild = TRUE;
-                   } else
-                       iswild = FALSE;
-                   if (strncmp(cur->value, *ep, len) == 0 &&
-                       (iswild || (*ep)[len] == '=')) {
-                       keepit = TRUE;
-                       break;
-                   }
-               }
-           }
+           /*
+            * First check certain variables for '%' and '/' characters.
+            * If no match there, check the keep list.
+            * If nothing matched, we remove it from the environment.
+            */
+           keepit = matches_env_check(*ep);
            if (keepit == -1)
-               keepit = FALSE;
+               keepit = matches_env_keep(*ep);
 
            /* For SUDO_PS1 -> PS1 conversion. */
            if (strncmp(*ep, "SUDO_PS1=", 8) == 0)
@@ -429,7 +491,7 @@ rebuild_env(envp, sudo_mode, noexec)
         * env_check.
         */
        for (ep = envp; *ep; ep++) {
-           okvar = TRUE;
+           int okvar;
 
            /* Skip variables with values beginning with () (bash functions) */
            if ((cp = strchr(*ep, '=')) != NULL) {
@@ -437,36 +499,13 @@ rebuild_env(envp, sudo_mode, noexec)
                    continue;
            }
 
-           /* Skip anything listed in env_delete. */
-           for (cur = def_env_delete; cur && okvar; cur = cur->next) {
-               len = strlen(cur->value);
-               /* Deal with '*' wildcard */
-               if (cur->value[len - 1] == '*') {
-                   len--;
-                   iswild = TRUE;
-               } else
-                   iswild = FALSE;
-               if (strncmp(cur->value, *ep, len) == 0 &&
-                   (iswild || (*ep)[len] == '=')) {
-                   okvar = FALSE;
-               }
-           }
-
-           /* Check certain variables for '%' and '/' characters. */
-           for (cur = def_env_check; cur && okvar; cur = cur->next) {
-               len = strlen(cur->value);
-               /* Deal with '*' wildcard */
-               if (cur->value[len - 1] == '*') {
-                   len--;
-                   iswild = TRUE;
-               } else
-                   iswild = FALSE;
-               if (strncmp(cur->value, *ep, len) == 0 &&
-                   (iswild || (*ep)[len] == '=') &&
-                   strpbrk(*ep, "/%")) {
-                   okvar = FALSE;
-               }
-           }
+           /*
+            * First check variables against the blacklist in env_delete.
+            * If no match there check for '%' and '/' characters.
+            */
+           okvar = matches_env_delete(*ep) != TRUE;
+           if (okvar)
+               okvar = matches_env_check(*ep) != FALSE;
 
            if (okvar) {
                if (strncmp(*ep, "SUDO_PS1=", 9) == 0)
@@ -552,19 +591,73 @@ rebuild_env(envp, sudo_mode, noexec)
     insert_env(cp, &env, 1);
 
     /* Add user-specified environment variables. */
+    /* XXX - this is not safe, should be done after authentication. */
+    /* XXX - also honor secure_path */
     for (cur = sudo_user.env_vars; cur != NULL; cur = cur->next)
        insert_env(cur->value, &env, 1);
 
     return(env.envp);
 }
 
+/*
+ * Validate the list of environment variables passed in on the command
+ * line against env_delete, env_check, and env_keep.
+ * Calls log_error() if any specified variables are not allowed.
+ */
+void
+validate_env_vars(env_vars)
+    struct list_member *env_vars;
+{
+    struct list_member *var;
+    char *eq, *bad = NULL;
+    size_t len, blen = 0, bsize = 0;
+    int okvar;
+
+    for (var = env_vars; var != NULL; var = var->next) {
+       if (def_env_reset) {
+           okvar = matches_env_check(var->value);
+           if (okvar == -1)
+               okvar = matches_env_keep(var->value);
+       } else {
+           okvar = matches_env_delete(var->value) == FALSE;
+           if (okvar == FALSE)
+               okvar = matches_env_check(var->value) != FALSE;
+       }
+       if (okvar == FALSE) {
+           /* Not allowed, add to error string, allocating as needed. */
+           if ((eq = strchr(var->value, '=')) != NULL)
+               *eq = '\0';
+           len = strlen(var->value) + 2;
+           if (blen + len >= bsize) {
+               do {
+                   bsize += 1024;
+               } while (blen + len >= bsize);
+               bad = erealloc(bad, bsize);
+               bad[blen] = '\0';
+           }
+           strlcat(bad, var->value, bsize);
+           strlcat(bad, ", ", bsize);
+           blen += len;
+           if (eq != NULL)
+               *eq = '=';
+       }
+    }
+    if (bad != NULL) {
+       bad[blen - 2] = '\0';           /* remove trailing ", " */
+       log_error(NO_MAIL,
+           "sorry, you are not allowed to set the following environment variables: %s", bad);
+       /* NOTREACHED */
+       efree(bad);
+    }
+}
+
 void
 init_envtables()
 {
     struct list_member *cur;
     const char **p;
 
-    /* Fill in "env_delete" variable. */
+    /* Fill in the "env_delete" list. */
     for (p = initial_badenv_table; *p; p++) {
        cur = emalloc(sizeof(struct list_member));
        cur->value = estrdup(*p);
@@ -572,7 +665,7 @@ init_envtables()
        def_env_delete = cur;
     }
 
-    /* Fill in "env_check" variable. */
+    /* Fill in the "env_check" list. */
     for (p = initial_checkenv_table; *p; p++) {
        cur = emalloc(sizeof(struct list_member));
        cur->value = estrdup(*p);
@@ -580,7 +673,7 @@ init_envtables()
        def_env_check = cur;
     }
 
-    /* Fill in "env_keep" variable. */
+    /* Fill in the "env_keep" list. */
     for (p = initial_keepenv_table; *p; p++) {
        cur = emalloc(sizeof(struct list_member));
        cur->value = estrdup(*p);

diff --git a/sudo.c b/sudo.c
index e50aa647bf3a7f02d242584c5a520fcf596bd6f6..de5b9b731fc534356601c4479c0d217d27270bfe 100644 (file)
--- a/sudo.c
+++ b/sudo.c
@@ -116,6 +116,7 @@ static void usage_excl                      __P((int))
 static struct passwd *get_authpw       __P((void));
 extern int sudo_edit                   __P((int, char **, char **));
 extern char **rebuild_env              __P((char **, int, int));
+void validate_env_vars                 __P((struct list_member *));
 
 /*
  * Globals
@@ -383,9 +384,8 @@ main(argc, argv, envp)
            if (ISSET(sudo_mode, MODE_PRESERVE_ENV))
                log_error(NO_MAIL,
                    "sorry, you are not allowed to preserve the environment");
-           else if (sudo_user.env_vars != NULL)
-               log_error(NO_MAIL,
-                   "sorry, you are not allowed to set environment variables");
+           else
+               validate_env_vars(sudo_user.env_vars);
        }
 
        log_auth(validated, 1);

diff --git a/sudo.pod b/sudo.pod
index dcb39be337e286b4c1e0c4b30bc02234a5554a10..33e9b91ec8746d0186dd303a216184da0114fd52 100644 (file)
--- a/sudo.pod
+++ b/sudo.pod
@@ -318,10 +318,12 @@ line arguments.  It is most useful in conjunction with the B<-s> flag.
 
 Environment variables to be set for the command may also be passed
 on the command line in the form of B<VAR>=I<value>, e.g.
-B<LD_LIBRARY_PATH>=I</usr/local/pkg/lib>.  This is only permitted
-when the I<setenv> option is set in I<sudoers> or the command to
-be run has the C<SETENV> tag set.  See L<sudoers(@mansectform@)>
-for more information.
+B<LD_LIBRARY_PATH>=I</usr/local/pkg/lib>.  Variables passed on the
+command line are subject to the same restrictions as normal environment
+variables with one important exception.  If the I<setenv> option
+is set in I<sudoers> or the command to be run has the C<SETENV> tag
+set the user may set variables that would overwise be forbidden.
+See L<sudoers(@mansectform@)> for more information.
 
 =head1 RETURN VALUES
 

diff --git a/sudoers.pod b/sudoers.pod
index 8801c2f24c62b38f02e088b5765be361d9bef2a6..ed0fe958fb24ce451da34acb82309ccea97810ad 100644 (file)
--- a/sudoers.pod
+++ b/sudoers.pod
@@ -334,10 +334,11 @@ on how C<NOEXEC> works and whether or not it will work on your system.
 =head3 SETENV and NOSETENV
 
 These tags override the value of the I<setenv> option on a per-command
-basis.  Note that environment variables set on the command line way
-are not subject to the restrictions imposed by I<env_check>,
-I<env_delete>, or I<env_reset>.  As such, only trusted users should
-be allowed to set variables in this manner.
+basis.  Note that if C<SETENV> has been set for a command, any
+environment variables set on the command line way are not subject
+to the restrictions imposed by I<env_check>, I<env_delete>, or
+I<env_keep>.  As such, only trusted users should be allowed to set
+variables in this manner.
 
 =head3 MONITOR and NOMONITOR
 
@@ -779,11 +780,11 @@ The default is 3.
 
 =item setenv
 
-Allow the user to set additional environment variables from the
-command line.  Note that variables set this way are not subject to
-the restrictions imposed by I<env_check>, I<env_delete>, or
-I<env_reset>.  As such, only trusted users should be allowed to set
-variables in this manner.
+Allow the user to disable the I<env_reset> option from the command
+line.  Additionally, environment variables set via the command line
+are not subject to the restrictions imposed by I<env_check>,
+I<env_delete>, or I<env_keep>.  As such, only trusted users should
+be allowed to set variables in this manner.
 
 =back