From b9f1e9d6a675f7ec03ac095c300075508c56c083 Mon Sep 17 00:00:00 2001 From: Sergei Trofimovich Date: Sun, 6 Jan 2019 09:32:55 +0000 Subject: [PATCH] wrapper: exec*: never override host's 'environ' In bug #669702 gcc exposed sandbox bug wehere 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 Bug: https://bugs.gentoo.org/669702 Signed-off-by: Sergei Trofimovich --- 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(-) create mode 100644 tests/execv-0.c create mode 100755 tests/execv-1.sh create mode 100644 tests/execv.at 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 +#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) -- 2.20.1