CVE-2019-25016: Incomplete Cleanup (CWE-459)
value: In OpenDoas from 6.6 to 6.8 the users PATH variable was incorrectly inherited by authenticated executions if the authenticating rule allowed the user to execute any command. Rules that only allowed to authenticated user to execute specific commands were not affected by this issue.
CWE description: The product does not properly "clean up" and remove temporary or supporting resources after they have been used.
CVSS v3 base score: 8.8
Published: 2021-01-28T20:15Z
Repository: https://github.com/Duncaen/OpenDoas
Language: C
Files changed: doas.c, doas.conf.5, doas.h, env.c
Commit date: 2019-10-19 14:52:39 +0200
Commit message: redo the environment inheritance to not inherit. it was intended to make life easier, but it can be surprising or even unsafe. instead, reset just about everything to the target user's values. ok deraadt martijn Thanks to Sander Bos in particular for pointing out some nasty edge cases.

Diff: diff --git a/doas.c b/doas.c
index 1fd0e9a..5396df0 100644
--- a/doas.c
+++ b/doas.c
@@ -447,10 +447,11 @@ main(int argc, char **argv)
 	    rule->options & PERSIST);
 #endif
 
 #ifdef HAVE_SETUSERCONTEXT
 	if (setusercontext(NULL, targpw, target, LOGIN_SETGROUP |
+	    LOGIN_SETPATH |
 	    LOGIN_SETPRIORITY | LOGIN_SETRESOURCES | LOGIN_SETUMASK |
 	    LOGIN_SETUSER) != 0)
 		errx(1, "failed to set user context for target");
 #else
 	if (setresgid(targpw->pw_gid, targpw->pw_gid, targpw->pw_gid) != 0)
@@ -477,13 +478,14 @@ main(int argc, char **argv)
 #endif
 
 	syslog(LOG_AUTHPRIV | LOG_INFO, "%s ran command %s as %s from %s",
 	    mypw->pw_name, cmdline, targpw->pw_name, cwd);
 
-	envp = prepenv(rule);
+	envp = prepenv(rule, mypw, targpw);
 
 	if (rule->cmd) {
+		/* do this again after setusercontext reset it */
 		if (setenv("PATH", safepath, 1) == -1)
 			err(1, "failed to set PATH '%s'", safepath);
 	}
 	execvpe(cmd, argv, envp);
 	if (errno == ENOENT)
diff --git a/doas.conf.5 b/doas.conf.5
index 95daf4c..8fd700b 100644
--- a/doas.conf.5
+++ b/doas.conf.5
@@ -49,19 +49,13 @@ The user is not required to enter a password.
 After the user successfully authenticates, do not ask for a password
 again for some time.
 .It Ic keepenv
 The user's environment is maintained.
 The default is to reset the environment, except for the variables
-.Ev DISPLAY ,
-.Ev HOME ,
-.Ev LOGNAME ,
-.Ev MAIL ,
-.Ev PATH ,
-.Ev TERM ,
-.Ev USER
+.Ev DISPLAY
 and
-.Ev USERNAME .
+.Ev TERM .
 .It Ic setenv { Oo Ar variable ... Oc Oo Ar variable=value ... Oc Ic }
 In addition to the variables mentioned above, keep the space-separated
 specified variables.
 Variables may also be removed with a leading
 .Sq -
diff --git a/doas.h b/doas.h
index 48069db..3831dc7 100644
--- a/doas.h
+++ b/doas.h
@@ -27,11 +27,14 @@ struct rule {
 
 extern struct rule **rules;
 extern int nrules;
 extern int parse_errors;
 
-char **prepenv(const struct rule *);
+struct passwd;
+
+char **prepenv(const struct rule *, const struct passwd *,
+    const struct passwd *);
 
 #define PERMIT	1
 #define DENY	2
 
 #define NOPASS		0x1
diff --git a/env.c b/env.c
index 3e8b95d..c0f3837 100644
--- a/env.c
+++ b/env.c
@@ -22,10 +22,11 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <err.h>
 #include <unistd.h>
 #include <errno.h>
+#include <pwd.h>
 
 #include "doas.h"
 #include "includes.h"
 
 struct envnode {
@@ -37,10 +38,12 @@ struct envnode {
 struct env {
 	RB_HEAD(envtree, envnode) root;
 	u_int count;
 };
 
+static void fillenv(struct env *env, const char **envlist);
+
 static int
 envcmp(struct envnode *a, struct envnode *b)
 {
 	return strcmp(a->key, b->key);
 }
@@ -67,22 +70,35 @@ freenode(struct envnode *node)
 	free((char *)node->key);
 	free((char *)node->value);
 	free(node);
 }
 
+static void
+addnode(struct env *env, const char *key, const char *value)
+{
+	struct envnode *node;
+
+	node = createnode(key, value);
+	RB_INSERT(envtree, &env->root, node);
+	env->count++;
+}
+
 static struct env *
-createenv(const struct rule *rule)
+createenv(const struct rule *rule, const struct passwd *mypw,
+    const struct passwd *targpw)
 {
 	struct env *env;
 	u_int i;
 
 	env = malloc(sizeof(*env));
 	if (!env)
 		err(1, NULL);
 	RB_INIT(&env->root);
 	env->count = 0;
 
+	addnode(env, "DOAS_USER", mypw->pw_name);
+
 	if (rule->options & KEEPENV) {
 		extern char **environ;
 
 		for (i = 0; environ[i] != NULL; i++) {
 			struct envnode *node;
@@ -107,10 +123,23 @@ createenv(const struct rule *rule)
 				freenode(node);
 			} else {
 				env->count++;
 			}
 		}
+	} else {
+		static const char *copyset[] = {
+			"DISPLAY", "TERM",
+			NULL
+		};
+
+		addnode(env, "HOME", targpw->pw_dir);
+		addnode(env, "LOGNAME", targpw->pw_name);
+		addnode(env, "PATH", getenv("PATH"));
+		addnode(env, "SHELL", targpw->pw_shell);
+		addnode(env, "USER", targpw->pw_name);
+
+		fillenv(env, copyset);
 	}
 
 	return env;
 }
 
@@ -185,24 +214,16 @@ fillenv(struct env *env, const char **envlist)
 		}
 	}
 }
 
 char **
-prepenv(const struct rule *rule)
+prepenv(const struct rule *rule, const struct passwd *mypw,
+    const struct passwd *targpw)
 {
-	static const char *safeset[] = {
-		"DISPLAY", "HOME", "LOGNAME", "MAIL",
-		"PATH", "TERM", "USER", "USERNAME",
-		NULL
-	};
 	struct env *env;
 
-	env = createenv(rule);
-
-	/* if we started with blank, fill some defaults then apply rules */
-	if (!(rule->options & KEEPENV))
-		fillenv(env, safeset);
+	env = createenv(rule, mypw, targpw);
 	if (rule->envlist)
 		fillenv(env, rule->envlist);
 
 	return flattenenv(env);
 }

Vulnerable code: 	envp = prepenv(rule);
.Ev DISPLAY ,
.Ev HOME ,
.Ev LOGNAME ,
.Ev MAIL ,
.Ev PATH ,
.Ev TERM ,
.Ev USER
.Ev USERNAME .
char **prepenv(const struct rule *);
createenv(const struct rule *rule)
prepenv(const struct rule *rule)
	static const char *safeset[] = {
		"DISPLAY", "HOME", "LOGNAME", "MAIL",
		"PATH", "TERM", "USER", "USERNAME",
		NULL
	};
	env = createenv(rule);

	/* if we started with blank, fill some defaults then apply rules */
	if (!(rule->options & KEEPENV))
		fillenv(env, safeset);
Fixed code: 	    LOGIN_SETPATH |
	envp = prepenv(rule, mypw, targpw);
		/* do this again after setusercontext reset it */
.Ev DISPLAY
.Ev TERM .
struct passwd;

char **prepenv(const struct rule *, const struct passwd *,
    const struct passwd *);
#include <pwd.h>
static void fillenv(struct env *env, const char **envlist);

static void
addnode(struct env *env, const char *key, const char *value)
{
	struct envnode *node;

	node = createnode(key, value);
	RB_INSERT(envtree, &env->root, node);
	env->count++;
}

createenv(const struct rule *rule, const struct passwd *mypw,
    const struct passwd *targpw)
	addnode(env, "DOAS_USER", mypw->pw_name);

	} else {
		static const char *copyset[] = {
			"DISPLAY", "TERM",
			NULL
		};

		addnode(env, "HOME", targpw->pw_dir);
		addnode(env, "LOGNAME", targpw->pw_name);
		addnode(env, "PATH", getenv("PATH"));
		addnode(env, "SHELL", targpw->pw_shell);
		addnode(env, "USER", targpw->pw_name);

		fillenv(env, copyset);
prepenv(const struct rule *rule, const struct passwd *mypw,
    const struct passwd *targpw)
	env = createenv(rule, mypw, targpw);