From 17884f979aa8d14b7f35cfc17107bb5f9c0f2b35 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 8 Mar 2021 11:09:37 +0900 Subject: [PATCH 1/8] test: move test_parse_syscall_and_errno() to test-seccomp.c --- src/test/meson.build | 4 +-- src/test/test-parse-util.c | 58 -------------------------------------- src/test/test-seccomp.c | 53 ++++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 61 deletions(-) diff --git a/src/test/meson.build b/src/test/meson.build index 62e31fd830..ff40a8d10d 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -224,9 +224,7 @@ tests += [ [['src/test/test-parse-argument.c']], - [['src/test/test-parse-util.c'], - [], - [libseccomp]], + [['src/test/test-parse-util.c']], [['src/test/test-sysctl-util.c']], diff --git a/src/test/test-parse-util.c b/src/test/test-parse-util.c index 756934acad..b1b23e2fbf 100644 --- a/src/test/test-parse-util.c +++ b/src/test/test-parse-util.c @@ -11,9 +11,6 @@ #include "log.h" #include "parse-util.h" #include "string-util.h" -#if HAVE_SECCOMP -#include "seccomp-util.h" -#endif static void test_parse_boolean(void) { assert_se(parse_boolean("1") == 1); @@ -783,60 +780,6 @@ static void test_parse_errno(void) { assert_se(parse_errno("EINVALaaa") == -EINVAL); } -static void test_parse_syscall_and_errno(void) { -#if HAVE_SECCOMP - _cleanup_free_ char *n = NULL; - int e; - - assert_se(parse_syscall_and_errno("uname:EILSEQ", &n, &e) >= 0); - assert_se(streq(n, "uname")); - assert_se(e == errno_from_name("EILSEQ") && e >= 0); - n = mfree(n); - - assert_se(parse_syscall_and_errno("uname:EINVAL", &n, &e) >= 0); - assert_se(streq(n, "uname")); - assert_se(e == errno_from_name("EINVAL") && e >= 0); - n = mfree(n); - - assert_se(parse_syscall_and_errno("@sync:4095", &n, &e) >= 0); - assert_se(streq(n, "@sync")); - assert_se(e == 4095); - n = mfree(n); - - /* If errno is omitted, then e is set to -1 */ - assert_se(parse_syscall_and_errno("mount", &n, &e) >= 0); - assert_se(streq(n, "mount")); - assert_se(e == -1); - n = mfree(n); - - /* parse_syscall_and_errno() does not check the syscall name is valid or not. */ - assert_se(parse_syscall_and_errno("hoge:255", &n, &e) >= 0); - assert_se(streq(n, "hoge")); - assert_se(e == 255); - n = mfree(n); - - assert_se(parse_syscall_and_errno("hoge:kill", &n, &e) >= 0); - assert_se(streq(n, "hoge")); - assert_se(e == SECCOMP_ERROR_NUMBER_KILL); - n = mfree(n); - - /* The function checks the syscall name is empty or not. */ - assert_se(parse_syscall_and_errno("", &n, &e) == -EINVAL); - assert_se(parse_syscall_and_errno(":255", &n, &e) == -EINVAL); - - /* errno must be a valid errno name or number between 0 and ERRNO_MAX == 4095, or "kill" */ - assert_se(parse_syscall_and_errno("hoge:4096", &n, &e) == -ERANGE); - assert_se(parse_syscall_and_errno("hoge:-3", &n, &e) == -ERANGE); - assert_se(parse_syscall_and_errno("hoge:12.3", &n, &e) == -EINVAL); - assert_se(parse_syscall_and_errno("hoge:123junk", &n, &e) == -EINVAL); - assert_se(parse_syscall_and_errno("hoge:junk123", &n, &e) == -EINVAL); - assert_se(parse_syscall_and_errno("hoge:255:EILSEQ", &n, &e) == -EINVAL); - assert_se(parse_syscall_and_errno("hoge:-EINVAL", &n, &e) == -EINVAL); - assert_se(parse_syscall_and_errno("hoge:EINVALaaa", &n, &e) == -EINVAL); - assert_se(parse_syscall_and_errno("hoge:", &n, &e) == -EINVAL); -#endif -} - static void test_parse_mtu(void) { uint32_t mtu = 0; @@ -914,7 +857,6 @@ int main(int argc, char *argv[]) { test_parse_nice(); test_parse_dev(); test_parse_errno(); - test_parse_syscall_and_errno(); test_parse_mtu(); test_parse_loadavg_fixed_point(); diff --git a/src/test/test-seccomp.c b/src/test/test-seccomp.c index 10393b6a7c..6aa91ae8ec 100644 --- a/src/test/test-seccomp.c +++ b/src/test/test-seccomp.c @@ -41,6 +41,58 @@ # define SECCOMP_RESTRICT_ADDRESS_FAMILIES_BROKEN 0 #endif +static void test_parse_syscall_and_errno(void) { + _cleanup_free_ char *n = NULL; + int e; + + assert_se(parse_syscall_and_errno("uname:EILSEQ", &n, &e) >= 0); + assert_se(streq(n, "uname")); + assert_se(e == errno_from_name("EILSEQ") && e >= 0); + n = mfree(n); + + assert_se(parse_syscall_and_errno("uname:EINVAL", &n, &e) >= 0); + assert_se(streq(n, "uname")); + assert_se(e == errno_from_name("EINVAL") && e >= 0); + n = mfree(n); + + assert_se(parse_syscall_and_errno("@sync:4095", &n, &e) >= 0); + assert_se(streq(n, "@sync")); + assert_se(e == 4095); + n = mfree(n); + + /* If errno is omitted, then e is set to -1 */ + assert_se(parse_syscall_and_errno("mount", &n, &e) >= 0); + assert_se(streq(n, "mount")); + assert_se(e == -1); + n = mfree(n); + + /* parse_syscall_and_errno() does not check the syscall name is valid or not. */ + assert_se(parse_syscall_and_errno("hoge:255", &n, &e) >= 0); + assert_se(streq(n, "hoge")); + assert_se(e == 255); + n = mfree(n); + + assert_se(parse_syscall_and_errno("hoge:kill", &n, &e) >= 0); + assert_se(streq(n, "hoge")); + assert_se(e == SECCOMP_ERROR_NUMBER_KILL); + n = mfree(n); + + /* The function checks the syscall name is empty or not. */ + assert_se(parse_syscall_and_errno("", &n, &e) == -EINVAL); + assert_se(parse_syscall_and_errno(":255", &n, &e) == -EINVAL); + + /* errno must be a valid errno name or number between 0 and ERRNO_MAX == 4095, or "kill" */ + assert_se(parse_syscall_and_errno("hoge:4096", &n, &e) == -ERANGE); + assert_se(parse_syscall_and_errno("hoge:-3", &n, &e) == -ERANGE); + assert_se(parse_syscall_and_errno("hoge:12.3", &n, &e) == -EINVAL); + assert_se(parse_syscall_and_errno("hoge:123junk", &n, &e) == -EINVAL); + assert_se(parse_syscall_and_errno("hoge:junk123", &n, &e) == -EINVAL); + assert_se(parse_syscall_and_errno("hoge:255:EILSEQ", &n, &e) == -EINVAL); + assert_se(parse_syscall_and_errno("hoge:-EINVAL", &n, &e) == -EINVAL); + assert_se(parse_syscall_and_errno("hoge:EINVALaaa", &n, &e) == -EINVAL); + assert_se(parse_syscall_and_errno("hoge:", &n, &e) == -EINVAL); +} + static void test_seccomp_arch_to_string(void) { uint32_t a, b; const char *name; @@ -1075,6 +1127,7 @@ static void test_restrict_suid_sgid(void) { int main(int argc, char *argv[]) { test_setup_logging(LOG_DEBUG); + test_parse_syscall_and_errno(); test_seccomp_arch_to_string(); test_architecture_table(); test_syscall_filter_set_find(); From 335171ca84c952033be661d67c98ed77efb83337 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 8 Mar 2021 11:10:26 +0900 Subject: [PATCH 2/8] test: parse_syscall_and_errno() accepts zero errno --- src/test/test-seccomp.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/test-seccomp.c b/src/test/test-seccomp.c index 6aa91ae8ec..b1f917eb54 100644 --- a/src/test/test-seccomp.c +++ b/src/test/test-seccomp.c @@ -72,6 +72,12 @@ static void test_parse_syscall_and_errno(void) { assert_se(e == 255); n = mfree(n); + /* 0 is also a valid errno. */ + assert_se(parse_syscall_and_errno("hoge:0", &n, &e) >= 0); + assert_se(streq(n, "hoge")); + assert_se(e == 0); + n = mfree(n); + assert_se(parse_syscall_and_errno("hoge:kill", &n, &e) >= 0); assert_se(streq(n, "hoge")); assert_se(e == SECCOMP_ERROR_NUMBER_KILL); From 1862b310c5780e41499251a495daea0832262dea Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 8 Mar 2021 11:10:54 +0900 Subject: [PATCH 3/8] seccomp: fix comment and change variable name --- src/shared/seccomp-util.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index ab24baaf9e..009992cfbf 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -1059,14 +1059,14 @@ int seccomp_load_syscall_filter_set(uint32_t default_action, const SyscallFilter return 0; } -int seccomp_load_syscall_filter_set_raw(uint32_t default_action, Hashmap* set, uint32_t action, bool log_missing) { +int seccomp_load_syscall_filter_set_raw(uint32_t default_action, Hashmap* filter, uint32_t action, bool log_missing) { uint32_t arch; int r; - /* Similar to seccomp_load_syscall_filter_set(), but takes a raw Set* of syscalls, instead of a - * SyscallFilterSet* table. */ + /* Similar to seccomp_load_syscall_filter_set(), but takes a raw Hashmap* of syscalls, instead + * of a SyscallFilterSet* table. */ - if (hashmap_isempty(set) && default_action == SCMP_ACT_ALLOW) + if (hashmap_isempty(filter) && default_action == SCMP_ACT_ALLOW) return 0; SECCOMP_FOREACH_LOCAL_ARCH(arch) { @@ -1079,7 +1079,7 @@ int seccomp_load_syscall_filter_set_raw(uint32_t default_action, Hashmap* set, u if (r < 0) return r; - HASHMAP_FOREACH_KEY(val, syscall_id, set) { + HASHMAP_FOREACH_KEY(val, syscall_id, filter) { uint32_t a = action; int id = PTR_TO_INT(syscall_id) - 1; int error = PTR_TO_INT(val); @@ -1095,7 +1095,8 @@ int seccomp_load_syscall_filter_set_raw(uint32_t default_action, Hashmap* set, u r = seccomp_rule_add_exact(seccomp, a, id, 0); if (r < 0) { - /* If the system call is not known on this architecture, then that's fine, let's ignore it */ + /* If the system call is not known on this architecture, then that's + * fine, let's ignore it */ _cleanup_free_ char *n = NULL; bool ignore; @@ -1113,7 +1114,8 @@ int seccomp_load_syscall_filter_set_raw(uint32_t default_action, Hashmap* set, u if (ERRNO_IS_SECCOMP_FATAL(r)) return r; if (r < 0) - log_debug_errno(r, "Failed to install filter set for architecture %s, skipping: %m", seccomp_arch_to_string(arch)); + log_debug_errno(r, "Failed to install systemc call filter for architecture %s, skipping: %m", + seccomp_arch_to_string(arch)); } return 0; From 696a13bab779b060a8254c9a1e74eee9d28ac5e1 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 8 Mar 2021 11:54:05 +0900 Subject: [PATCH 4/8] core: drop meaningless parse_syscall_and_errno() calls parse_syscall_and_errno() does not check the validity of syscall name or syscall group name, but it just split into syscall name and errno. So, it is not necessary to call it for SystemCallLog=. --- src/core/dbus-execute.c | 11 ++--------- src/core/load-fragment.c | 11 ++--------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index b664bbffa6..399986db4a 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -2315,15 +2315,8 @@ int bus_exec_context_set_transient_property( } STRV_FOREACH(s, l) { - _cleanup_free_ char *n = NULL; - int e; - - r = parse_syscall_and_errno(*s, &n, &e); - if (r < 0) - return r; - - r = seccomp_parse_syscall_filter(n, - 0, /* errno not used */ + r = seccomp_parse_syscall_filter(*s, + -1, /* errno not used */ c->syscall_log, SECCOMP_PARSE_LOG | SECCOMP_PARSE_PERMISSIVE | invert_flag | diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index f9c334cc65..41ee7b6438 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3280,8 +3280,7 @@ int config_parse_syscall_log( p = rvalue; for (;;) { - _cleanup_free_ char *word = NULL, *name = NULL; - int num; + _cleanup_free_ char *word = NULL; r = extract_first_word(&p, &word, NULL, 0); if (r == 0) @@ -3293,14 +3292,8 @@ int config_parse_syscall_log( return 0; } - r = parse_syscall_and_errno(word, &name, &num); - if (r < 0 || num >= 0) { /* errno code not allowed */ - log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to parse syscall, ignoring: %s", word); - continue; - } - r = seccomp_parse_syscall_filter( - name, 0, c->syscall_log, + word, -1, c->syscall_log, SECCOMP_PARSE_LOG|SECCOMP_PARSE_PERMISSIVE| (invert ? SECCOMP_PARSE_INVERT : 0)| (c->syscall_log_allow_list ? SECCOMP_PARSE_ALLOW_LIST : 0), From 084a46d7c56a3f5812bb63f3e1a860c8952e6e0d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 8 Mar 2021 11:57:59 +0900 Subject: [PATCH 5/8] core,seccomp: refuse to specify errno for allow-listed syscalls --- src/core/dbus-execute.c | 3 +++ src/core/load-fragment.c | 11 +++++++++-- src/shared/seccomp-util.c | 3 +++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 399986db4a..4a1585f663 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -2252,6 +2252,9 @@ int bus_exec_context_set_transient_property( if (r < 0) return r; + if (allow_list && e >= 0) + return -EINVAL; + r = seccomp_parse_syscall_filter(n, e, c->syscall_filter, diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 41ee7b6438..c6fc4fe083 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3209,13 +3209,20 @@ int config_parse_syscall_filter( if (r == -ENOMEM) return log_oom(); if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, "Invalid syntax, ignoring: %s", rvalue); + log_syntax(unit, LOG_WARNING, filename, line, r, + "Invalid syntax, ignoring: %s", rvalue); return 0; } r = parse_syscall_and_errno(word, &name, &num); if (r < 0) { - log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to parse syscall:errno, ignoring: %s", word); + log_syntax(unit, LOG_WARNING, filename, line, r, + "Failed to parse syscall:errno, ignoring: %s", word); + continue; + } + if (!invert && num >= 0) { + log_syntax(unit, LOG_WARNING, filename, line, 0, + "Allow-listed system calls cannot take error number, ignoring: %s", word); continue; } diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index 009992cfbf..3354f7bc08 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -1135,6 +1135,9 @@ int seccomp_parse_syscall_filter( assert(name); assert(filter); + if (!FLAGS_SET(flags, SECCOMP_PARSE_INVERT) && errno_num >= 0) + return -EINVAL; + if (name[0] == '@') { const SyscallFilterSet *set; const char *i; From 9e29ee407253a605b3fddf852594716564f6b20b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 8 Mar 2021 11:58:58 +0900 Subject: [PATCH 6/8] seccomp: use FLAGS_SET() macro --- src/shared/seccomp-util.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index 3354f7bc08..bfb72ce3ed 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -1144,10 +1144,10 @@ int seccomp_parse_syscall_filter( set = syscall_filter_set_find(name); if (!set) { - if (!(flags & SECCOMP_PARSE_PERMISSIVE)) + if (!FLAGS_SET(flags, SECCOMP_PARSE_PERMISSIVE)) return -EINVAL; - log_syntax(unit, flags & SECCOMP_PARSE_LOG ? LOG_WARNING : LOG_DEBUG, filename, line, 0, + log_syntax(unit, FLAGS_SET(flags, SECCOMP_PARSE_LOG) ? LOG_WARNING : LOG_DEBUG, filename, line, 0, "Unknown system call group, ignoring: %s", name); return 0; } @@ -1166,10 +1166,10 @@ int seccomp_parse_syscall_filter( id = seccomp_syscall_resolve_name(name); if (id == __NR_SCMP_ERROR) { - if (!(flags & SECCOMP_PARSE_PERMISSIVE)) + if (!FLAGS_SET(flags, SECCOMP_PARSE_PERMISSIVE)) return -EINVAL; - log_syntax(unit, flags & SECCOMP_PARSE_LOG ? LOG_WARNING : LOG_DEBUG, filename, line, 0, + log_syntax(unit, FLAGS_SET(flags, SECCOMP_PARSE_LOG) ? LOG_WARNING : LOG_DEBUG, filename, line, 0, "Failed to parse system call, ignoring: %s", name); return 0; } @@ -1181,7 +1181,7 @@ int seccomp_parse_syscall_filter( if (r < 0) switch (r) { case -ENOMEM: - return flags & SECCOMP_PARSE_LOG ? log_oom() : -ENOMEM; + return FLAGS_SET(flags, SECCOMP_PARSE_LOG) ? log_oom() : -ENOMEM; case -EEXIST: assert_se(hashmap_update(filter, INT_TO_PTR(id + 1), INT_TO_PTR(errno_num)) == 0); break; From 68acc1afbe5cec50da1ffdc411dadda504e4caf5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 8 Mar 2021 12:00:32 +0900 Subject: [PATCH 7/8] seccomp: do not ignore deny-listed syscalls with errno when list is allow-list Previously, if the hashmap is allow-list and a new deny-listed syscall is added, seccomp_parse_syscall_filter() simply drop the new syscall from hashmap even if error number is specified. This makes 'allow-list' hashmap store two types of entries: - allow-listed syscalls, which are stored with negative value (-1). - deny-listed syscalls, which are stored with specified errno. Fixes #18916. --- src/shared/seccomp-util.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index bfb72ce3ed..9813d82f95 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -1090,7 +1090,7 @@ int seccomp_load_syscall_filter_set_raw(uint32_t default_action, Hashmap* filter else if (action == SCMP_ACT_LOG) a = SCMP_ACT_LOG; #endif - else if (action != SCMP_ACT_ALLOW && error >= 0) + else if (error >= 0) a = SCMP_ACT_ERRNO(error); r = seccomp_rule_add_exact(seccomp, a, id, 0); @@ -1174,9 +1174,11 @@ int seccomp_parse_syscall_filter( return 0; } - /* If we previously wanted to forbid a syscall and now - * we want to allow it, then remove it from the list. */ - if (!(flags & SECCOMP_PARSE_INVERT) == !!(flags & SECCOMP_PARSE_ALLOW_LIST)) { + /* If we previously wanted to forbid a syscall and now we want to allow it, then remove + * it from the list. The entries in allow-list with non-negative error value will be + * handled with SCMP_ACT_ERRNO() instead of the default action. */ + if (!FLAGS_SET(flags, SECCOMP_PARSE_INVERT) == FLAGS_SET(flags, SECCOMP_PARSE_ALLOW_LIST) || + (FLAGS_SET(flags, SECCOMP_PARSE_INVERT | SECCOMP_PARSE_ALLOW_LIST) && errno_num >= 0)) { r = hashmap_put(filter, INT_TO_PTR(id + 1), INT_TO_PTR(errno_num)); if (r < 0) switch (r) { From a62f651b32ddd406bf395312d20095690123d46a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 8 Mar 2021 12:06:09 +0900 Subject: [PATCH 8/8] test: add one more test for system call filter with errno --- src/test/test-execute.c | 1 + ...xec-systemcallfilter-with-errno-in-allow-list.service | 9 +++++++++ 2 files changed, 10 insertions(+) create mode 100644 test/test-execute/exec-systemcallfilter-with-errno-in-allow-list.service diff --git a/src/test/test-execute.c b/src/test/test-execute.c index c0e046b5e2..239fcea5e3 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -444,6 +444,7 @@ static void test_exec_systemcallfilter(Manager *m) { test(m, "exec-systemcallfilter-with-errno-name.service", errno_from_name("EILSEQ"), CLD_EXITED); test(m, "exec-systemcallfilter-with-errno-number.service", 255, CLD_EXITED); test(m, "exec-systemcallfilter-with-errno-multi.service", errno_from_name("EILSEQ"), CLD_EXITED); + test(m, "exec-systemcallfilter-with-errno-in-allow-list.service", errno_from_name("EILSEQ"), CLD_EXITED); test(m, "exec-systemcallfilter-override-error-action.service", SIGSYS, CLD_KILLED); test(m, "exec-systemcallfilter-override-error-action2.service", errno_from_name("EILSEQ"), CLD_EXITED); #endif diff --git a/test/test-execute/exec-systemcallfilter-with-errno-in-allow-list.service b/test/test-execute/exec-systemcallfilter-with-errno-in-allow-list.service new file mode 100644 index 0000000000..4b2636eb44 --- /dev/null +++ b/test/test-execute/exec-systemcallfilter-with-errno-in-allow-list.service @@ -0,0 +1,9 @@ +[Unit] +Description=Test for SystemCallFilter with errno name (for issue #18916) + +[Service] +ExecStart=/usr/bin/python3 -c 'import os\ntry: os.uname()\nexcept Exception as e: exit(e.errno)' +Type=oneshot +SystemCallFilter=@system-service +SystemCallFilter=~uname:EILSEQ +SystemCallErrorNumber=EACCES