From c79b4397d3839b77e85ceccc5a948f58c9fe37e6 Mon Sep 17 00:00:00 2001 From: Daniel Pinto Date: Wed, 1 Jul 2020 20:30:04 +0100 Subject: [PATCH 1/3] Fix seccomp sandbox rules for openat #27315 The need for casting negative syscall arguments depends on the glibc version. This affects the rules for the openat syscall which uses the constant AT_FDCWD that is defined as a negative number. This commit adds logic to only apply the cast when necessary, on glibc versions from 2.27 onwards. --- changes/bug27315 | 6 ++++++ src/lib/sandbox/sandbox.c | 42 +++++++++++++++++++++++++++++---------- 2 files changed, 38 insertions(+), 10 deletions(-) create mode 100644 changes/bug27315 diff --git a/changes/bug27315 b/changes/bug27315 new file mode 100644 index 0000000000..8af3ac8559 --- /dev/null +++ b/changes/bug27315 @@ -0,0 +1,6 @@ + o Minor bugfixes (linux seccomp2 sandbox): + - Fix a regression on sandboxing rules for the openat() syscall. + The fix for bug 25440 fixed the problem on systems with glibc >= + 2.27 but broke tor on previous versions of glibc. We now apply + the correct seccomp rule according to the running glibc version. + Patch from Daniel Pinto. Fixes bug 27315; bugfix on 0.3.5.11. diff --git a/src/lib/sandbox/sandbox.c b/src/lib/sandbox/sandbox.c index 8f577b0660..76aacb0834 100644 --- a/src/lib/sandbox/sandbox.c +++ b/src/lib/sandbox/sandbox.c @@ -134,6 +134,10 @@ static sandbox_cfg_t *filter_dynamic = NULL; * 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)) +/* For negative constants, the rule to add depends on the glibc version. */ +#define SCMP_CMP_NEG(a,op,b) (libc_negative_constant_needs_cast() ? \ + (SCMP_CMP((a), (op), (unsigned int)(b))) : \ + (SCMP_CMP_STR((a), (op), (b)))) /** Variable used for storing all syscall numbers that will be allowed with the * stage 1 general Tor sandbox. @@ -424,31 +428,49 @@ sb_mmap2(scmp_filter_ctx ctx, sandbox_cfg_t *filter) #endif #endif -/* Return true if we think we're running with a libc that always uses - * openat on linux. */ +/* Return true the libc version is greater or equal than + * major.minor. Returns false otherwise. */ static int -libc_uses_openat_for_everything(void) +is_libc_at_least(int major, int minor) { #ifdef CHECK_LIBC_VERSION const char *version = gnu_get_libc_version(); if (version == NULL) return 0; - int major = -1; - int minor = -1; + int libc_major = -1; + int libc_minor = -1; - tor_sscanf(version, "%d.%d", &major, &minor); - if (major >= 3) + tor_sscanf(version, "%d.%d", &libc_major, &libc_minor); + if (libc_major > major) return 1; - else if (major == 2 && minor >= 26) + else if (libc_major == major && libc_minor >= minor) return 1; else return 0; #else /* !(defined(CHECK_LIBC_VERSION)) */ + (void)major; + (void)minor; return 0; #endif /* defined(CHECK_LIBC_VERSION) */ } +/* Return true if we think we're running with a libc that always uses + * openat on linux. */ +static int +libc_uses_openat_for_everything(void) +{ + return is_libc_at_least(2, 26); +} + +/* Return true if we think we're running with a libc that needs to cast + * negative arguments like AT_FDCWD for seccomp rules. */ +static int +libc_negative_constant_needs_cast(void) +{ + return is_libc_at_least(2, 27); +} + /** Allow a single file to be opened. If use_openat is true, * we're using a libc that remaps all the opens into openats. */ static int @@ -456,7 +478,7 @@ allow_file_open(scmp_filter_ctx ctx, int use_openat, const char *file) { if (use_openat) { return seccomp_rule_add_2(ctx, SCMP_ACT_ALLOW, SCMP_SYS(openat), - SCMP_CMP(0, SCMP_CMP_EQ, (unsigned int)AT_FDCWD), + SCMP_CMP_NEG(0, SCMP_CMP_EQ, AT_FDCWD), SCMP_CMP_STR(1, SCMP_CMP_EQ, file)); } else { return seccomp_rule_add_1(ctx, SCMP_ACT_ALLOW, SCMP_SYS(open), @@ -592,7 +614,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_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(openat), - SCMP_CMP(0, SCMP_CMP_EQ, AT_FDCWD), + SCMP_CMP_NEG(0, SCMP_CMP_EQ, AT_FDCWD), SCMP_CMP_STR(1, SCMP_CMP_EQ, param->value), SCMP_CMP(2, SCMP_CMP_EQ, O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_DIRECTORY| O_CLOEXEC)); -- GitLab From d28bfb2cd5665c38bd14d6a72848209dcd66faf9 Mon Sep 17 00:00:00 2001 From: Daniel Pinto Date: Wed, 1 Jul 2020 23:51:39 +0100 Subject: [PATCH 2/3] Fix seccomp sandbox rules for opening directories #40020 Different versions of glibc use either open or openat for the opendir function. This commit adds logic to use the correct rule for each glibc version, namely: - Until 2.14 open is used - From 2.15 to to 2.21 openat is used - From 2.22 to 2.26 open is used - From 2.27 onwards openat is used --- changes/bug40020 | 9 +++++ src/app/main/main.c | 15 ++++++-- src/lib/sandbox/sandbox.c | 77 +++++++++++++++++++++++++++++++++++++-- src/lib/sandbox/sandbox.h | 7 ++++ 4 files changed, 100 insertions(+), 8 deletions(-) create mode 100644 changes/bug40020 diff --git a/changes/bug40020 b/changes/bug40020 new file mode 100644 index 0000000000..ca6ee2b85b --- /dev/null +++ b/changes/bug40020 @@ -0,0 +1,9 @@ + o Minor bugfixes (linux seccomp2 sandbox): + - Makes the seccomp sandbox allow the correct syscall for opendir + according to the running glibc version. The opendir function + either uses open or openat but the current code does not + differenciate between opendir and open calls. This adds a new + seccomp sandbox rule for opendir. This fixes crashes when + reloading torrc with sandbox enabled when running on glibc + 2.15 to 2.21 and 2.26. Patch from Daniel Pinto. Fixes bug 40020; + bugfix on 0.3.5.11. diff --git a/src/app/main/main.c b/src/app/main/main.c index 67f2181cd5..aceba78cfc 100644 --- a/src/app/main/main.c +++ b/src/app/main/main.c @@ -989,6 +989,9 @@ sandbox_init_filter(void) #define OPEN(name) \ sandbox_cfg_allow_open_filename(&cfg, tor_strdup(name)) +#define OPENDIR(dir) \ + sandbox_cfg_allow_opendir_dirname(&cfg, tor_strdup(dir)) + #define OPEN_DATADIR(name) \ sandbox_cfg_allow_open_filename(&cfg, get_datadir_fname(name)) @@ -1006,7 +1009,7 @@ sandbox_init_filter(void) } while (0) #define OPEN_KEY_DIRECTORY() \ - sandbox_cfg_allow_open_filename(&cfg, tor_strdup(options->KeyDirectory)) + OPENDIR(options->KeyDirectory) #define OPEN_CACHEDIR(name) \ sandbox_cfg_allow_open_filename(&cfg, get_cachedir_fname(name)) #define OPEN_CACHEDIR_SUFFIX(name, suffix) do { \ @@ -1020,7 +1023,7 @@ sandbox_init_filter(void) OPEN_KEYDIR(name suffix); \ } while (0) - OPEN(options->DataDirectory); + OPENDIR(options->DataDirectory); OPEN_KEY_DIRECTORY(); OPEN_CACHEDIR_SUFFIX("cached-certs", ".tmp"); @@ -1067,7 +1070,11 @@ sandbox_init_filter(void) } SMARTLIST_FOREACH(options->FilesOpenedByIncludes, char *, f, { - OPEN(f); + if (file_status(f) == FN_DIR) { + OPENDIR(f); + } else { + OPEN(f); + } }); #define RENAME_SUFFIX(name, suffix) \ @@ -1180,7 +1187,7 @@ sandbox_init_filter(void) * directory that holds it. */ char *dirname = tor_strdup(port->unix_addr); if (get_parent_directory(dirname) == 0) { - OPEN(dirname); + OPENDIR(dirname); } tor_free(dirname); sandbox_cfg_allow_chmod_filename(&cfg, tor_strdup(port->unix_addr)); diff --git a/src/lib/sandbox/sandbox.c b/src/lib/sandbox/sandbox.c index 76aacb0834..1903da70e8 100644 --- a/src/lib/sandbox/sandbox.c +++ b/src/lib/sandbox/sandbox.c @@ -276,6 +276,12 @@ static int filter_nopar_gen[] = { SCMP_SYS(poll) }; +/* opendir is not a syscall but it will use either open or openat. We do not + * want the decision to allow open/openat to be the callers reponsability, so + * we create a phony syscall number for opendir and sb_opendir will choose the + * correct syscall. */ +#define PHONY_OPENDIR_SYSCALL -2 + /* 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) \ @@ -455,14 +461,24 @@ is_libc_at_least(int major, int minor) #endif /* defined(CHECK_LIBC_VERSION) */ } -/* Return true if we think we're running with a libc that always uses - * openat on linux. */ +/* Return true if we think we're running with a libc that uses openat for the + * open function on linux. */ static int -libc_uses_openat_for_everything(void) +libc_uses_openat_for_open(void) { return is_libc_at_least(2, 26); } +/* Return true if we think we're running with a libc that uses openat for the + * opendir function on linux. */ +static int +libc_uses_openat_for_opendir(void) +{ + // libc 2.27 and above or between 2.15 (inclusive) and 2.22 (exclusive) + return is_libc_at_least(2, 27) || + (is_libc_at_least(2, 15) && !is_libc_at_least(2, 22)); +} + /* Return true if we think we're running with a libc that needs to cast * negative arguments like AT_FDCWD for seccomp rules. */ static int @@ -496,7 +512,7 @@ sb_open(scmp_filter_ctx ctx, sandbox_cfg_t *filter) int rc; sandbox_cfg_t *elem = NULL; - int use_openat = libc_uses_openat_for_everything(); + int use_openat = libc_uses_openat_for_open(); // for each dynamic parameter filters for (elem = filter; elem != NULL; elem = elem->next) { @@ -629,6 +645,38 @@ sb_openat(scmp_filter_ctx ctx, sandbox_cfg_t *filter) return 0; } +static int +sb_opendir(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 + == PHONY_OPENDIR_SYSCALL) { + if (libc_uses_openat_for_opendir()) { + rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(openat), + SCMP_CMP_NEG(0, SCMP_CMP_EQ, AT_FDCWD), + SCMP_CMP_STR(1, SCMP_CMP_EQ, param->value), + SCMP_CMP(2, SCMP_CMP_EQ, O_RDONLY|O_NONBLOCK|O_LARGEFILE| + O_DIRECTORY|O_CLOEXEC)); + } else { + rc = allow_file_open(ctx, 0, param->value); + } + if (rc != 0) { + log_err(LD_BUG,"(Sandbox) failed to add openat syscall, received " + "libseccomp error %d", rc); + return rc; + } + } + } + + return 0; +} + /** * Function responsible for setting up the socket syscall for * the seccomp filter sandbox. @@ -1128,6 +1176,7 @@ static sandbox_filter_func_t filter_func[] = { sb_chmod, sb_open, sb_openat, + sb_opendir, sb_rename, #ifdef __NR_fcntl64 sb_fcntl64, @@ -1447,6 +1496,19 @@ sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file) return 0; } +int +sandbox_cfg_allow_opendir_dirname(sandbox_cfg_t **cfg, char *dir) +{ + sandbox_cfg_t *elem = NULL; + + elem = new_element(PHONY_OPENDIR_SYSCALL, dir); + + elem->next = *cfg; + *cfg = elem; + + return 0; +} + /** * Function responsible for going through the parameter syscall filters and * call each function pointer in the list. @@ -1752,6 +1814,13 @@ sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file) return 0; } +int +sandbox_cfg_allow_opendir_dirname(sandbox_cfg_t **cfg, char *dir) +{ + (void)cfg; (void)dir; + return 0; +} + int sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file) { diff --git a/src/lib/sandbox/sandbox.h b/src/lib/sandbox/sandbox.h index 5bec09a36a..8542b57f9c 100644 --- a/src/lib/sandbox/sandbox.h +++ b/src/lib/sandbox/sandbox.h @@ -135,6 +135,13 @@ int sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2); */ int sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file); +/** + * Function used to add a opendir allowed filename to a supplied configuration. + * The (char*) specifies the path to the allowed dir; we steal the pointer to + * that dir. + */ +int sandbox_cfg_allow_opendir_dirname(sandbox_cfg_t **cfg, char *dir); + /** * Function used to add a stat/stat64 allowed filename to a configuration. * The (char*) specifies the path to the allowed file; that pointer is stolen. -- GitLab From eab8e7af522d18620450003667579eebaa339896 Mon Sep 17 00:00:00 2001 From: Daniel Pinto Date: Wed, 29 Jul 2020 00:34:08 +0100 Subject: [PATCH 3/3] Fix startup crash with seccomp sandbox enabled #40072 Fix crash introduced in #40020. On startup, tor calls check_private_dir on the data and key directories. This function uses open instead of opendir on the received directory. Data and key directoryes are only opened here, so the seccomp rule added should be for open instead of opendir, despite the fact that they are directories. --- src/app/main/main.c | 8 ++++++-- src/lib/sandbox/sandbox.c | 10 +--------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/app/main/main.c b/src/app/main/main.c index aceba78cfc..3f35d4d23f 100644 --- a/src/app/main/main.c +++ b/src/app/main/main.c @@ -1008,8 +1008,10 @@ sandbox_init_filter(void) OPEN_DATADIR2(name, name2 suffix); \ } while (0) +// KeyDirectory is a directory, but it is only opened in check_private_dir +// which calls open instead of opendir #define OPEN_KEY_DIRECTORY() \ - OPENDIR(options->KeyDirectory) + OPEN(options->KeyDirectory) #define OPEN_CACHEDIR(name) \ sandbox_cfg_allow_open_filename(&cfg, get_cachedir_fname(name)) #define OPEN_CACHEDIR_SUFFIX(name, suffix) do { \ @@ -1023,7 +1025,9 @@ sandbox_init_filter(void) OPEN_KEYDIR(name suffix); \ } while (0) - OPENDIR(options->DataDirectory); + // DataDirectory is a directory, but it is only opened in check_private_dir + // which calls open instead of opendir + OPEN(options->DataDirectory); OPEN_KEY_DIRECTORY(); OPEN_CACHEDIR_SUFFIX("cached-certs", ".tmp"); diff --git a/src/lib/sandbox/sandbox.c b/src/lib/sandbox/sandbox.c index 1903da70e8..2f26c5429b 100644 --- a/src/lib/sandbox/sandbox.c +++ b/src/lib/sandbox/sandbox.c @@ -657,15 +657,7 @@ sb_opendir(scmp_filter_ctx ctx, sandbox_cfg_t *filter) if (param != NULL && param->prot == 1 && param->syscall == PHONY_OPENDIR_SYSCALL) { - if (libc_uses_openat_for_opendir()) { - rc = seccomp_rule_add_3(ctx, SCMP_ACT_ALLOW, SCMP_SYS(openat), - SCMP_CMP_NEG(0, SCMP_CMP_EQ, AT_FDCWD), - SCMP_CMP_STR(1, SCMP_CMP_EQ, param->value), - SCMP_CMP(2, SCMP_CMP_EQ, O_RDONLY|O_NONBLOCK|O_LARGEFILE| - O_DIRECTORY|O_CLOEXEC)); - } else { - rc = allow_file_open(ctx, 0, param->value); - } + rc = allow_file_open(ctx, libc_uses_openat_for_opendir(), param->value); if (rc != 0) { log_err(LD_BUG,"(Sandbox) failed to add openat syscall, received " "libseccomp error %d", rc); -- GitLab