Go to:
Gentoo Home
Documentation
Forums
Lists
Bugs
Planet
Store
Wiki
Get Gentoo!
Gentoo's Bugzilla – Attachment 560120 Details for
Bug 669702
=sys-apps/sandbox-2.13 kills gcc (segmentation fault) if /bin/sh points to /bin/dash instead of /bin/bash (=app-shells/dash-0.5.10.2) for some packages
Home
|
New
–
[Ex]
|
Browse
|
Search
|
Privacy Policy
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
sandbox-2.14-execv-environ.patch
sandbox-2.14-execv-environ.patch (text/plain), 9.59 KB, created by
Sergei Trofimovich (RETIRED)
on 2019-01-06 18:25:55 UTC
(
hide
)
Description:
sandbox-2.14-execv-environ.patch
Filename:
MIME Type:
Creator:
Sergei Trofimovich (RETIRED)
Created:
2019-01-06 18:25:55 UTC
Size:
9.59 KB
patch
obsolete
>From b9f1e9d6a675f7ec03ac095c300075508c56c083 Mon Sep 17 00:00:00 2001 >From: Sergei Trofimovich <slyfox@gentoo.org> >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 <slyfox@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(-) > 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 <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) >-- >2.20.1 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
Attachments on
bug 669702
:
553230
|
553232
| 560120