[gentoo-commits] proj/sandbox:master commit in: libsandbox/, tests/, libsandbox/wrapper-funcs/

[Sergei Trofimovich]

commit:     f3e51a930312422cc78b693a247b7c5704ac90a2
Author:     Sergei Trofimovich <slyfox <AT> gentoo <DOT> org>
AuthorDate: Sun Jan  6 09:32:55 2019 +0000
Commit:     Sergei Trofimovich <slyfox <AT> gentoo <DOT> org>
CommitDate: Tue Jan  8 22:48:44 2019 +0000
URL:        https://gitweb.gentoo.org/proj/sandbox.git/commit/?id=f3e51a93

exec*() wrappers: never mutate 'environ' of host process

In bug #669702 gcc exposed sandbox bug where execv()
wrapper changed 'environ' global variable underneath.

A few GNU projects (pex_unix_exec_child in gcc and gdb)
use the following idiom:

for (;;) {
    vfork();
    char ** save_environ = environ; // [1]
    if (child) {
        environ = child_environ; // [2]
        execv(payload); // [3]
    }
    if (parent) {
        environ = save_environ; // [4]
        ...
        waitpid(child, ...);
    }
}

Code above assumes that execv() does not mutate 'environ'.

In case of #669702 sandbox's execv() wrapper at '[3]' mutated
'environ' and relocated it (via maloc()/free() internally).
This caused '[4]' to point 'environ' fo freed location.

The change fixes it in a following way:
- execv() call now works more like execve() call by mutating
  external array and substitutes 'environ' only for a period
  of 'execv()' execution.
- add basic execv()/'environ' corruption test

Tested on:
- linux/glibc-2.28
- linux/uclibc-ng-1.0.31

Reported-and-tested-by: Walther
Reported-by: 0x6d6174 <AT> posteo.de
Reported-by: Andrey Korolyov
Bug: https://bugs.gentoo.org/669702
Signed-off-by: Sergei Trofimovich <slyfox <AT> gentoo.org>

 libsandbox/libsandbox.c                   | 92 +++++++++++++++----------------
 libsandbox/libsandbox.h                   | 17 +++++-
 libsandbox/wrapper-funcs/__wrapper_exec.c | 15 +++--
 tests/Makefile.am                         |  1 +
 tests/execv-0.c                           | 21 +++++++
 tests/execv-1.sh                          |  4 ++
 tests/execv.at                            |  1 +
 7 files changed, 96 insertions(+), 55 deletions(-)

diff --git a/libsandbox/libsandbox.c b/libsandbox/libsandbox.c
index e0c9d1a..77e9959 100644
--- a/libsandbox/libsandbox.c
+++ b/libsandbox/libsandbox.c
@@ -1122,10 +1122,18 @@ typedef struct {
  *
  * XXX: Might be much nicer if we could serialize these vars behind the back of
  *      the program.  Might be hard to handle LD_PRELOAD though ...
+ *
+ * execv*() must never modify environment inplace with
+ * setenv/putenv/unsetenv as it can relocate 'environ' and break
+ * vfork()/execv() users: https://bugs.gentoo.org/669702
  */
-char **sb_check_envp(char **envp, size_t *mod_cnt, bool insert)
+struct sb_envp_ctx sb_new_envp(char **envp, bool insert)
 {
-	char **my_env;
+	struct sb_envp_ctx r = {
+		.sb_envp   = envp,
+		.orig_envp = envp,
+		.__mod_cnt = 0,
+	};
 	char *entry;
 	size_t count, i;
 	env_pair vars[] = {
@@ -1152,7 +1160,7 @@ char **sb_check_envp(char **envp, size_t *mod_cnt, bool insert)
 	/* If sandbox is explicitly disabled, do not propagate the vars
 	 * and just return user's envp */
 	if (!sbcontext.on)
-		return envp;
+		return r;
 
 	/* First figure out how many vars are already in the env */
 	found_var_cnt = 0;
@@ -1188,7 +1196,7 @@ char **sb_check_envp(char **envp, size_t *mod_cnt, bool insert)
 	if ((insert && num_vars == found_var_cnt) ||
 	    (!insert && found_var_cnt == 0))
 		/* Use the user's envp */
-		return envp;
+		return r;
 
 	/* Ok, we need to create our own envp, as we need to restore stuff
 	 * and we should not touch the user's envp.  First we add our vars,
@@ -1208,61 +1216,50 @@ char **sb_check_envp(char **envp, size_t *mod_cnt, bool insert)
 		vars[13].value = "1";
 	}
 
-	my_env = NULL;
+	char ** my_env = NULL;
 	if (!insert) {
-		if (mod_cnt) {
-			str_list_for_each_item(envp, entry, count) {
-				for (i = 0; i < num_vars; ++i)
-					if (i != 12 && is_env_var(entry, vars[i].name, vars[i].len)) {
-						(*mod_cnt)++;
-						goto skip;
-					}
-				str_list_add_item(my_env, entry, error);
- skip: ;
-			}
-		} else {
+		str_list_for_each_item(envp, entry, count) {
 			for (i = 0; i < num_vars; ++i)
-				if (i != 12) unsetenv(vars[i].name);
+				if (i != 12 /* LD_LIBRARY_PATH index */
+				    && is_env_var(entry, vars[i].name, vars[i].len)) {
+					r.__mod_cnt++;
+					goto skip;
+				}
+			str_list_add_item(my_env, entry, error);
+ skip: ;
 		}
 	} else {
-		if (mod_cnt) {
-			/* Count directly due to variability with LD_PRELOAD and the value
-			 * logic below.  Getting out of sync can mean memory corruption. */
-			*mod_cnt = 0;
-			if (unlikely(merge_ld_preload)) {
-				str_list_add_item(my_env, ld_preload, error);
-				(*mod_cnt)++;
-			}
-			for (i = 0; i < num_vars; ++i) {
-				if (found_vars[i] || !vars[i].value)
-					continue;
-				str_list_add_item_env(my_env, vars[i].name, vars[i].value, error);
-				(*mod_cnt)++;
-			}
+		/* Count directly due to variability with LD_PRELOAD and the value
+		 * logic below.  Getting out of sync can mean memory corruption. */
+		r.__mod_cnt = 0;
+		if (unlikely(merge_ld_preload)) {
+			str_list_add_item(my_env, ld_preload, error);
+			r.__mod_cnt++;
+		}
+		for (i = 0; i < num_vars; ++i) {
+			if (found_vars[i] || !vars[i].value)
+				continue;
+			str_list_add_item_env(my_env, vars[i].name, vars[i].value, error);
+			r.__mod_cnt++;
+		}
 
-			str_list_for_each_item(envp, entry, count) {
-				if (unlikely(merge_ld_preload && is_env_var(entry, vars[0].name, vars[0].len)))
-					continue;
-				str_list_add_item(my_env, entry, error);
-			}
-		} else {
-			if (unlikely(merge_ld_preload))
-				putenv(ld_preload);
-			for (i = 0; i < num_vars; ++i) {
-				if (found_vars[i] || !vars[i].value)
-					continue;
-				setenv(vars[i].name, vars[i].value, 1);
-			}
+		str_list_for_each_item(envp, entry, count) {
+			if (unlikely(merge_ld_preload && is_env_var(entry, vars[0].name, vars[0].len)))
+				continue;
+			str_list_add_item(my_env, entry, error);
 		}
 	}
 
  error:
-	return my_env;
+	r.sb_envp = my_env;
+	return r;
 }
 
-void sb_cleanup_envp(char **envp, size_t mod_cnt)
+void sb_free_envp(struct sb_envp_ctx * envp_ctx)
 {
 	/* We assume all the stuffed vars are at the start */
+	size_t mod_cnt = envp_ctx->__mod_cnt;
+	char ** envp = envp_ctx->sb_envp;
 	size_t i;
 	for (i = 0; i < mod_cnt; ++i)
 		free(envp[i]);
@@ -1271,5 +1268,6 @@ void sb_cleanup_envp(char **envp, size_t mod_cnt)
 	 * entries except for LD_PRELOAD.  All the other entries are
 	 * pointers to existing envp memory.
 	 */
-	free(envp);
+	if (envp != envp_ctx->orig_envp)
+		free(envp);
 }

diff --git a/libsandbox/libsandbox.h b/libsandbox/libsandbox.h
index 63882e7..70fc422 100644
--- a/libsandbox/libsandbox.h
+++ b/libsandbox/libsandbox.h
@@ -56,8 +56,21 @@ void *get_dlsym(const char *symname, const char *symver);
 
 extern char sandbox_lib[SB_PATH_MAX];
 extern bool sandbox_on;
-char **sb_check_envp(char **envp, size_t *mod_cnt, bool insert);
-void sb_cleanup_envp(char **envp, size_t mod_cnt);
+
+struct sb_envp_ctx {
+	/* Sandboxified environment with sandbox variables injected.
+	 * Allocated by 'sb_new_envp', freed by 'sb_free_envp'. */
+	char ** sb_envp;
+	/* Original environment. Passed from outside. */
+	char ** orig_envp;
+
+	/* Internal counter to free.
+	 * Not to be used outside sb_{new,free}_envp. */
+	size_t __mod_cnt;
+};
+struct sb_envp_ctx sb_new_envp(char **envp, bool insert);
+void sb_free_envp(struct sb_envp_ctx * envp_ctx);
+
 extern pid_t trace_pid;
 
 extern void sb_lock(void);

diff --git a/libsandbox/wrapper-funcs/__wrapper_exec.c b/libsandbox/wrapper-funcs/__wrapper_exec.c
index 226c0c0..974156e 100644
--- a/libsandbox/wrapper-funcs/__wrapper_exec.c
+++ b/libsandbox/wrapper-funcs/__wrapper_exec.c
@@ -275,10 +275,11 @@ WRAPPER_RET_TYPE WRAPPER_NAME(WRAPPER_ARGS_PROTO)
 #endif
 
 #ifdef EXEC_MY_ENV
-	size_t mod_cnt;
-	char **my_env = sb_check_envp((char **)envp, &mod_cnt, run_in_process);
+	struct sb_envp_ctx ec = sb_new_envp((char**)envp, run_in_process);
+	char **my_env = ec.sb_envp;
 #else
-	sb_check_envp(environ, NULL, run_in_process);
+	struct sb_envp_ctx ec = sb_new_envp(environ, run_in_process);
+	environ = ec.sb_envp;
 #endif
 
 	restore_errno();
@@ -287,10 +288,12 @@ WRAPPER_RET_TYPE WRAPPER_NAME(WRAPPER_ARGS_PROTO)
 #endif
 	result = SB_HIDDEN_FUNC(WRAPPER_NAME)(EXEC_ARGS);
 
-#ifdef EXEC_MY_ENV
-	if (my_env != envp)
-		sb_cleanup_envp(my_env, mod_cnt);
+#ifndef EXEC_MY_ENV
+	/* https://bugs.gentoo.org/669702: maintain illusion
+	 or unmodified 'environ'. */
+	environ = ec.orig_envp;
 #endif
+	sb_free_envp(&ec);
 
 #ifdef EXEC_RECUR_CHECK
 	--recursive;

diff --git a/tests/Makefile.am b/tests/Makefile.am
index d98898f..3baf5b1 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -18,6 +18,7 @@ check_PROGRAMS = \
 	chown-0 \
 	creat-0 \
 	creat64-0 \
+	execv-0 \
 	execvp-0 \
 	faccessat-0 \
 	fchmodat-0 \

diff --git a/tests/execv-0.c b/tests/execv-0.c
new file mode 100644
index 0000000..bea0aaa
--- /dev/null
+++ b/tests/execv-0.c
@@ -0,0 +1,21 @@
+/*
+ * A simple wrapper for execv that validates environment
+ * is not corrupted by wrapper: https://bugs.gentoo.org/669702
+ */
+
+#define _GNU_SOURCE /* for environ */
+#include <stdio.h>
+#include "tests.h"
+
+int main(int argc, char *argv[])
+{
+	char* execv_argv[] = {"nope", (char*)NULL,};
+	char* execv_environ[] = {"FOO=1", (char*)NULL,};
+	environ = execv_environ;
+	execv("./does/not/exist", execv_argv);
+	if (environ != execv_environ) {
+		fprintf(stderr, "environ was changed unexpectedly by execv wrapper\n");
+		return 1;
+	}
+	return 0;
+}

diff --git a/tests/execv-1.sh b/tests/execv-1.sh
new file mode 100755
index 0000000..ab816b0
--- /dev/null
+++ b/tests/execv-1.sh
@@ -0,0 +1,4 @@
+#!/bin/sh
+# Make sure execv run does not corrup 'environ' of caller process:
+# https://bugs.gentoo.org/669702
+timeout -s KILL 10 execv-0

diff --git a/tests/execv.at b/tests/execv.at
new file mode 100644
index 0000000..081d7d2
--- /dev/null
+++ b/tests/execv.at
@@ -0,0 +1 @@
+SB_CHECK(1)