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)
