From ae9d6d73f50f6205f0651a627d3bf7b0d99273f1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 28 Mar 2014 01:52:08 -0400 Subject: [PATCH 01/22] Fix some initial sandbox issues. Allow files that weren't in the list; Allow the _sysctl syscall; allow accept4 with CLOEXEC and NONBLOCK. --- src/common/sandbox.c | 6 ++++++ src/or/main.c | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 0548f3edd4..299c6f20bd 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -124,6 +124,7 @@ static int filter_nopar_gen[] = { SCMP_SYS(rename), SCMP_SYS(rt_sigreturn), SCMP_SYS(set_robust_list), + SCMP_SYS(_sysctl), #ifdef __NR_sigreturn SCMP_SYS(sigreturn), #endif @@ -249,6 +250,11 @@ sb_accept4(scmp_filter_ctx ctx, sandbox_cfg_t *filter) if (rc) { return rc; } + rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(accept4), 1, + SCMP_CMP(3, SCMP_CMP_EQ, SOCK_CLOEXEC|SOCK_NONBLOCK)); + if (rc) { + return rc; + } return 0; } diff --git a/src/or/main.c b/src/or/main.c index 0264064edc..16149544bf 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2732,8 +2732,11 @@ sandbox_init_filter(void) get_datadir_fname("cached-certs"), 1, get_datadir_fname("cached-certs.tmp"), 1, get_datadir_fname("cached-consensus"), 1, + get_datadir_fname("cached-consensus.tmp"), 1, get_datadir_fname("unverified-consensus"), 1, get_datadir_fname("unverified-consensus.tmp"), 1, + get_datadir_fname("unverified-microdesc-consensus"), 1, + get_datadir_fname("unverified-microdesc-consensus.tmp"), 1, get_datadir_fname("cached-microdesc-consensus"), 1, get_datadir_fname("cached-microdesc-consensus.tmp"), 1, get_datadir_fname("cached-microdescs"), 1, @@ -2747,9 +2750,15 @@ sandbox_init_filter(void) get_datadir_fname("cached-descriptors.new.tmp"), 1, get_datadir_fname("cached-descriptors.tmp.tmp"), 1, get_datadir_fname("cached-extrainfo"), 1, + get_datadir_fname("cached-extrainfo.new"), 1, + get_datadir_fname("cached-extrainfo.tmp"), 1, + get_datadir_fname("cached-extrainfo.new.tmp"), 1, + get_datadir_fname("cached-extrainfo.tmp.tmp"), 1, get_datadir_fname("state.tmp"), 1, get_datadir_fname("unparseable-desc.tmp"), 1, get_datadir_fname("unparseable-desc"), 1, + get_datadir_fname("v3-status-votes"), 1, + get_datadir_fname("v3-status-votes.tmp"), 1, "/dev/srandom", 0, "/dev/urandom", 0, "/dev/random", 0, From 3802e32c7d94c599546069d8246636b0d3a4ad10 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 28 Mar 2014 02:10:19 -0400 Subject: [PATCH 02/22] Only intern one copy of each magic string for the sandbox If we intern two copies of a string, later calls to sandbox_intern_string will give the wrong one sometimes. --- src/common/sandbox.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 299c6f20bd..363333a038 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -27,6 +27,7 @@ #include #include "sandbox.h" +#include "container.h" #include "torlog.h" #include "torint.h" #include "util.h" @@ -859,8 +860,9 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) size_t pr_mem_size = 0, pr_mem_left = 0; char *pr_mem_next = NULL, *pr_mem_base; sandbox_cfg_t *el = NULL; + strmap_t *locations = NULL; - // get total number of bytes required to mmap + // get total number of bytes required to mmap. (Overestimate.) for (el = cfg; el != NULL; el = el->next) { pr_mem_size += strlen((char*) ((smp_param_t*)el->param)->value) + 1; } @@ -878,12 +880,24 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) pr_mem_next = pr_mem_base + MALLOC_MP_LIM; pr_mem_left = pr_mem_size; + locations = strmap_new(); + // change el value pointer to protected for (el = cfg; el != NULL; el = el->next) { char *param_val = (char*)((smp_param_t *)el->param)->value; size_t param_size = strlen(param_val) + 1; - if (pr_mem_left >= param_size) { + void *location = strmap_get(locations, param_val); + if (location) { + // We already interned this string. + { + void *old_val = (void *) ((smp_param_t*)el->param)->value; + tor_free(old_val); + } + ((smp_param_t*)el->param)->value = (intptr_t) location; + ((smp_param_t*)el->param)->prot = 1; + + } else if (pr_mem_left >= param_size) { // copy to protected memcpy(pr_mem_next, param_val, param_size); @@ -895,6 +909,8 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) ((smp_param_t*)el->param)->value = (intptr_t) pr_mem_next; ((smp_param_t*)el->param)->prot = 1; + strmap_set(locations, pr_mem_next, pr_mem_next); + // move next available protected memory pr_mem_next += param_size; pr_mem_left -= param_size; @@ -962,7 +978,8 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) } out: - return ret; + strmap_free(locations, NULL); + return ret; } /** From cbfb8e703ed9c7e31848ebf959ac7a4cf27b4a64 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 28 Mar 2014 03:51:50 -0400 Subject: [PATCH 03/22] Add 'rename' to the sandboxed syscalls (If we don't restrict rename, there's not much point in restricting open, since an attacker could always use rename to make us open whatever they want.) --- src/common/compat.c | 12 +++++-- src/common/compat.h | 1 + src/common/sandbox.c | 83 +++++++++++++++++++++++++++++++++++++++++--- src/common/sandbox.h | 7 ++++ src/or/config.c | 2 +- src/or/main.c | 39 +++++++++++++++++++-- src/or/statefile.c | 3 +- 7 files changed, 136 insertions(+), 11 deletions(-) diff --git a/src/common/compat.c b/src/common/compat.c index 04c9d59235..5c18535285 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -175,6 +175,14 @@ tor_fopen_cloexec(const char *path, const char *mode) return result; } +/** As rename(), but work correctly with the sandbox. */ +int +tor_rename(const char *path_old, const char *path_new) +{ + return rename(sandbox_intern_string(path_old), + sandbox_intern_string(path_new)); +} + #if defined(HAVE_SYS_MMAN_H) || defined(RUNNING_DOXYGEN) /** Try to create a memory mapping for filename and return it. On * failure, return NULL. Sets errno properly, using ERANGE to mean @@ -799,7 +807,7 @@ int replace_file(const char *from, const char *to) { #ifndef _WIN32 - return rename(from,to); + return tor_rename(from, to); #else switch (file_status(to)) { @@ -814,7 +822,7 @@ replace_file(const char *from, const char *to) errno = EISDIR; return -1; } - return rename(from,to); + return tor_rename(from,to); #endif } diff --git a/src/common/compat.h b/src/common/compat.h index bb88818d82..9a381fb97f 100644 --- a/src/common/compat.h +++ b/src/common/compat.h @@ -410,6 +410,7 @@ struct tm *tor_gmtime_r(const time_t *timep, struct tm *result); /* ===== File compatibility */ int tor_open_cloexec(const char *path, int flags, unsigned mode); FILE *tor_fopen_cloexec(const char *path, const char *mode); +int tor_rename(const char *path_old, const char *path_new); int replace_file(const char *from, const char *to); int touch_file(const char *fname); diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 363333a038..c2f482d0c7 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -122,7 +122,6 @@ static int filter_nopar_gen[] = { SCMP_SYS(mmap), SCMP_SYS(munmap), SCMP_SYS(read), - SCMP_SYS(rename), SCMP_SYS(rt_sigreturn), SCMP_SYS(set_robust_list), SCMP_SYS(_sysctl), @@ -361,6 +360,41 @@ sb_open(scmp_filter_ctx ctx, sandbox_cfg_t *filter) return 0; } +/** + * Function responsible for setting up the rename syscall for + * the seccomp filter sandbox. + */ +static int +sb_rename(scmp_filter_ctx ctx, sandbox_cfg_t *filter) +{ + int rc; + sandbox_cfg_t *elem = NULL; + + // for each dynamic parameter filters + for (elem = filter; elem != NULL; elem = elem->next) { + smp_param_t *param = elem->param; + + if (param != NULL && param->prot == 1 && + param->syscall == SCMP_SYS(rename)) { + + intptr_t value2 = (intptr_t)(void*)sandbox_intern_string( + (char*)param->value2); + + rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, + SCMP_SYS(rename), 1, + SCMP_CMP(0, SCMP_CMP_EQ, param->value), + SCMP_CMP(1, SCMP_CMP_EQ, value2)); + if (rc != 0) { + log_err(LD_BUG,"(Sandbox) failed to add rename syscall, received " + "libseccomp error %d", rc); + return rc; + } + } + } + + return 0; +} + /** * Function responsible for setting up the openat syscall for * the seccomp filter sandbox. @@ -807,6 +841,7 @@ static sandbox_filter_func_t filter_func[] = { #endif sb_open, sb_openat, + sb_rename, #ifdef __NR_fcntl64 sb_fcntl64, #endif @@ -888,6 +923,7 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) size_t param_size = strlen(param_val) + 1; void *location = strmap_get(locations, param_val); + if (location) { // We already interned this string. { @@ -989,22 +1025,30 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) * point. */ static sandbox_cfg_t* -new_element(int syscall, int index, intptr_t value) +new_element2(int syscall, int index, int index2, intptr_t value, intptr_t value2) { smp_param_t *param = NULL; - sandbox_cfg_t *elem = tor_malloc(sizeof(sandbox_cfg_t)); - elem->param = tor_malloc(sizeof(smp_param_t)); + sandbox_cfg_t *elem = tor_malloc_zero(sizeof(sandbox_cfg_t)); + elem->param = tor_malloc_zero(sizeof(smp_param_t)); param = elem->param; param->syscall = syscall; param->pindex = index; + param->pindex2 = index2; param->value = value; + param->value2 = value2; param->prot = 0; return elem; } +static sandbox_cfg_t* +new_element(int syscall, int index, intptr_t value) +{ + return new_element2(syscall, index, -1, value, 0); +} + #ifdef __NR_stat64 #define SCMP_stat SCMP_SYS(stat64) #else @@ -1072,6 +1116,37 @@ sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file, int fr) return 0; } +int +sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) +{ + sandbox_cfg_t *elem = NULL; + + elem = new_element2(SCMP_SYS(rename), 0, 1, + (intptr_t)(void *)tor_strdup(file1), + (intptr_t)(void *)tor_strdup(file2)); + + if (!elem) { + log_err(LD_BUG,"(Sandbox) failed to register parameter!"); + return -1; + } + + elem->next = *cfg; + *cfg = elem; + + /* For interning */ + elem = new_element(-1, 0, (intptr_t)(void*)tor_strdup(file2)); + if (!elem) { + log_err(LD_BUG,"(Sandbox) failed to register parameter!"); + return -1; + } + elem->next = *cfg; + *cfg = elem; + + tor_free(file1); + tor_free(file2); + return 0; +} + int sandbox_cfg_allow_open_filename_array(sandbox_cfg_t **cfg, ...) { diff --git a/src/common/sandbox.h b/src/common/sandbox.h index d64d427d3e..b15f31cc41 100644 --- a/src/common/sandbox.h +++ b/src/common/sandbox.h @@ -67,8 +67,12 @@ typedef struct smp_param { /** parameter index. */ int pindex; + /** parameter index, second one. */ + int pindex2; /** parameter value. */ intptr_t value; + /** parameter value, second argument. */ + intptr_t value2; /** parameter flag (0 = not protected, 1 = protected). */ int prot; @@ -174,6 +178,9 @@ sandbox_cfg_t * sandbox_cfg_new(void); int sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file, int fr); +/**DOCDOC*/ +int sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2); + /** Function used to add a series of open allowed filenames to a supplied * configuration. * @param cfg sandbox configuration. diff --git a/src/or/config.c b/src/or/config.c index ca99d014fc..89aedccb4c 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -6276,7 +6276,7 @@ write_configuration_file(const char *fname, const or_options_t *options) ++i; } log_notice(LD_CONFIG, "Renaming old configuration file to \"%s\"", fn_tmp); - if (rename(fname, fn_tmp) < 0) { + if (tor_rename(fname, fn_tmp) < 0) {//XXXX sandbox doesn't allow log_warn(LD_FS, "Couldn't rename configuration file \"%s\" to \"%s\": %s", fname, fn_tmp, strerror(errno)); diff --git a/src/or/main.c b/src/or/main.c index 16149544bf..3c248bb800 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2743,7 +2743,6 @@ sandbox_init_filter(void) get_datadir_fname("cached-microdescs.tmp"), 1, get_datadir_fname("cached-microdescs.new"), 1, get_datadir_fname("cached-microdescs.new.tmp"), 1, - get_datadir_fname("unverified-microdesc-consensus"), 1, get_datadir_fname("cached-descriptors"), 1, get_datadir_fname("cached-descriptors.new"), 1, get_datadir_fname("cached-descriptors.tmp"), 1, @@ -2765,6 +2764,34 @@ sandbox_init_filter(void) NULL, 0 ); +#define RENAME_SUFFIX(name, suffix) \ + sandbox_cfg_allow_rename(&cfg, \ + get_datadir_fname(name suffix), \ + get_datadir_fname(name)) + +#define RENAME_SUFFIX2(prefix, name, suffix) \ + sandbox_cfg_allow_rename(&cfg, \ + get_datadir_fname2(prefix, name suffix), \ + get_datadir_fname2(prefix, name)) + + RENAME_SUFFIX("cached-certs", ".tmp"); + RENAME_SUFFIX("cached-consensus", ".tmp"); + RENAME_SUFFIX("unverified-consensus", ".tmp"); + RENAME_SUFFIX("unverified-microdesc-consensus", ".tmp"); + RENAME_SUFFIX("cached-microdesc-consensus", ".tmp"); + RENAME_SUFFIX("cached-microdescs", ".tmp"); + RENAME_SUFFIX("cached-microdescs", ".new"); + RENAME_SUFFIX("cached-microdescs.new", ".tmp"); + RENAME_SUFFIX("cached-descriptors", ".tmp"); + RENAME_SUFFIX("cached-descriptors", ".new"); + RENAME_SUFFIX("cached-descriptors.new", ".tmp"); + RENAME_SUFFIX("cached-extrainfo", ".tmp"); + RENAME_SUFFIX("cached-extrainfo", ".new"); + RENAME_SUFFIX("cached-extrainfo.new", ".tmp"); + RENAME_SUFFIX("state", ".tmp"); + RENAME_SUFFIX("unparseable-desc", ".tmp"); + RENAME_SUFFIX("v3-status-votes", ".tmp"); + sandbox_cfg_allow_stat_filename_array(&cfg, get_datadir_fname(NULL), 1, get_datadir_fname("lock"), 1, @@ -2790,12 +2817,18 @@ sandbox_init_filter(void) get_datadir_fname("fingerprint.tmp"), 1, get_datadir_fname("hashed-fingerprint"), 1, get_datadir_fname("hashed-fingerprint.tmp"), 1, - get_datadir_fname("cached-consensus"), 1, - get_datadir_fname("cached-consensus.tmp"), 1, "/etc/resolv.conf", 0, NULL, 0 ); + RENAME_SUFFIX("fingerprint", ".tmp"); + RENAME_SUFFIX2("keys", "secret_onion_key_ntor", ".tmp"); + RENAME_SUFFIX2("keys", "secret_id_key", ".tmp"); + RENAME_SUFFIX2("keys", "secret_id_key.old", ".tmp"); + RENAME_SUFFIX2("keys", "secret_onion_key", ".tmp"); + RENAME_SUFFIX2("keys", "secret_onion_key.old", ".tmp"); + RENAME_SUFFIX("hashed-fingerprint", ".tmp"); + sandbox_cfg_allow_stat_filename_array(&cfg, get_datadir_fname("keys"), 1, get_datadir_fname("stats/dirreq-stats"), 1, diff --git a/src/or/statefile.c b/src/or/statefile.c index 2251f25e94..da31341712 100644 --- a/src/or/statefile.c +++ b/src/or/statefile.c @@ -13,6 +13,7 @@ #include "hibernate.h" #include "rephist.h" #include "router.h" +#include "sandbox.h" #include "statefile.h" /** A list of state-file "abbreviations," for compatibility. */ @@ -285,7 +286,7 @@ or_state_save_broken(char *fname) log_warn(LD_BUG, "Unable to parse state in \"%s\". Moving it aside " "to \"%s\". This could be a bug in Tor; please tell " "the developers.", fname, fname2); - if (rename(fname, fname2) < 0) { + if (tor_rename(fname, fname2) < 0) {//XXXX sandbox prohibits log_warn(LD_BUG, "Weirdly, I couldn't even move the state aside. The " "OS gave an error of %s", strerror(errno)); } From e051e192a8b199c20ece2a4205c9642a4a0cee22 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 11 Apr 2014 02:40:15 -0400 Subject: [PATCH 04/22] Remove nonsensical exec permission from sandbox code. --- src/or/main.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 3c248bb800..e6c163609e 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2836,8 +2836,6 @@ sandbox_init_filter(void) ); } - sandbox_cfg_allow_execve(&cfg, "/usr/local/bin/tor"); - init_addrinfo(); return cfg; From 71eaebd971f4d42b26fb6b85780163bbc0111aae Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 11 Apr 2014 03:04:16 -0400 Subject: [PATCH 05/22] Drop 'fr' parameter from sandbox code. Appearently, the majority of the filenames we pass to sandbox_cfg_allow() functions are "freeable right after". So, consider _all_ of them safe-to-steal, and add a tor_strdup() in the few cases that aren't. (Maybe buggy; revise when I can test.) --- src/common/sandbox.c | 55 ++++++++-------------- src/common/sandbox.h | 53 ++++++++------------- src/or/main.c | 108 +++++++++++++++++++++---------------------- 3 files changed, 92 insertions(+), 124 deletions(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index c2f482d0c7..b97b900c84 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -1056,11 +1056,11 @@ new_element(int syscall, int index, intptr_t value) #endif int -sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file, int fr) +sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file) { sandbox_cfg_t *elem = NULL; - elem = new_element(SCMP_stat, 0, (intptr_t)(void*) tor_strdup(file)); + elem = new_element(SCMP_stat, 0, (intptr_t)(void*) file); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1069,7 +1069,6 @@ sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file, int fr) elem->next = *cfg; *cfg = elem; - if (fr) tor_free(file); return 0; } @@ -1083,9 +1082,7 @@ sandbox_cfg_allow_stat_filename_array(sandbox_cfg_t **cfg, ...) va_start(ap, cfg); while ((fn = va_arg(ap, char*)) != NULL) { - int fr = va_arg(ap, int); - - rc = sandbox_cfg_allow_stat_filename(cfg, fn, fr); + rc = sandbox_cfg_allow_stat_filename(cfg, fn); if (rc) { log_err(LD_BUG,"(Sandbox) sandbox_cfg_allow_stat_filename_array fail"); goto end; @@ -1098,11 +1095,11 @@ sandbox_cfg_allow_stat_filename_array(sandbox_cfg_t **cfg, ...) } int -sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file, int fr) +sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file) { sandbox_cfg_t *elem = NULL; - elem = new_element(SCMP_SYS(open), 0, (intptr_t)(void *)tor_strdup(file)); + elem = new_element(SCMP_SYS(open), 0, (intptr_t)(void *) file); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1111,8 +1108,6 @@ sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file, int fr) elem->next = *cfg; *cfg = elem; - if (fr) tor_free(file); - return 0; } @@ -1122,8 +1117,8 @@ sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) sandbox_cfg_t *elem = NULL; elem = new_element2(SCMP_SYS(rename), 0, 1, - (intptr_t)(void *)tor_strdup(file1), - (intptr_t)(void *)tor_strdup(file2)); + (intptr_t)(void *) file1, + (intptr_t)(void *) file2); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); @@ -1142,8 +1137,6 @@ sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) elem->next = *cfg; *cfg = elem; - tor_free(file1); - tor_free(file2); return 0; } @@ -1157,9 +1150,7 @@ sandbox_cfg_allow_open_filename_array(sandbox_cfg_t **cfg, ...) va_start(ap, cfg); while ((fn = va_arg(ap, char*)) != NULL) { - int fr = va_arg(ap, int); - - rc = sandbox_cfg_allow_open_filename(cfg, fn, fr); + rc = sandbox_cfg_allow_open_filename(cfg, fn); if (rc) { log_err(LD_BUG,"(Sandbox) sandbox_cfg_allow_open_filename_array fail"); goto end; @@ -1172,11 +1163,11 @@ sandbox_cfg_allow_open_filename_array(sandbox_cfg_t **cfg, ...) } int -sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file, int fr) +sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file) { sandbox_cfg_t *elem = NULL; - elem = new_element(SCMP_SYS(openat), 1, (intptr_t)(void *)tor_strdup(file)); + elem = new_element(SCMP_SYS(openat), 1, (intptr_t)(void *) file); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1185,8 +1176,6 @@ sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file, int fr) elem->next = *cfg; *cfg = elem; - if (fr) tor_free(file); - return 0; } @@ -1200,9 +1189,7 @@ sandbox_cfg_allow_openat_filename_array(sandbox_cfg_t **cfg, ...) va_start(ap, cfg); while ((fn = va_arg(ap, char*)) != NULL) { - int fr = va_arg(ap, int); - - rc = sandbox_cfg_allow_openat_filename(cfg, fn, fr); + rc = sandbox_cfg_allow_openat_filename(cfg, fn); if (rc) { log_err(LD_BUG,"(Sandbox) sandbox_cfg_allow_openat_filename_array fail"); goto end; @@ -1219,7 +1206,7 @@ sandbox_cfg_allow_execve(sandbox_cfg_t **cfg, const char *com) { sandbox_cfg_t *elem = NULL; - elem = new_element(SCMP_SYS(execve), 1, (intptr_t)(void *)tor_strdup(com)); + elem = new_element(SCMP_SYS(execve), 1, (intptr_t)(void *) com); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1519,7 +1506,8 @@ register_cfg(sandbox_cfg_t* cfg) return 0; } - for (elem = filter_dynamic; elem->next != NULL; elem = elem->next); + for (elem = filter_dynamic; elem->next != NULL; elem = elem->next) + ; elem->next = cfg; @@ -1583,10 +1571,9 @@ sandbox_init(sandbox_cfg_t *cfg) #ifndef USE_LIBSECCOMP int -sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file, - int fr) +sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file) { - (void)cfg; (void)file; (void)fr; + (void)cfg; (void)file; return 0; } @@ -1598,10 +1585,9 @@ sandbox_cfg_allow_open_filename_array(sandbox_cfg_t **cfg, ...) } int -sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file, - int fr) +sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file) { - (void)cfg; (void)file; (void)fr; + (void)cfg; (void)file; return 0; } @@ -1627,10 +1613,9 @@ sandbox_cfg_allow_execve_array(sandbox_cfg_t **cfg, ...) } int -sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file, - int fr) +sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file) { - (void)cfg; (void)file; (void)fr; + (void)cfg; (void)file; return 0; } diff --git a/src/common/sandbox.h b/src/common/sandbox.h index b15f31cc41..8fcd221ef5 100644 --- a/src/common/sandbox.h +++ b/src/common/sandbox.h @@ -171,12 +171,10 @@ sandbox_cfg_t * sandbox_cfg_new(void); /** * Function used to add a open allowed filename to a supplied configuration. - * The (char*) specifies the path to the allowed file, fr = 1 tells the - * function that the char* needs to be free-ed, 0 means the pointer does not - * need to be free-ed. + * The (char*) specifies the path to the allowed file; we take ownership + * of the pointer. */ -int sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file, - int fr); +int sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file); /**DOCDOC*/ int sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2); @@ -184,66 +182,51 @@ int sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2); /** Function used to add a series of open allowed filenames to a supplied * configuration. * @param cfg sandbox configuration. - * @param ... all future parameters are specified as pairs of <(char*), 1 / 0> - * the char* specifies the path to the allowed file, 1 tells the function - * that the char* needs to be free-ed, 0 means the pointer does not need to - * be free-ed; the final parameter needs to be . - */ + * @param ... a list of stealable pointers to permitted files. The last + * one must be NULL. +*/ int sandbox_cfg_allow_open_filename_array(sandbox_cfg_t **cfg, ...); /** * Function used to add a openat allowed filename to a supplied configuration. - * The (char*) specifies the path to the allowed file, fr = 1 tells the - * function that the char* needs to be free-ed, 0 means the pointer does not - * need to be free-ed. + * The (char*) specifies the path to the allowed file; we steal the pointer to + * that file. */ -int sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file, - int fr); +int sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file); /** Function used to add a series of openat allowed filenames to a supplied * configuration. * @param cfg sandbox configuration. - * @param ... all future parameters are specified as pairs of <(char*), 1 / 0> - * the char* specifies the path to the allowed file, 1 tells the function - * that the char* needs to be free-ed, 0 means the pointer does not need to - * be free-ed; the final parameter needs to be . + * @param ... a list of stealable pointers to permitted files. The last + * one must be NULL. */ int sandbox_cfg_allow_openat_filename_array(sandbox_cfg_t **cfg, ...); /** * Function used to add a execve allowed filename to a supplied configuration. - * The (char*) specifies the path to the allowed file, fr = 1 tells the - * function that the char* needs to be free-ed, 0 means the pointer does not - * need to be free-ed. + * The (char*) specifies the path to the allowed file; that pointer is stolen. */ int sandbox_cfg_allow_execve(sandbox_cfg_t **cfg, const char *com); /** Function used to add a series of execve allowed filenames to a supplied * configuration. * @param cfg sandbox configuration. - * @param ... all future parameters are specified as pairs of <(char*), 1 / 0> - * the char* specifies the path to the allowed file, 1 tells the function - * that the char* needs to be free-ed, 0 means the pointer does not need to - * be free-ed; the final parameter needs to be . + * @param ... an array of stealable pointers to permitted files. The last + * one must be NULL. */ int sandbox_cfg_allow_execve_array(sandbox_cfg_t **cfg, ...); /** * Function used to add a stat/stat64 allowed filename to a configuration. - * The (char*) specifies the path to the allowed file, fr = 1 tells the - * function that the char* needs to be free-ed, 0 means the pointer does not - * need to be free-ed. + * The (char*) specifies the path to the allowed file; that pointer is stolen. */ -int sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file, - int fr); +int sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file); /** Function used to add a series of stat64 allowed filenames to a supplied * configuration. * @param cfg sandbox configuration. - * @param ... all future parameters are specified as pairs of <(char*), 1 / 0> - * the char* specifies the path to the allowed file, 1 tells the function - * that the char* needs to be free-ed, 0 means the pointer does not need to - * be free-ed; the final parameter needs to be . + * @param ... an array of stealable pointers to permitted files. The last + * one must be NULL. */ int sandbox_cfg_allow_stat_filename_array(sandbox_cfg_t **cfg, ...); diff --git a/src/or/main.c b/src/or/main.c index e6c163609e..341f22adcb 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2726,41 +2726,41 @@ sandbox_init_filter(void) sandbox_cfg_t *cfg = sandbox_cfg_new(); sandbox_cfg_allow_openat_filename(&cfg, - get_datadir_fname("cached-status"), 1); + get_datadir_fname("cached-status")); sandbox_cfg_allow_open_filename_array(&cfg, - get_datadir_fname("cached-certs"), 1, - get_datadir_fname("cached-certs.tmp"), 1, - get_datadir_fname("cached-consensus"), 1, - get_datadir_fname("cached-consensus.tmp"), 1, - get_datadir_fname("unverified-consensus"), 1, - get_datadir_fname("unverified-consensus.tmp"), 1, - get_datadir_fname("unverified-microdesc-consensus"), 1, - get_datadir_fname("unverified-microdesc-consensus.tmp"), 1, - get_datadir_fname("cached-microdesc-consensus"), 1, - get_datadir_fname("cached-microdesc-consensus.tmp"), 1, - get_datadir_fname("cached-microdescs"), 1, - get_datadir_fname("cached-microdescs.tmp"), 1, - get_datadir_fname("cached-microdescs.new"), 1, - get_datadir_fname("cached-microdescs.new.tmp"), 1, - get_datadir_fname("cached-descriptors"), 1, - get_datadir_fname("cached-descriptors.new"), 1, - get_datadir_fname("cached-descriptors.tmp"), 1, - get_datadir_fname("cached-descriptors.new.tmp"), 1, - get_datadir_fname("cached-descriptors.tmp.tmp"), 1, - get_datadir_fname("cached-extrainfo"), 1, - get_datadir_fname("cached-extrainfo.new"), 1, - get_datadir_fname("cached-extrainfo.tmp"), 1, - get_datadir_fname("cached-extrainfo.new.tmp"), 1, - get_datadir_fname("cached-extrainfo.tmp.tmp"), 1, - get_datadir_fname("state.tmp"), 1, - get_datadir_fname("unparseable-desc.tmp"), 1, - get_datadir_fname("unparseable-desc"), 1, - get_datadir_fname("v3-status-votes"), 1, - get_datadir_fname("v3-status-votes.tmp"), 1, - "/dev/srandom", 0, - "/dev/urandom", 0, - "/dev/random", 0, + get_datadir_fname("cached-certs"), + get_datadir_fname("cached-certs.tmp"), + get_datadir_fname("cached-consensus"), + get_datadir_fname("cached-consensus.tmp"), + get_datadir_fname("unverified-consensus"), + get_datadir_fname("unverified-consensus.tmp"), + get_datadir_fname("unverified-microdesc-consensus"), + get_datadir_fname("unverified-microdesc-consensus.tmp"), + get_datadir_fname("cached-microdesc-consensus"), + get_datadir_fname("cached-microdesc-consensus.tmp"), + get_datadir_fname("cached-microdescs"), + get_datadir_fname("cached-microdescs.tmp"), + get_datadir_fname("cached-microdescs.new"), + get_datadir_fname("cached-microdescs.new.tmp"), + get_datadir_fname("cached-descriptors"), + get_datadir_fname("cached-descriptors.new"), + get_datadir_fname("cached-descriptors.tmp"), + get_datadir_fname("cached-descriptors.new.tmp"), + get_datadir_fname("cached-descriptors.tmp.tmp"), + get_datadir_fname("cached-extrainfo"), + get_datadir_fname("cached-extrainfo.new"), + get_datadir_fname("cached-extrainfo.tmp"), + get_datadir_fname("cached-extrainfo.new.tmp"), + get_datadir_fname("cached-extrainfo.tmp.tmp"), + get_datadir_fname("state.tmp"), + get_datadir_fname("unparseable-desc.tmp"), + get_datadir_fname("unparseable-desc"), + get_datadir_fname("v3-status-votes"), + get_datadir_fname("v3-status-votes.tmp"), + tor_strdup("/dev/srandom"), + tor_strdup("/dev/urandom"), + tor_strdup("/dev/random"), NULL, 0 ); @@ -2793,31 +2793,31 @@ sandbox_init_filter(void) RENAME_SUFFIX("v3-status-votes", ".tmp"); sandbox_cfg_allow_stat_filename_array(&cfg, - get_datadir_fname(NULL), 1, - get_datadir_fname("lock"), 1, - get_datadir_fname("state"), 1, - get_datadir_fname("router-stability"), 1, - get_datadir_fname("cached-extrainfo.new"), 1, + get_datadir_fname(NULL), + get_datadir_fname("lock"), + get_datadir_fname("state"), + get_datadir_fname("router-stability"), + get_datadir_fname("cached-extrainfo.new"), NULL, 0 ); // orport if (server_mode(get_options())) { sandbox_cfg_allow_open_filename_array(&cfg, - get_datadir_fname2("keys", "secret_id_key"), 1, - get_datadir_fname2("keys", "secret_onion_key"), 1, - get_datadir_fname2("keys", "secret_onion_key_ntor"), 1, - get_datadir_fname2("keys", "secret_onion_key_ntor.tmp"), 1, - get_datadir_fname2("keys", "secret_id_key.old"), 1, - get_datadir_fname2("keys", "secret_onion_key.old"), 1, - get_datadir_fname2("keys", "secret_onion_key_ntor.old"), 1, - get_datadir_fname2("keys", "secret_onion_key.tmp"), 1, - get_datadir_fname2("keys", "secret_id_key.tmp"), 1, - get_datadir_fname("fingerprint"), 1, - get_datadir_fname("fingerprint.tmp"), 1, - get_datadir_fname("hashed-fingerprint"), 1, - get_datadir_fname("hashed-fingerprint.tmp"), 1, - "/etc/resolv.conf", 0, + get_datadir_fname2("keys", "secret_id_key"), + get_datadir_fname2("keys", "secret_onion_key"), + get_datadir_fname2("keys", "secret_onion_key_ntor"), + get_datadir_fname2("keys", "secret_onion_key_ntor.tmp"), + get_datadir_fname2("keys", "secret_id_key.old"), + get_datadir_fname2("keys", "secret_onion_key.old"), + get_datadir_fname2("keys", "secret_onion_key_ntor.old"), + get_datadir_fname2("keys", "secret_onion_key.tmp"), + get_datadir_fname2("keys", "secret_id_key.tmp"), + get_datadir_fname("fingerprint"), + get_datadir_fname("fingerprint.tmp"), + get_datadir_fname("hashed-fingerprint"), + get_datadir_fname("hashed-fingerprint.tmp"), + tor_strdup("/etc/resolv.conf"), NULL, 0 ); @@ -2830,8 +2830,8 @@ sandbox_init_filter(void) RENAME_SUFFIX("hashed-fingerprint", ".tmp"); sandbox_cfg_allow_stat_filename_array(&cfg, - get_datadir_fname("keys"), 1, - get_datadir_fname("stats/dirreq-stats"), 1, + get_datadir_fname("keys"), + get_datadir_fname("stats/dirreq-stats"), NULL, 0 ); } From 6807b76a5e004a2d6c9f46f8b4dec0c90007996c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 11 Apr 2014 03:06:05 -0400 Subject: [PATCH 06/22] Add missing rename function for non-linux platforms --- src/common/sandbox.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index b97b900c84..01aefe1406 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -1625,5 +1625,12 @@ sandbox_cfg_allow_stat_filename_array(sandbox_cfg_t **cfg, ...) (void)cfg; return 0; } + +int +sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) +{ + (void)cfg; (void)file1; (void)file2; + return 0; +} #endif From f268101a612bd637e270914365271d08fc5813db Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 12:24:08 -0400 Subject: [PATCH 07/22] Clean up sandbox structures a bit Drop pindex,pindex2 as unused. Admit a type to avoid using a void* --- src/common/sandbox.c | 42 ++++++++++++++++++++---------------------- src/common/sandbox.h | 6 +----- 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 01aefe1406..85a2f23c92 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -198,7 +198,7 @@ sb_execve(scmp_filter_ctx ctx, sandbox_cfg_t *filter) // for each dynamic parameter filters for (elem = filter; elem != NULL; elem = elem->next) { - smp_param_t *param = (smp_param_t*) elem->param; + smp_param_t *param = elem->param; if (param != NULL && param->prot == 1 && param->syscall == SCMP_SYS(execve)) { @@ -899,7 +899,8 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) // get total number of bytes required to mmap. (Overestimate.) for (el = cfg; el != NULL; el = el->next) { - pr_mem_size += strlen((char*) ((smp_param_t*)el->param)->value) + 1; + pr_mem_size += strlen((char*) el->param->value) + 1; + } // allocate protected memory with MALLOC_MP_LIM canary @@ -919,7 +920,7 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) // change el value pointer to protected for (el = cfg; el != NULL; el = el->next) { - char *param_val = (char*)((smp_param_t *)el->param)->value; + char *param_val = (char*)(el->param)->value; size_t param_size = strlen(param_val) + 1; void *location = strmap_get(locations, param_val); @@ -927,11 +928,11 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) if (location) { // We already interned this string. { - void *old_val = (void *) ((smp_param_t*)el->param)->value; + void *old_val = (void *) el->param->value; tor_free(old_val); } - ((smp_param_t*)el->param)->value = (intptr_t) location; - ((smp_param_t*)el->param)->prot = 1; + el->param->value = (intptr_t) location; + el->param->prot = 1; } else if (pr_mem_left >= param_size) { // copy to protected @@ -939,11 +940,11 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) // re-point el parameter to protected { - void *old_val = (void *) ((smp_param_t*)el->param)->value; + void *old_val = (void *) el->param->value; tor_free(old_val); } - ((smp_param_t*)el->param)->value = (intptr_t) pr_mem_next; - ((smp_param_t*)el->param)->prot = 1; + el->param->value = (intptr_t) pr_mem_next; + el->param->prot = 1; strmap_set(locations, pr_mem_next, pr_mem_next); @@ -1025,17 +1026,14 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) * point. */ static sandbox_cfg_t* -new_element2(int syscall, int index, int index2, intptr_t value, intptr_t value2) +new_element2(int syscall, intptr_t value, intptr_t value2) { smp_param_t *param = NULL; sandbox_cfg_t *elem = tor_malloc_zero(sizeof(sandbox_cfg_t)); - elem->param = tor_malloc_zero(sizeof(smp_param_t)); + param = elem->param = tor_malloc_zero(sizeof(smp_param_t)); - param = elem->param; param->syscall = syscall; - param->pindex = index; - param->pindex2 = index2; param->value = value; param->value2 = value2; param->prot = 0; @@ -1044,9 +1042,9 @@ new_element2(int syscall, int index, int index2, intptr_t value, intptr_t value2 } static sandbox_cfg_t* -new_element(int syscall, int index, intptr_t value) +new_element(int syscall, intptr_t value) { - return new_element2(syscall, index, -1, value, 0); + return new_element2(syscall, value, 0); } #ifdef __NR_stat64 @@ -1060,7 +1058,7 @@ sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file) { sandbox_cfg_t *elem = NULL; - elem = new_element(SCMP_stat, 0, (intptr_t)(void*) file); + elem = new_element(SCMP_stat, (intptr_t)(void*) file); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1099,7 +1097,7 @@ sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file) { sandbox_cfg_t *elem = NULL; - elem = new_element(SCMP_SYS(open), 0, (intptr_t)(void *) file); + elem = new_element(SCMP_SYS(open), (intptr_t)(void *) file); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1116,7 +1114,7 @@ sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) { sandbox_cfg_t *elem = NULL; - elem = new_element2(SCMP_SYS(rename), 0, 1, + elem = new_element2(SCMP_SYS(rename), (intptr_t)(void *) file1, (intptr_t)(void *) file2); @@ -1129,7 +1127,7 @@ sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) *cfg = elem; /* For interning */ - elem = new_element(-1, 0, (intptr_t)(void*)tor_strdup(file2)); + elem = new_element(-1, (intptr_t)(void*)tor_strdup(file2)); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1167,7 +1165,7 @@ sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file) { sandbox_cfg_t *elem = NULL; - elem = new_element(SCMP_SYS(openat), 1, (intptr_t)(void *) file); + elem = new_element(SCMP_SYS(openat), (intptr_t)(void *) file); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1206,7 +1204,7 @@ sandbox_cfg_allow_execve(sandbox_cfg_t **cfg, const char *com) { sandbox_cfg_t *elem = NULL; - elem = new_element(SCMP_SYS(execve), 1, (intptr_t)(void *) com); + elem = new_element(SCMP_SYS(execve), (intptr_t)(void *) com); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; diff --git a/src/common/sandbox.h b/src/common/sandbox.h index 8fcd221ef5..c4144dbb2e 100644 --- a/src/common/sandbox.h +++ b/src/common/sandbox.h @@ -65,10 +65,6 @@ typedef struct smp_param { /** syscall associated with parameter. */ int syscall; - /** parameter index. */ - int pindex; - /** parameter index, second one. */ - int pindex2; /** parameter value. */ intptr_t value; /** parameter value, second argument. */ @@ -89,7 +85,7 @@ struct sandbox_cfg_elem { SB_IMPL implem; /** Configuration parameter. */ - void *param; + smp_param_t *param; /** Next element of the configuration*/ struct sandbox_cfg_elem *next; From 5aaac938a94fc0b5d7dc63cac92e78d3125f08c8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 12:45:34 -0400 Subject: [PATCH 08/22] Have sandbox string protection include multi-valued parmeters. --- src/common/sandbox.c | 103 +++++++++++++++++++++++++++---------------- 1 file changed, 64 insertions(+), 39 deletions(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 85a2f23c92..8cbcd04fa6 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -862,7 +862,7 @@ static sandbox_filter_func_t filter_func[] = { sb_socketpair }; -const char* +const char * sandbox_intern_string(const char *str) { sandbox_cfg_t *elem; @@ -873,8 +873,13 @@ sandbox_intern_string(const char *str) for (elem = filter_dynamic; elem != NULL; elem = elem->next) { smp_param_t *param = elem->param; - if (param->prot && !strcmp(str, (char*)(param->value))) { - return (char*)(param->value); + if (param->prot) { + if (!strcmp(str, (char*)(param->value))) { + return (char*)param->value; + } + if (param->value2 && !strcmp(str, (char*)param->value2)) { + return (char*)param->value2; + } } } @@ -882,6 +887,50 @@ sandbox_intern_string(const char *str) return str; } +/** DOCDOC */ +static int +prot_strings_helper(strmap_t *locations, + char **pr_mem_next_p, + size_t *pr_mem_left_p, + intptr_t *value_p) +{ + char *param_val; + size_t param_size; + void *location; + + if (*value_p == 0) + return 0; + + param_val = (char*) *value_p; + param_size = strlen(param_val) + 1; + location = strmap_get(locations, param_val); + + if (location) { + // We already interned this string. + tor_free(param_val); + *value_p = (intptr_t) location; + return 0; + } else if (*pr_mem_left_p >= param_size) { + // copy to protected + location = *pr_mem_next_p; + memcpy(location, param_val, param_size); + + // re-point el parameter to protected + tor_free(param_val); + *value_p = (intptr_t) location; + + strmap_set(locations, location, location); /* good real estate advice */ + + // move next available protected memory + *pr_mem_next_p += param_size; + *pr_mem_left_p -= param_size; + return 0; + } else { + log_err(LD_BUG,"(Sandbox) insufficient protected memory!"); + return -1; + } +} + /** * Protects all the strings in the sandbox's parameter list configuration. It * works by calculating the total amount of memory required by the parameter @@ -900,7 +949,8 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) // get total number of bytes required to mmap. (Overestimate.) for (el = cfg; el != NULL; el = el->next) { pr_mem_size += strlen((char*) el->param->value) + 1; - + if (el->param->value2) + pr_mem_size += strlen((char*) el->param->value2) + 1; } // allocate protected memory with MALLOC_MP_LIM canary @@ -920,42 +970,17 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) // change el value pointer to protected for (el = cfg; el != NULL; el = el->next) { - char *param_val = (char*)(el->param)->value; - size_t param_size = strlen(param_val) + 1; - - void *location = strmap_get(locations, param_val); - - if (location) { - // We already interned this string. - { - void *old_val = (void *) el->param->value; - tor_free(old_val); - } - el->param->value = (intptr_t) location; - el->param->prot = 1; - - } else if (pr_mem_left >= param_size) { - // copy to protected - memcpy(pr_mem_next, param_val, param_size); - - // re-point el parameter to protected - { - void *old_val = (void *) el->param->value; - tor_free(old_val); - } - el->param->value = (intptr_t) pr_mem_next; - el->param->prot = 1; - - strmap_set(locations, pr_mem_next, pr_mem_next); - - // move next available protected memory - pr_mem_next += param_size; - pr_mem_left -= param_size; - } else { - log_err(LD_BUG,"(Sandbox) insufficient protected memory!"); + if (prot_strings_helper(locations, &pr_mem_next, &pr_mem_left, + &el->param->value) < 0) { ret = -2; goto out; } + if (prot_strings_helper(locations, &pr_mem_next, &pr_mem_left, + &el->param->value2) < 0) { + ret = -2; + goto out; + } + el->param->prot = 1; } // protecting from writes @@ -995,7 +1020,7 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) * There is a restriction on how much you can mprotect with R|W up to the * size of the canary. */ - ret = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), 2, + ret = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), 3, SCMP_CMP(0, SCMP_CMP_LT, (intptr_t) pr_mem_base), SCMP_CMP(1, SCMP_CMP_LE, MALLOC_MP_LIM), SCMP_CMP(2, SCMP_CMP_EQ, PROT_READ|PROT_WRITE)); @@ -1004,7 +1029,7 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) return ret; } - ret = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), 2, + ret = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), 3, SCMP_CMP(0, SCMP_CMP_GT, (intptr_t) pr_mem_base + pr_mem_size + MALLOC_MP_LIM), SCMP_CMP(1, SCMP_CMP_LE, MALLOC_MP_LIM), From 739a52592bdb771d7ba4e40b6c9df84ea539f7fd Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 12:48:56 -0400 Subject: [PATCH 09/22] Upgrade warning about missing interned string for sandbox --- src/common/sandbox.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 8cbcd04fa6..73966de6e2 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -883,7 +883,8 @@ sandbox_intern_string(const char *str) } } - log_info(LD_GENERAL, "(Sandbox) Parameter %s not found", str); + if (sandbox_active) + log_warn(LD_BUG, "No interned sandbox parameter found for %s", str); return str; } From 12028c29e6ee8d0d9c02b32f1a52a35138e148e3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 12:50:24 -0400 Subject: [PATCH 10/22] Fix sandbox protection for rename (We were only checking the first parameter of each rename call.) --- src/common/sandbox.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 73966de6e2..d50e07494d 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -377,13 +377,10 @@ sb_rename(scmp_filter_ctx ctx, sandbox_cfg_t *filter) if (param != NULL && param->prot == 1 && param->syscall == SCMP_SYS(rename)) { - intptr_t value2 = (intptr_t)(void*)sandbox_intern_string( - (char*)param->value2); - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, - SCMP_SYS(rename), 1, + SCMP_SYS(rename), 2, SCMP_CMP(0, SCMP_CMP_EQ, param->value), - SCMP_CMP(1, SCMP_CMP_EQ, value2)); + SCMP_CMP(1, SCMP_CMP_EQ, param->value2)); if (rc != 0) { log_err(LD_BUG,"(Sandbox) failed to add rename syscall, received " "libseccomp error %d", rc); @@ -1152,15 +1149,6 @@ sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) elem->next = *cfg; *cfg = elem; - /* For interning */ - elem = new_element(-1, (intptr_t)(void*)tor_strdup(file2)); - if (!elem) { - log_err(LD_BUG,"(Sandbox) failed to register parameter!"); - return -1; - } - elem->next = *cfg; - *cfg = elem; - return 0; } From 8dc6755f6d65d7ff847bd5e8cf681e6de7fabbc5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 12:59:33 -0400 Subject: [PATCH 11/22] Introduce arg-counting macros to wrap seccomp_rule_add() The compiler doesn't warn about this code: rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(openat), 1, SCMP_CMP(0, SCMP_CMP_EQ, AT_FDCWD), SCMP_CMP(1, SCMP_CMP_EQ, param->value), SCMP_CMP(2, SCMP_CMP_EQ, O_RDONLY|...)); but note that the arg_cnt argument above is only 1. This means that only the first filter (argument 0 == AT_FDCWD) is actually checked! This patch also fixes the above error in the openat() filter. Earlier I fixed corresponding errors in filters for rename() and mprotect(). --- src/common/sandbox.c | 124 ++++++++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 56 deletions(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index d50e07494d..2a1ddd13d1 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -160,6 +160,19 @@ static int filter_nopar_gen[] = { SCMP_SYS(unlink) }; +/* These macros help avoid the error where the number of filters we add on a + * single rule don't match the arg_cnt param. */ +#define seccomp_rule_add_0(ctx,act,call) \ + seccomp_rule_add((ctx),(act),(call),0) +#define seccomp_rule_add_1(ctx,act,call,f1) \ + seccomp_rule_add((ctx),(act),(call),1,(f1)) +#define seccomp_rule_add_2(ctx,act,call,f1,f2) \ + seccomp_rule_add((ctx),(act),(call),2,(f1),(f2)) +#define seccomp_rule_add_3(ctx,act,call,f1,f2,f3) \ + seccomp_rule_add((ctx),(act),(call),3,(f1),(f2),(f3)) +#define seccomp_rule_add_4(ctx,act,call,f1,f2,f3,f4) \ + seccomp_rule_add((ctx),(act),(call),4,(f1),(f2),(f3),(f4)) + /** * Function responsible for setting up the rt_sigaction syscall for * the seccomp filter sandbox. @@ -177,7 +190,7 @@ sb_rt_sigaction(scmp_filter_ctx ctx, sandbox_cfg_t *filter) (void) filter; for (i = 0; i < ARRAY_LENGTH(param); i++) { - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(rt_sigaction), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(rt_sigaction), SCMP_CMP(0, SCMP_CMP_EQ, param[i])); if (rc) break; @@ -202,7 +215,7 @@ sb_execve(scmp_filter_ctx ctx, sandbox_cfg_t *filter) if (param != NULL && param->prot == 1 && param->syscall == SCMP_SYS(execve)) { - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(execve), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(execve), SCMP_CMP(0, SCMP_CMP_EQ, param->value)); if (rc != 0) { log_err(LD_BUG,"(Sandbox) failed to add execve syscall, received " @@ -223,7 +236,7 @@ static int sb_time(scmp_filter_ctx ctx, sandbox_cfg_t *filter) { (void) filter; - return seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(time), 1, + return seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(time), SCMP_CMP(0, SCMP_CMP_EQ, 0)); } @@ -238,19 +251,19 @@ sb_accept4(scmp_filter_ctx ctx, sandbox_cfg_t *filter) (void)filter; #ifdef __i386__ - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socketcall), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socketcall), SCMP_CMP(0, SCMP_CMP_EQ, 18)); if (rc) { return rc; } #endif - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(accept4), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(accept4), SCMP_CMP(3, SCMP_CMP_EQ, SOCK_CLOEXEC)); if (rc) { return rc; } - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(accept4), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(accept4), SCMP_CMP(3, SCMP_CMP_EQ, SOCK_CLOEXEC|SOCK_NONBLOCK)); if (rc) { return rc; @@ -270,49 +283,49 @@ sb_mmap2(scmp_filter_ctx ctx, sandbox_cfg_t *filter) int rc = 0; (void)filter; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), SCMP_CMP(2, SCMP_CMP_EQ, PROT_READ), SCMP_CMP(3, SCMP_CMP_EQ, MAP_PRIVATE)); if (rc) { return rc; } - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), SCMP_CMP(2, SCMP_CMP_EQ, PROT_NONE), SCMP_CMP(3, SCMP_CMP_EQ, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE)); if (rc) { return rc; } - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), SCMP_CMP(2, SCMP_CMP_EQ, PROT_READ|PROT_WRITE), SCMP_CMP(3, SCMP_CMP_EQ, MAP_PRIVATE|MAP_ANONYMOUS)); if (rc) { return rc; } - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), SCMP_CMP(2, SCMP_CMP_EQ, PROT_READ|PROT_WRITE), SCMP_CMP(3, SCMP_CMP_EQ,MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK)); if (rc) { return rc; } - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), SCMP_CMP(2, SCMP_CMP_EQ, PROT_READ|PROT_WRITE), SCMP_CMP(3, SCMP_CMP_EQ, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE)); if (rc) { return rc; } - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), SCMP_CMP(2, SCMP_CMP_EQ, PROT_READ|PROT_WRITE), SCMP_CMP(3, SCMP_CMP_EQ, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS)); if (rc) { return rc; } - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mmap2), SCMP_CMP(2, SCMP_CMP_EQ, PROT_READ|PROT_EXEC), SCMP_CMP(3, SCMP_CMP_EQ, MAP_PRIVATE|MAP_DENYWRITE)); if (rc) { @@ -339,7 +352,7 @@ sb_open(scmp_filter_ctx ctx, sandbox_cfg_t *filter) if (param != NULL && param->prot == 1 && param->syscall == SCMP_SYS(open)) { - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(open), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(open), SCMP_CMP(0, SCMP_CMP_EQ, param->value)); if (rc != 0) { log_err(LD_BUG,"(Sandbox) failed to add open syscall, received " @@ -349,7 +362,7 @@ sb_open(scmp_filter_ctx ctx, sandbox_cfg_t *filter) } } - rc = seccomp_rule_add(ctx, SCMP_ACT_ERRNO(-1), SCMP_SYS(open), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ERRNO(-1), SCMP_SYS(open), SCMP_CMP(1, SCMP_CMP_EQ, O_RDONLY|O_CLOEXEC)); if (rc != 0) { log_err(LD_BUG,"(Sandbox) failed to add open syscall, received libseccomp " @@ -377,8 +390,7 @@ sb_rename(scmp_filter_ctx ctx, sandbox_cfg_t *filter) if (param != NULL && param->prot == 1 && param->syscall == SCMP_SYS(rename)) { - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, - SCMP_SYS(rename), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(rename), SCMP_CMP(0, SCMP_CMP_EQ, param->value), SCMP_CMP(1, SCMP_CMP_EQ, param->value2)); if (rc != 0) { @@ -408,7 +420,7 @@ sb_openat(scmp_filter_ctx ctx, sandbox_cfg_t *filter) if (param != NULL && param->prot == 1 && param->syscall == SCMP_SYS(openat)) { - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(openat), 1, + rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(openat), SCMP_CMP(0, SCMP_CMP_EQ, AT_FDCWD), SCMP_CMP(1, SCMP_CMP_EQ, param->value), SCMP_CMP(2, SCMP_CMP_EQ, O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_DIRECTORY| @@ -435,40 +447,40 @@ sb_socket(scmp_filter_ctx ctx, sandbox_cfg_t *filter) (void) filter; #ifdef __i386__ - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), 0); + rc = seccomp_rule_add_0(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket)); if (rc) return rc; #endif - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), 3, + rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), SCMP_CMP(0, SCMP_CMP_EQ, PF_FILE), SCMP_CMP(1, SCMP_CMP_EQ, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK), SCMP_CMP(2, SCMP_CMP_EQ, IPPROTO_IP)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), 3, + rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), SCMP_CMP(0, SCMP_CMP_EQ, PF_INET), SCMP_CMP(1, SCMP_CMP_EQ, SOCK_STREAM|SOCK_CLOEXEC), SCMP_CMP(2, SCMP_CMP_EQ, IPPROTO_TCP)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), 3, + rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), SCMP_CMP(0, SCMP_CMP_EQ, PF_INET), SCMP_CMP(1, SCMP_CMP_EQ, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK), SCMP_CMP(2, SCMP_CMP_EQ, IPPROTO_TCP)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), 3, + rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), SCMP_CMP(0, SCMP_CMP_EQ, PF_INET), SCMP_CMP(1, SCMP_CMP_EQ, SOCK_DGRAM|SOCK_CLOEXEC|SOCK_NONBLOCK), SCMP_CMP(2, SCMP_CMP_EQ, IPPROTO_IP)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), 3, + rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), SCMP_CMP(0, SCMP_CMP_EQ, PF_NETLINK), SCMP_CMP(1, SCMP_CMP_EQ, SOCK_RAW), SCMP_CMP(2, SCMP_CMP_EQ, 0)); @@ -489,12 +501,12 @@ sb_socketpair(scmp_filter_ctx ctx, sandbox_cfg_t *filter) (void) filter; #ifdef __i386__ - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socketpair), 0); + rc = seccomp_rule_add_0(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socketpair)); if (rc) return rc; #endif - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socketpair), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socketpair), SCMP_CMP(0, SCMP_CMP_EQ, PF_FILE), SCMP_CMP(1, SCMP_CMP_EQ, SOCK_STREAM|SOCK_CLOEXEC)); if (rc) @@ -514,19 +526,19 @@ sb_setsockopt(scmp_filter_ctx ctx, sandbox_cfg_t *filter) (void) filter; #ifdef __i386__ - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(setsockopt), 0); + rc = seccomp_rule_add_0(ctx, SCMP_ACT_ALLOW, SCMP_SYS(setsockopt)); if (rc) return rc; #endif - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(setsockopt), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(setsockopt), SCMP_CMP(1, SCMP_CMP_EQ, SOL_SOCKET), SCMP_CMP(2, SCMP_CMP_EQ, SO_REUSEADDR)); if (rc) return rc; #ifdef IP_TRANSPARENT - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(setsockopt), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(setsockopt), SCMP_CMP(1, SCMP_CMP_EQ, SOL_IP), SCMP_CMP(2, SCMP_CMP_EQ, IP_TRANSPARENT)); if (rc) @@ -547,12 +559,12 @@ sb_getsockopt(scmp_filter_ctx ctx, sandbox_cfg_t *filter) (void) filter; #ifdef __i386__ - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(getsockopt), 0); + rc = seccomp_rule_add_0(ctx, SCMP_ACT_ALLOW, SCMP_SYS(getsockopt)); if (rc) return rc; #endif - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(getsockopt), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(getsockopt), SCMP_CMP(1, SCMP_CMP_EQ, SOL_SOCKET), SCMP_CMP(2, SCMP_CMP_EQ, SO_ERROR)); if (rc) @@ -572,23 +584,23 @@ sb_fcntl64(scmp_filter_ctx ctx, sandbox_cfg_t *filter) int rc = 0; (void) filter; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fcntl64), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fcntl64), SCMP_CMP(1, SCMP_CMP_EQ, F_GETFL)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fcntl64), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fcntl64), SCMP_CMP(1, SCMP_CMP_EQ, F_SETFL), SCMP_CMP(2, SCMP_CMP_EQ, O_RDWR|O_NONBLOCK)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fcntl64), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fcntl64), SCMP_CMP(1, SCMP_CMP_EQ, F_GETFD)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fcntl64), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fcntl64), SCMP_CMP(1, SCMP_CMP_EQ, F_SETFD), SCMP_CMP(2, SCMP_CMP_EQ, FD_CLOEXEC)); if (rc) @@ -610,17 +622,17 @@ sb_epoll_ctl(scmp_filter_ctx ctx, sandbox_cfg_t *filter) int rc = 0; (void) filter; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(epoll_ctl), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(epoll_ctl), SCMP_CMP(1, SCMP_CMP_EQ, EPOLL_CTL_ADD)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(epoll_ctl), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(epoll_ctl), SCMP_CMP(1, SCMP_CMP_EQ, EPOLL_CTL_MOD)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(epoll_ctl), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(epoll_ctl), SCMP_CMP(1, SCMP_CMP_EQ, EPOLL_CTL_DEL)); if (rc) return rc; @@ -641,7 +653,7 @@ sb_prctl(scmp_filter_ctx ctx, sandbox_cfg_t *filter) int rc = 0; (void) filter; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(prctl), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(prctl), SCMP_CMP(0, SCMP_CMP_EQ, PR_SET_DUMPABLE)); if (rc) return rc; @@ -662,12 +674,12 @@ sb_mprotect(scmp_filter_ctx ctx, sandbox_cfg_t *filter) int rc = 0; (void) filter; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), SCMP_CMP(2, SCMP_CMP_EQ, PROT_READ)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), SCMP_CMP(2, SCMP_CMP_EQ, PROT_NONE)); if (rc) return rc; @@ -685,12 +697,12 @@ sb_rt_sigprocmask(scmp_filter_ctx ctx, sandbox_cfg_t *filter) int rc = 0; (void) filter; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(rt_sigprocmask), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(rt_sigprocmask), SCMP_CMP(0, SCMP_CMP_EQ, SIG_UNBLOCK)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(rt_sigprocmask), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(rt_sigprocmask), SCMP_CMP(0, SCMP_CMP_EQ, SIG_SETMASK)); if (rc) return rc; @@ -710,12 +722,12 @@ sb_flock(scmp_filter_ctx ctx, sandbox_cfg_t *filter) int rc = 0; (void) filter; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(flock), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(flock), SCMP_CMP(1, SCMP_CMP_EQ, LOCK_EX|LOCK_NB)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(flock), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(flock), SCMP_CMP(1, SCMP_CMP_EQ, LOCK_UN)); if (rc) return rc; @@ -734,18 +746,18 @@ sb_futex(scmp_filter_ctx ctx, sandbox_cfg_t *filter) (void) filter; // can remove - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(futex), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(futex), SCMP_CMP(1, SCMP_CMP_EQ, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(futex), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(futex), SCMP_CMP(1, SCMP_CMP_EQ, FUTEX_WAKE_PRIVATE)); if (rc) return rc; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(futex), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(futex), SCMP_CMP(1, SCMP_CMP_EQ, FUTEX_WAIT_PRIVATE)); if (rc) return rc; @@ -765,7 +777,7 @@ sb_mremap(scmp_filter_ctx ctx, sandbox_cfg_t *filter) int rc = 0; (void) filter; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mremap), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mremap), SCMP_CMP(3, SCMP_CMP_EQ, MREMAP_MAYMOVE)); if (rc) return rc; @@ -783,7 +795,7 @@ sb_poll(scmp_filter_ctx ctx, sandbox_cfg_t *filter) int rc = 0; (void) filter; - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(poll), 2, + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(poll), SCMP_CMP(1, SCMP_CMP_EQ, 1), SCMP_CMP(2, SCMP_CMP_EQ, 10)); if (rc) @@ -809,7 +821,7 @@ sb_stat64(scmp_filter_ctx ctx, sandbox_cfg_t *filter) if (param != NULL && param->prot == 1 && (param->syscall == SCMP_SYS(open) || param->syscall == SCMP_SYS(stat64))) { - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(stat64), 1, + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(stat64), SCMP_CMP(0, SCMP_CMP_EQ, param->value)); if (rc != 0) { log_err(LD_BUG,"(Sandbox) failed to add open syscall, received " @@ -993,7 +1005,7 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) * Setting sandbox restrictions so the string memory cannot be tampered with */ // no mremap of the protected base address - ret = seccomp_rule_add(ctx, SCMP_ACT_KILL, SCMP_SYS(mremap), 1, + ret = seccomp_rule_add_1(ctx, SCMP_ACT_KILL, SCMP_SYS(mremap), SCMP_CMP(0, SCMP_CMP_EQ, (intptr_t) pr_mem_base)); if (ret) { log_err(LD_BUG,"(Sandbox) mremap protected memory filter fail!"); @@ -1001,7 +1013,7 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) } // no munmap of the protected base address - ret = seccomp_rule_add(ctx, SCMP_ACT_KILL, SCMP_SYS(munmap), 1, + ret = seccomp_rule_add_1(ctx, SCMP_ACT_KILL, SCMP_SYS(munmap), SCMP_CMP(0, SCMP_CMP_EQ, (intptr_t) pr_mem_base)); if (ret) { log_err(LD_BUG,"(Sandbox) munmap protected memory filter fail!"); @@ -1018,7 +1030,7 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) * There is a restriction on how much you can mprotect with R|W up to the * size of the canary. */ - ret = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), 3, + ret = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), SCMP_CMP(0, SCMP_CMP_LT, (intptr_t) pr_mem_base), SCMP_CMP(1, SCMP_CMP_LE, MALLOC_MP_LIM), SCMP_CMP(2, SCMP_CMP_EQ, PROT_READ|PROT_WRITE)); @@ -1027,7 +1039,7 @@ prot_strings(scmp_filter_ctx ctx, sandbox_cfg_t* cfg) return ret; } - ret = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), 3, + ret = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(mprotect), SCMP_CMP(0, SCMP_CMP_GT, (intptr_t) pr_mem_base + pr_mem_size + MALLOC_MP_LIM), SCMP_CMP(1, SCMP_CMP_LE, MALLOC_MP_LIM), @@ -1356,7 +1368,7 @@ add_noparam_filter(scmp_filter_ctx ctx) // add general filters for (i = 0; i < ARRAY_LENGTH(filter_nopar_gen); i++) { - rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, filter_nopar_gen[i], 0); + rc = seccomp_rule_add_0(ctx, SCMP_ACT_ALLOW, filter_nopar_gen[i]); if (rc != 0) { log_err(LD_BUG,"(Sandbox) failed to add syscall index %d (NR=%d), " "received libseccomp error %d", i, filter_nopar_gen[i], rc); From 156eefca454e10440d1070f7500e1708589fc64b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 13:17:09 -0400 Subject: [PATCH 12/22] Make sure everything using an interned string is preceded by a log (It's nice to know what we were about to rename before we died from renaming it.) --- src/common/compat.c | 2 ++ src/common/crypto.c | 1 + src/common/util.c | 2 ++ src/or/config.c | 1 + src/or/dns.c | 2 ++ 5 files changed, 8 insertions(+) diff --git a/src/common/compat.c b/src/common/compat.c index 5c18535285..c5945fbd22 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -144,6 +144,7 @@ tor_open_cloexec(const char *path, int flags, unsigned mode) return -1; #endif + log_debug(LD_FS, "Opening %s with flags %x", path, flags); fd = open(path, flags, mode); #ifdef FD_CLOEXEC if (fd >= 0) { @@ -179,6 +180,7 @@ tor_fopen_cloexec(const char *path, const char *mode) int tor_rename(const char *path_old, const char *path_new) { + log_debug(LD_FS, "Renaming %s to %s", path_old, path_new); return rename(sandbox_intern_string(path_old), sandbox_intern_string(path_new)); } diff --git a/src/common/crypto.c b/src/common/crypto.c index 8a4ffb6948..a247a87d48 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -2471,6 +2471,7 @@ crypto_strongest_rand(uint8_t *out, size_t out_len) return 0; #else for (i = 0; filenames[i]; ++i) { + log_debug(LD_FS, "Opening %s for entropy", filenames[i]); fd = open(sandbox_intern_string(filenames[i]), O_RDONLY, 0); if (fd<0) continue; log_info(LD_CRYPTO, "Reading entropy from \"%s\"", filenames[i]); diff --git a/src/common/util.c b/src/common/util.c index a50f2566db..86bb8baaef 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -1830,6 +1830,7 @@ file_status(const char *fname) int r; f = tor_strdup(fname); clean_name_for_stat(f); + log_debug(LD_FS, "stat()ing %s", f); r = stat(sandbox_intern_string(f), &st); tor_free(f); if (r) { @@ -1880,6 +1881,7 @@ check_private_dir(const char *dirname, cpd_check_t check, tor_assert(dirname); f = tor_strdup(dirname); clean_name_for_stat(f); + log_debug(LD_FS, "stat()ing %s", f); r = stat(sandbox_intern_string(f), &st); tor_free(f); if (r) { diff --git a/src/or/config.c b/src/or/config.c index 89aedccb4c..c2eebf77a6 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -6462,6 +6462,7 @@ remove_file_if_very_old(const char *fname, time_t now) #define VERY_OLD_FILE_AGE (28*24*60*60) struct stat st; + log_debug(LD_FS, "stat()ing %s", fname); if (stat(sandbox_intern_string(fname), &st)==0 && st.st_mtime < now-VERY_OLD_FILE_AGE) { char buf[ISO_TIME_LEN+1]; diff --git a/src/or/dns.c b/src/or/dns.c index 21c82e8062..36271939b4 100644 --- a/src/or/dns.c +++ b/src/or/dns.c @@ -1480,6 +1480,7 @@ configure_nameservers(int force) evdns_set_log_fn(evdns_log_cb); if (conf_fname) { + log_debug(LD_FS, "stat()ing %s", conf_fname); if (stat(sandbox_intern_string(conf_fname), &st)) { log_warn(LD_EXIT, "Unable to stat resolver configuration in '%s': %s", conf_fname, strerror(errno)); @@ -1497,6 +1498,7 @@ configure_nameservers(int force) #if defined(DNS_OPTION_HOSTSFILE) && defined(USE_LIBSECCOMP) if (flags & DNS_OPTION_HOSTSFILE) { flags ^= DNS_OPTION_HOSTSFILE; + log_debug(LD_FS, "Loading /etc/hosts"); evdns_base_load_hosts(the_evdns_base, sandbox_intern_string("/etc/hosts")); } From e6785ee16dce675aa770616bcdbd128d5dfb1132 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 14:54:39 -0400 Subject: [PATCH 13/22] Get Libevent's PRNG functioning under the linux sandbox Libevent uses an arc4random implementation (I know, I know) to generate DNS transaction IDs and capitalization. But it liked to initialize it either with opening /dev/urandom (which won't work under the sandbox if it doesn't use the right pointer), or with sysctl({CTL_KERN,KERN_RANDOM,RANDOM_UUIC}). To make _that_ work, we were permitting sysctl unconditionally. That's not such a great idea. Instead, we try to initialize the libevent PRNG _before_ installing the sandbox, and make sysctl always fail with EPERM under the sandbox. --- configure.ac | 1 + src/common/compat_libevent.c | 19 +++++++++++++++++++ src/common/compat_libevent.h | 2 ++ src/common/sandbox.c | 19 ++++++++++++++++++- src/or/main.c | 10 ++++++++++ 5 files changed, 50 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 6e41041961..6e5331b4c6 100644 --- a/configure.ac +++ b/configure.ac @@ -435,6 +435,7 @@ AC_CHECK_FUNCS([event_get_version \ event_set_log_callback \ evdns_set_outgoing_bind_address \ evutil_secure_rng_set_urandom_device_file \ + evutil_secure_rng_init \ event_base_loopexit]) AC_CHECK_MEMBERS([struct event.min_heap_idx], , , [#include diff --git a/src/common/compat_libevent.c b/src/common/compat_libevent.c index 8525b4a721..74b54bb855 100644 --- a/src/common/compat_libevent.c +++ b/src/common/compat_libevent.c @@ -13,6 +13,8 @@ #include "compat.h" #include "compat_libevent.h" +#include "crypto.h" + #include "util.h" #include "torlog.h" @@ -626,6 +628,23 @@ tor_add_bufferevent_to_rate_limit_group(struct bufferevent *bev, } #endif +int +tor_init_libevent_rng(void) +{ + int rv = 0; +#ifdef HAVE_EVUTIL_SECURE_RNG_INIT + char buf[256]; + if (evutil_secure_rng_init() < 0) { + rv = -1; + } + /* Older libevent -- manually initialize the RNG */ + crypto_rand(buf, 32); + evutil_secure_rng_add_bytes(buf, 32); + evutil_secure_rng_get_bytes(buf, sizeof(buf)); +#endif + return rv; +} + #if defined(LIBEVENT_VERSION_NUMBER) && LIBEVENT_VERSION_NUMBER >= V(2,1,1) \ && !defined(TOR_UNIT_TESTS) void diff --git a/src/common/compat_libevent.h b/src/common/compat_libevent.h index f0d1828b7b..9ee7b49cfb 100644 --- a/src/common/compat_libevent.h +++ b/src/common/compat_libevent.h @@ -89,6 +89,8 @@ int tor_add_bufferevent_to_rate_limit_group(struct bufferevent *bev, struct bufferevent_rate_limit_group *g); #endif +int tor_init_libevent_rng(void); + void tor_gettimeofday_cached(struct timeval *tv); void tor_gettimeofday_cache_clear(void); #ifdef TOR_UNIT_TESTS diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 2a1ddd13d1..40f2003096 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -124,7 +124,6 @@ static int filter_nopar_gen[] = { SCMP_SYS(read), SCMP_SYS(rt_sigreturn), SCMP_SYS(set_robust_list), - SCMP_SYS(_sysctl), #ifdef __NR_sigreturn SCMP_SYS(sigreturn), #endif @@ -373,6 +372,23 @@ sb_open(scmp_filter_ctx ctx, sandbox_cfg_t *filter) return 0; } +static int +sb__sysctl(scmp_filter_ctx ctx, sandbox_cfg_t *filter) +{ + int rc; + (void) filter; + (void) ctx; + + rc = seccomp_rule_add_0(ctx, SCMP_ACT_ERRNO(EPERM), SCMP_SYS(_sysctl)); + if (rc != 0) { + log_err(LD_BUG,"(Sandbox) failed to add _sysctl syscall, " + "received libseccomp error %d", rc); + return rc; + } + + return 0; +} + /** * Function responsible for setting up the rename syscall for * the seccomp filter sandbox. @@ -850,6 +866,7 @@ static sandbox_filter_func_t filter_func[] = { #endif sb_open, sb_openat, + sb__sysctl, sb_rename, #ifdef __NR_fcntl64 sb_fcntl64, diff --git a/src/or/main.c b/src/or/main.c index 341f22adcb..c0f14c0b89 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2432,6 +2432,9 @@ tor_init(int argc, char *argv[]) return -1; } stream_choice_seed_weak_rng(); + if (tor_init_libevent_rng() < 0) { + log_warn(LD_NET, "Problem initializing libevent RNG."); + } return 0; } @@ -2723,6 +2726,7 @@ init_addrinfo(void) static sandbox_cfg_t* sandbox_init_filter(void) { + const or_options_t *options = get_options(); sandbox_cfg_t *cfg = sandbox_cfg_new(); sandbox_cfg_allow_openat_filename(&cfg, @@ -2761,8 +2765,14 @@ sandbox_init_filter(void) tor_strdup("/dev/srandom"), tor_strdup("/dev/urandom"), tor_strdup("/dev/random"), + tor_strdup("/etc/hosts"), NULL, 0 ); + if (options->ServerDNSResolvConfFile) + sandbox_cfg_allow_open_filename(&cfg, + tor_strdup(options->ServerDNSResolvConfFile)); + else + sandbox_cfg_allow_open_filename(&cfg, tor_strdup("/etc/resolv.conf")); #define RENAME_SUFFIX(name, suffix) \ sandbox_cfg_allow_rename(&cfg, \ From ce776cf2700e5bb484630bc287b31204162adfac Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 15:15:31 -0400 Subject: [PATCH 14/22] Add a couple of missing renames so the server sandbox works again --- src/or/main.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/or/main.c b/src/or/main.c index c0f14c0b89..cdbb2db553 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2839,6 +2839,13 @@ sandbox_init_filter(void) RENAME_SUFFIX2("keys", "secret_onion_key.old", ".tmp"); RENAME_SUFFIX("hashed-fingerprint", ".tmp"); + sandbox_cfg_allow_rename(&cfg, + get_datadir_fname2("keys", "secret_onion_key"), + get_datadir_fname2("keys", "secret_onion_key.old")); + sandbox_cfg_allow_rename(&cfg, + get_datadir_fname2("keys", "secret_onion_key_ntor"), + get_datadir_fname2("keys", "secret_onion_key_ntor.old")); + sandbox_cfg_allow_stat_filename_array(&cfg, get_datadir_fname("keys"), get_datadir_fname("stats/dirreq-stats"), From 69eb2788302aa96e7d37597c407e8f7da4e8a96f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 15:17:23 -0400 Subject: [PATCH 15/22] Use SCMP_CMP_MASKED_EQ to allow flags, not force them Older versions of Libevent are happy to open SOCK_DGRAM sockets non-cloexec and non-nonblocking, and then set those flags afterwards. It's nice to be able to allow a flag to be on or off in the sandbox without having to enumerate all its values. Also, permit PF_INET6 sockets. (D'oh!) --- src/common/sandbox.c | 53 ++++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 40f2003096..5f9d625ef3 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -77,6 +77,13 @@ static sb_addr_info_t *sb_addr_info = NULL; #undef SCMP_CMP #define SCMP_CMP(a,b,c) ((struct scmp_arg_cmp){(a),(b),(c),0}) +#define SCMP_CMP4(a,b,c,d) ((struct scmp_arg_cmp){(a),(b),(c),(d)}) +/* We use a wrapper here because these masked comparisons seem to be pretty + * verbose. Also, it's important to cast to scmp_datum_t before negating the + * mask, since otherwise the negation might get applied to a 32 bit value, and + * the high bits of the value might get masked out improperly. */ +#define SCMP_CMP_MASKED(a,b,c) \ + SCMP_CMP4((a), SCMP_CMP_MASKED_EQ, ~(scmp_datum_t)(b), (c)) /** Variable used for storing all syscall numbers that will be allowed with the * stage 1 general Tor sandbox. @@ -258,12 +265,7 @@ sb_accept4(scmp_filter_ctx ctx, sandbox_cfg_t *filter) #endif rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(accept4), - SCMP_CMP(3, SCMP_CMP_EQ, SOCK_CLOEXEC)); - if (rc) { - return rc; - } - rc = seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(accept4), - SCMP_CMP(3, SCMP_CMP_EQ, SOCK_CLOEXEC|SOCK_NONBLOCK)); + SCMP_CMP_MASKED(3, SOCK_CLOEXEC|SOCK_NONBLOCK, 0)); if (rc) { return rc; } @@ -362,7 +364,7 @@ sb_open(scmp_filter_ctx ctx, sandbox_cfg_t *filter) } rc = seccomp_rule_add_1(ctx, SCMP_ACT_ERRNO(-1), SCMP_SYS(open), - SCMP_CMP(1, SCMP_CMP_EQ, O_RDONLY|O_CLOEXEC)); + SCMP_CMP_MASKED(1, O_CLOEXEC, O_RDONLY)); if (rc != 0) { log_err(LD_BUG,"(Sandbox) failed to add open syscall, received libseccomp " "error %d", rc); @@ -460,6 +462,7 @@ static int sb_socket(scmp_filter_ctx ctx, sandbox_cfg_t *filter) { int rc = 0; + int i; (void) filter; #ifdef __i386__ @@ -468,33 +471,29 @@ sb_socket(scmp_filter_ctx ctx, sandbox_cfg_t *filter) return rc; #endif - rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), + rc = seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), SCMP_CMP(0, SCMP_CMP_EQ, PF_FILE), - SCMP_CMP(1, SCMP_CMP_EQ, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK), - SCMP_CMP(2, SCMP_CMP_EQ, IPPROTO_IP)); + SCMP_CMP_MASKED(1, SOCK_CLOEXEC|SOCK_NONBLOCK, SOCK_STREAM)); if (rc) return rc; - rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), - SCMP_CMP(0, SCMP_CMP_EQ, PF_INET), - SCMP_CMP(1, SCMP_CMP_EQ, SOCK_STREAM|SOCK_CLOEXEC), + for (i = 0; i < 2; ++i) { + const int pf = i ? PF_INET : PF_INET6; + + rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), + SCMP_CMP(0, SCMP_CMP_EQ, pf), + SCMP_CMP_MASKED(1, SOCK_CLOEXEC|SOCK_NONBLOCK, SOCK_STREAM), SCMP_CMP(2, SCMP_CMP_EQ, IPPROTO_TCP)); - if (rc) - return rc; + if (rc) + return rc; - rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), - SCMP_CMP(0, SCMP_CMP_EQ, PF_INET), - SCMP_CMP(1, SCMP_CMP_EQ, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK), - SCMP_CMP(2, SCMP_CMP_EQ, IPPROTO_TCP)); - if (rc) - return rc; - - rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), - SCMP_CMP(0, SCMP_CMP_EQ, PF_INET), - SCMP_CMP(1, SCMP_CMP_EQ, SOCK_DGRAM|SOCK_CLOEXEC|SOCK_NONBLOCK), + rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), + SCMP_CMP(0, SCMP_CMP_EQ, pf), + SCMP_CMP_MASKED(1, SOCK_CLOEXEC|SOCK_NONBLOCK, SOCK_DGRAM), SCMP_CMP(2, SCMP_CMP_EQ, IPPROTO_IP)); - if (rc) - return rc; + if (rc) + return rc; + } rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(socket), SCMP_CMP(0, SCMP_CMP_EQ, PF_NETLINK), From 18f7f49a8c08a38c15de4b8e6413ed2ae0968639 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 15:54:45 -0400 Subject: [PATCH 16/22] Allow reloading torrc and writing to router-stability --- src/or/main.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/or/main.c b/src/or/main.c index cdbb2db553..4770b7e6dd 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2728,6 +2728,7 @@ sandbox_init_filter(void) { const or_options_t *options = get_options(); sandbox_cfg_t *cfg = sandbox_cfg_new(); + int i; sandbox_cfg_allow_openat_filename(&cfg, get_datadir_fname("cached-status")); @@ -2774,6 +2775,12 @@ sandbox_init_filter(void) else sandbox_cfg_allow_open_filename(&cfg, tor_strdup("/etc/resolv.conf")); + for (i = 0; i < 2; ++i) { + if (get_torrc_fname(i)) { + sandbox_cfg_allow_open_filename(&cfg, tor_strdup(get_torrc_fname(i))); + } + } + #define RENAME_SUFFIX(name, suffix) \ sandbox_cfg_allow_rename(&cfg, \ get_datadir_fname(name suffix), \ @@ -2827,6 +2834,8 @@ sandbox_init_filter(void) get_datadir_fname("fingerprint.tmp"), get_datadir_fname("hashed-fingerprint"), get_datadir_fname("hashed-fingerprint.tmp"), + get_datadir_fname("router-stability"), + get_datadir_fname("router-stability.tmp"), tor_strdup("/etc/resolv.conf"), NULL, 0 ); @@ -2838,6 +2847,7 @@ sandbox_init_filter(void) RENAME_SUFFIX2("keys", "secret_onion_key", ".tmp"); RENAME_SUFFIX2("keys", "secret_onion_key.old", ".tmp"); RENAME_SUFFIX("hashed-fingerprint", ".tmp"); + RENAME_SUFFIX("router-stability", ".tmp"); sandbox_cfg_allow_rename(&cfg, get_datadir_fname2("keys", "secret_onion_key"), From 619497076585c54dc80656cdd4e6181f1109ff53 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 16:05:10 -0400 Subject: [PATCH 17/22] Don't allow change to ConnLimit while sandbox is active --- src/common/sandbox.c | 11 +++++++++++ src/common/sandbox.h | 3 +++ src/or/config.c | 18 ++++++++++++------ 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 5f9d625ef3..0722751745 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -1576,6 +1576,11 @@ initialise_libseccomp_sandbox(sandbox_cfg_t* cfg) return 0; } +int +sandbox_is_active(void) +{ + return sandbox_active != 0; +} #endif // USE_LIBSECCOMP sandbox_cfg_t* @@ -1672,5 +1677,11 @@ sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) (void)cfg; (void)file1; (void)file2; return 0; } + +int +sandbox_is_active(void) +{ + return 0; +} #endif diff --git a/src/common/sandbox.h b/src/common/sandbox.h index c4144dbb2e..c40f5e0d1f 100644 --- a/src/common/sandbox.h +++ b/src/common/sandbox.h @@ -229,5 +229,8 @@ int sandbox_cfg_allow_stat_filename_array(sandbox_cfg_t **cfg, ...); /** Function used to initialise a sandbox configuration.*/ int sandbox_init(sandbox_cfg_t* cfg); +/** Return true iff the sandbox is turned on. */ +int sandbox_is_active(void); + #endif /* SANDBOX_H_ */ diff --git a/src/or/config.c b/src/or/config.c index c2eebf77a6..881da37855 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1,4 +1,4 @@ - /* Copyright (c) 2001 Matej Pfajfar. +/* Copyright (c) 2001 Matej Pfajfar. * Copyright (c) 2001-2004, Roger Dingledine. * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. * Copyright (c) 2007-2013, The Tor Project, Inc. */ @@ -1043,12 +1043,18 @@ options_act_reversible(const or_options_t *old_options, char **msg) if (running_tor) { int n_ports=0; /* We need to set the connection limit before we can open the listeners. */ - if (set_max_file_descriptors((unsigned)options->ConnLimit, - &options->ConnLimit_) < 0) { - *msg = tor_strdup("Problem with ConnLimit value. See logs for details."); - goto rollback; + if (! sandbox_is_active()) { + if (set_max_file_descriptors((unsigned)options->ConnLimit, + &options->ConnLimit_) < 0) { + *msg = tor_strdup("Problem with ConnLimit value. " + "See logs for details."); + goto rollback; + } + set_conn_limit = 1; + } else { + tor_assert(old_options); + options->ConnLimit_ = old_options->ConnLimit_; } - set_conn_limit = 1; /* Set up libevent. (We need to do this before we can register the * listeners as listeners.) */ From c80a6bd9d5cc341f822255050a24760d11cbd94a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 16:26:20 -0400 Subject: [PATCH 18/22] Don't reload logs or rewrite pidfile while sandbox is active --- src/or/config.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index 881da37855..77dcd1660b 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1137,11 +1137,13 @@ options_act_reversible(const or_options_t *old_options, char **msg) if (!running_tor) goto commit; - mark_logs_temp(); /* Close current logs once new logs are open. */ - logs_marked = 1; - if (options_init_logs(options, 0)<0) { /* Configure the tor_log(s) */ - *msg = tor_strdup("Failed to init Log options. See logs for details."); - goto rollback; + if (!sandbox_is_active()) { + mark_logs_temp(); /* Close current logs once new logs are open. */ + logs_marked = 1; + if (options_init_logs(options, 0)<0) { /* Configure the tor_log(s) */ + *msg = tor_strdup("Failed to init Log options. See logs for details."); + goto rollback; + } } commit: @@ -1488,8 +1490,9 @@ options_act(const or_options_t *old_options) /* Write our PID to the PID file. If we do not have write permissions we * will log a warning */ - if (options->PidFile) + if (options->PidFile && !sandbox_is_active()) { write_pidfile(options->PidFile); + } /* Register addressmap directives */ config_register_addressmaps(options); From f70cf9982ae3b0e57ca62612988478906707567f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 21:50:49 -0400 Subject: [PATCH 19/22] Sandbox: permit O_NONBLOCK and O_NOCTTY for files we refuse OpenSSL needs this, or RAND_poll() will kill the process. Also, refuse with EACCESS, not errno==-1 (!). --- src/common/sandbox.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 0722751745..7067a72c7d 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -363,8 +363,8 @@ sb_open(scmp_filter_ctx ctx, sandbox_cfg_t *filter) } } - rc = seccomp_rule_add_1(ctx, SCMP_ACT_ERRNO(-1), SCMP_SYS(open), - SCMP_CMP_MASKED(1, O_CLOEXEC, O_RDONLY)); + rc = seccomp_rule_add_1(ctx, SCMP_ACT_ERRNO(EACCES), SCMP_SYS(open), + SCMP_CMP_MASKED(1, O_CLOEXEC|O_NONBLOCK|O_NOCTTY, O_RDONLY)); if (rc != 0) { log_err(LD_BUG,"(Sandbox) failed to add open syscall, received libseccomp " "error %d", rc); From 2ae47d3c3ad7121b3ebfa8aa47cd67336218163e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 21:57:45 -0400 Subject: [PATCH 20/22] Block certain option transitions while sandbox enabled --- src/or/config.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/or/config.c b/src/or/config.c index 77dcd1660b..b686b66062 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -3584,6 +3584,12 @@ options_transition_allowed(const or_options_t *old, return -1; } + if (old->Sandbox != new_val->Sandbox) { + *msg = tor_strdup("While Tor is running, changing Sandbox " + "is not allowed."); + return -1; + } + if (strcmp(old->DataDirectory,new_val->DataDirectory)!=0) { tor_asprintf(msg, "While Tor is running, changing DataDirectory " @@ -3636,6 +3642,32 @@ options_transition_allowed(const or_options_t *old, return -1; } + if (sandbox_is_active()) { + if (! opt_streq(old->PidFile, new_val->PidFile)) { + *msg = tor_strdup("Can't change PidFile while Sandbox is active"); + return -1; + } + if (! config_lines_eq(old->Logs, new_val->Logs)) { + *msg = tor_strdup("Can't change Logs while Sandbox is active"); + return -1; + } + if (old->ConnLimit != new_val->ConnLimit) { + *msg = tor_strdup("Can't change ConnLimit while Sandbox is active"); + return -1; + } + if (! opt_streq(old->ServerDNSResolvConfFile, + new_val->ServerDNSResolvConfFile)) { + *msg = tor_strdup("Can't change ServerDNSResolvConfFile" + " while Sandbox is active"); + return -1; + } + if (server_mode(old) != server_mode(new_val)) { + *msg = tor_strdup("Can't start/stop being a server while " + "Sandbox is active"); + return -1; + } + } + return 0; } From f41491816ca1ac8dc39c4da0f6b9fe1065c5228a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Apr 2014 22:22:47 -0400 Subject: [PATCH 21/22] Log the name of the failing syscall on failure --- src/common/gen_linux_syscalls.pl | 37 + src/common/include.am | 1 + src/common/linux_syscalls.inc | 1153 ++++++++++++++++++++++++++++++ src/common/sandbox.c | 25 +- 4 files changed, 1213 insertions(+), 3 deletions(-) create mode 100755 src/common/gen_linux_syscalls.pl create mode 100644 src/common/linux_syscalls.inc diff --git a/src/common/gen_linux_syscalls.pl b/src/common/gen_linux_syscalls.pl new file mode 100755 index 0000000000..3c64098a0b --- /dev/null +++ b/src/common/gen_linux_syscalls.pl @@ -0,0 +1,37 @@ +#!/usr/bin/perl -w + +use strict; +my %syscalls = (); + +while (<>) { + if (/^#define (__NR_\w+) /) { + $syscalls{$1} = 1; + } +} + +print < Date: Wed, 16 Apr 2014 22:45:27 -0400 Subject: [PATCH 22/22] add a changes file for the sandbox fixes series --- changes/sandbox_fixes_11351 | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 changes/sandbox_fixes_11351 diff --git a/changes/sandbox_fixes_11351 b/changes/sandbox_fixes_11351 new file mode 100644 index 0000000000..2fe2173ced --- /dev/null +++ b/changes/sandbox_fixes_11351 @@ -0,0 +1,13 @@ + o Major features: + - Refinements and improvements to the Linux seccomp2 sandbox code: + the sandbox can now run a test network for multiple hours without + crashing. (Previous crash reasons included: reseeding the OpenSSL PRNG, + seeding the Libevent PRNG, using the wrong combination of CLOEXEC and + NONBLOCK at the same place and time, having server keys, being an + authority, receiving a HUP, or using IPv6.) The sandbox is still + experimental, and more bugs will probably turn up. To try it, + enable "Sandbox 1" on a Linux host. + + - Strengthen the Linux seccomp2 sandbox code: the sandbox can now + test the arguments for rename(), and blocks _sysctl() entirely. +