From 8c93ebbdf0e2fd0bc13e26b61581cdecbd805f41 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 16 Jun 2022 01:13:58 +0900 Subject: [PATCH 1/6] core/dbus-execute: do not append denied syscalls in allow-list Follow-up for 68acc1afbe5cec50da1ffdc411dadda504e4caf5. Before the commit, SystemCallFilter bus property provides only allowed syscalls if ExecContext.syscall_filter is an allow-list, and vice versa. After the commit, if the list is allow-list, it contains allowed syscalls with value `-1`, and denied syscalls with non-negative values. To keep the backward compatibility, denied syscalls must be dropped in SystemCallFilter bus property. --- src/core/dbus-execute.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index a3e54e6411..59c9352296 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -375,6 +375,10 @@ static int property_get_syscall_filter( char *s; int num = PTR_TO_INT(val); + if (c->syscall_allow_list && num >= 0) + /* syscall with num >= 0 in allow-list is denied. */ + continue; + name = seccomp_syscall_resolve_num_arch(SCMP_ARCH_NATIVE, PTR_TO_INT(id) - 1); if (!name) continue; From 1008d415e7054e206354fa54c61e4020f6f7573e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 16 Jun 2022 01:14:30 +0900 Subject: [PATCH 2/6] core/dbus-execute: drop unnecessary flag The code block is called only when the list was empty, and the newly requested list is allow-list. Hence, invert_flag is always zero here. --- src/core/dbus-execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 59c9352296..0b28d4f603 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -2470,7 +2470,7 @@ int bus_exec_context_set_transient_property( -1, c->syscall_filter, SECCOMP_PARSE_PERMISSIVE | - SECCOMP_PARSE_ALLOW_LIST | invert_flag, + SECCOMP_PARSE_ALLOW_LIST, u->id, NULL, 0); if (r < 0) From cb649d12bf3283974305c98ecf51e4bf7596a8bf Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 16 Jun 2022 01:20:20 +0900 Subject: [PATCH 3/6] set: introduce set_put_strndup() Note, if `n != SIZE_MAX`, we cannot check the existence of the specified string in the set without duplicating the string. And, set_consume() also checks the existence of the string. Hence, it is not necessary to call set_contains() if `n != SIZE_MAX`. --- src/basic/hashmap.c | 13 ++++++++----- src/basic/set.h | 9 ++++++--- src/test/test-set.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/basic/hashmap.c b/src/basic/hashmap.c index e33d6c3073..62380b0e4a 100644 --- a/src/basic/hashmap.c +++ b/src/basic/hashmap.c @@ -1842,7 +1842,7 @@ int _hashmap_put_strdup_full(Hashmap **h, const struct hash_ops *hash_ops, const return r; } -int _set_put_strdup_full(Set **s, const struct hash_ops *hash_ops, const char *p HASHMAP_DEBUG_PARAMS) { +int _set_put_strndup_full(Set **s, const struct hash_ops *hash_ops, const char *p, size_t n HASHMAP_DEBUG_PARAMS) { char *c; int r; @@ -1853,10 +1853,13 @@ int _set_put_strdup_full(Set **s, const struct hash_ops *hash_ops, const char *p if (r < 0) return r; - if (set_contains(*s, (char*) p)) - return 0; + if (n == SIZE_MAX) { + if (set_contains(*s, (char*) p)) + return 0; - c = strdup(p); + c = strdup(p); + } else + c = strndup(p, n); if (!c) return -ENOMEM; @@ -1869,7 +1872,7 @@ int _set_put_strdupv_full(Set **s, const struct hash_ops *hash_ops, char **l HA assert(s); STRV_FOREACH(i, l) { - r = _set_put_strdup_full(s, hash_ops, *i HASHMAP_DEBUG_PASS_ARGS); + r = _set_put_strndup_full(s, hash_ops, *i, SIZE_MAX HASHMAP_DEBUG_PASS_ARGS); if (r < 0) return r; diff --git a/src/basic/set.h b/src/basic/set.h index 243a747e98..52cf63e2dd 100644 --- a/src/basic/set.h +++ b/src/basic/set.h @@ -127,9 +127,12 @@ int _set_ensure_consume(Set **s, const struct hash_ops *hash_ops, void *key HAS int set_consume(Set *s, void *value); -int _set_put_strdup_full(Set **s, const struct hash_ops *hash_ops, const char *p HASHMAP_DEBUG_PARAMS); -#define set_put_strdup_full(s, hash_ops, p) _set_put_strdup_full(s, hash_ops, p HASHMAP_DEBUG_SRC_ARGS) -#define set_put_strdup(s, p) set_put_strdup_full(s, &string_hash_ops_free, p) +int _set_put_strndup_full(Set **s, const struct hash_ops *hash_ops, const char *p, size_t n HASHMAP_DEBUG_PARAMS); +#define set_put_strndup_full(s, hash_ops, p, n) _set_put_strndup_full(s, hash_ops, p, n HASHMAP_DEBUG_SRC_ARGS) +#define set_put_strdup_full(s, hash_ops, p) set_put_strndup_full(s, hash_ops, p, SIZE_MAX) +#define set_put_strndup(s, p, n) set_put_strndup_full(s, &string_hash_ops_free, p, n) +#define set_put_strdup(s, p) set_put_strndup(s, p, SIZE_MAX) + int _set_put_strdupv_full(Set **s, const struct hash_ops *hash_ops, char **l HASHMAP_DEBUG_PARAMS); #define set_put_strdupv_full(s, hash_ops, l) _set_put_strdupv_full(s, hash_ops, l HASHMAP_DEBUG_SRC_ARGS) #define set_put_strdupv(s, l) set_put_strdupv_full(s, &string_hash_ops_free, l) diff --git a/src/test/test-set.c b/src/test/test-set.c index 5c5c35f3a2..0fc9dffe23 100644 --- a/src/test/test-set.c +++ b/src/test/test-set.c @@ -90,6 +90,27 @@ TEST(set_put) { assert_se(strv_length(t) == 3); } +TEST(set_put_strndup) { + _cleanup_set_free_ Set *m = NULL; + + assert_se(set_put_strndup(&m, "12345", 0) == 1); + assert_se(set_put_strndup(&m, "12345", 1) == 1); + assert_se(set_put_strndup(&m, "12345", 2) == 1); + assert_se(set_put_strndup(&m, "12345", 3) == 1); + assert_se(set_put_strndup(&m, "12345", 4) == 1); + assert_se(set_put_strndup(&m, "12345", 5) == 1); + assert_se(set_put_strndup(&m, "12345", 6) == 0); + + assert_se(set_contains(m, "")); + assert_se(set_contains(m, "1")); + assert_se(set_contains(m, "12")); + assert_se(set_contains(m, "123")); + assert_se(set_contains(m, "1234")); + assert_se(set_contains(m, "12345")); + + assert_se(set_size(m) == 6); +} + TEST(set_put_strdup) { _cleanup_set_free_ Set *m = NULL; @@ -98,6 +119,10 @@ TEST(set_put_strdup) { assert_se(set_put_strdup(&m, "bbb") == 1); assert_se(set_put_strdup(&m, "bbb") == 0); assert_se(set_put_strdup(&m, "aaa") == 0); + + assert_se(set_contains(m, "aaa")); + assert_se(set_contains(m, "bbb")); + assert_se(set_size(m) == 2); } @@ -106,6 +131,11 @@ TEST(set_put_strdupv) { assert_se(set_put_strdupv(&m, STRV_MAKE("aaa", "aaa", "bbb", "bbb", "aaa")) == 2); assert_se(set_put_strdupv(&m, STRV_MAKE("aaa", "aaa", "bbb", "bbb", "ccc")) == 1); + + assert_se(set_contains(m, "aaa")); + assert_se(set_contains(m, "bbb")); + assert_se(set_contains(m, "ccc")); + assert_se(set_size(m) == 3); } From 5862e5561c9bbe87ad201e8d6b2ce2d0f04e7c37 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 16 Jun 2022 01:23:20 +0900 Subject: [PATCH 4/6] analyze-security: always save syscall name This reverts dd51e725df9aec2847482131ef601e0215b371a0 and fixes bugs introduced by 1624114d74f55ad9791b7624b08d89d2339a68b3. Previously, - On online scan, the syscall filter was a string Hashmap, but it might contain syscall name with errno or error action. Hence, we need to drop the errno or error action in the string. - On offline scan, the syscall filter was a Hashmap of syscall ID, so hashmap_contains() with syscall name did not work. We need to convert syscall IDs to syscall names. - If hashmap_contains() in syscall_names_in_filter() is true, then the syscall is allowed when the list is an allow-list, and vice versa. Hence, the condition in syscall_names_in_filter() was errnously inverted by dd51e725df9aec2847482131ef601e0215b371a0. This makes syscalls are always stored with its name, instead of ID, and also correct the condition. Fixes #23663. --- src/analyze/analyze-security.c | 39 +++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/analyze/analyze-security.c b/src/analyze/analyze-security.c index 5b4d4caf46..9255f4cb89 100644 --- a/src/analyze/analyze-security.c +++ b/src/analyze/analyze-security.c @@ -105,7 +105,7 @@ typedef struct SecurityInfo { Set *system_call_architectures; bool system_call_filter_allow_list; - Hashmap *system_call_filter; + Set *system_call_filter; mode_t _umask; } SecurityInfo; @@ -172,8 +172,7 @@ static SecurityInfo *security_info_free(SecurityInfo *i) { strv_free(i->supplementary_groups); set_free(i->system_call_architectures); - - hashmap_free(i->system_call_filter); + set_free(i->system_call_filter); return mfree(i); } @@ -567,12 +566,10 @@ static int assess_system_call_architectures( return 0; } -static bool syscall_names_in_filter(Hashmap *s, bool allow_list, const SyscallFilterSet *f, const char **ret_offending_syscall) { +static bool syscall_names_in_filter(Set *s, bool allow_list, const SyscallFilterSet *f, const char **ret_offending_syscall) { const char *syscall; NULSTR_FOREACH(syscall, f->value) { - int id; - if (syscall[0] == '@') { const SyscallFilterSet *g; @@ -584,11 +581,10 @@ static bool syscall_names_in_filter(Hashmap *s, bool allow_list, const SyscallFi } /* Let's see if the system call actually exists on this platform, before complaining */ - id = seccomp_syscall_resolve_name(syscall); - if (id < 0) + if (seccomp_syscall_resolve_name(syscall) < 0) continue; - if (hashmap_contains(s, syscall) != allow_list) { + if (set_contains(s, syscall) == allow_list) { log_debug("Offending syscall filter item: %s", syscall); if (ret_offending_syscall) *ret_offending_syscall = syscall; @@ -619,7 +615,7 @@ static int assess_system_call_filter( uint64_t b; int r; - if (!info->system_call_filter_allow_list && hashmap_isempty(info->system_call_filter)) { + if (!info->system_call_filter_allow_list && set_isempty(info->system_call_filter)) { r = free_and_strdup(&d, "Service does not filter system calls"); b = 10; } else { @@ -2139,9 +2135,8 @@ static int property_read_system_call_filter( if (r == 0) break; - /* The actual ExecContext stores the system call id as the map value, which we don't - * need. So we assign NULL to all values here. */ - r = hashmap_put_strdup(&info->system_call_filter, name, NULL); + /* ignore errno or action after colon */ + r = set_put_strndup(&info->system_call_filter, name, strchrnul(name, ':') - name); if (r < 0) return r; } @@ -2589,14 +2584,24 @@ static int get_security_info(Unit *u, ExecContext *c, CGroupContext *g, Security if (set_put_strdup(&info->system_call_architectures, name) < 0) return log_oom(); } -#endif info->system_call_filter_allow_list = c->syscall_allow_list; - if (c->syscall_filter) { - info->system_call_filter = hashmap_copy(c->syscall_filter); - if (!info->system_call_filter) + + void *id, *num; + HASHMAP_FOREACH_KEY(num, id, c->syscall_filter) { + _cleanup_free_ char *name = NULL; + + if (info->system_call_filter_allow_list && PTR_TO_INT(num) >= 0) + continue; + + name = seccomp_syscall_resolve_num_arch(SCMP_ARCH_NATIVE, PTR_TO_INT(id) - 1); + if (!name) + continue; + + if (set_ensure_consume(&info->system_call_filter, &string_hash_ops_free, TAKE_PTR(name)) < 0) return log_oom(); } +#endif } if (g) { From 6d6a08547c03f96dc798cda1ef4a8d3013d292d5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 16 Jun 2022 03:18:44 +0900 Subject: [PATCH 5/6] seccomp-util: make @known include @obsolete @known is generated from syscall-list.txt, which generated from kernel headers. So, some syscalls in @obsolete may not be listed in syscall-list.txt. --- src/shared/seccomp-util.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index b76189d6b3..0996ca6625 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -928,6 +928,7 @@ const SyscallFilterSet syscall_filter_sets[_SYSCALL_FILTER_SET_MAX] = { .name = "@known", .help = "All known syscalls declared in the kernel", .value = + "@obsolete\0" #include "syscall-list.h" }, }; From cf906beaef38def8d965f0ec593666a71fb5cc90 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 16 Jun 2022 03:21:28 +0900 Subject: [PATCH 6/6] test: add syscall filter tests for analyze security --- test/units/testsuite-65.sh | 60 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test/units/testsuite-65.sh b/test/units/testsuite-65.sh index 393297b17f..64ce629f3b 100755 --- a/test/units/testsuite-65.sh +++ b/test/units/testsuite-65.sh @@ -3,6 +3,9 @@ # shellcheck disable=SC2016 set -eux +# shellcheck source=test/units/assert.sh +. "$(dirname "$0")"/assert.sh + systemd-analyze log-level debug export SYSTEMD_LOG_LEVEL=debug @@ -606,6 +609,63 @@ fi systemd-analyze --threshold=90 security systemd-journald.service +# issue 23663 +check() {( + set +x + output=$(systemd-analyze security --offline="${2?}" "${3?}" | grep -F 'SystemCallFilter=') + assert_in "System call ${1?} list" "$output" + assert_in "[+✓] SystemCallFilter=~@swap" "$output" + assert_in "[+✓] SystemCallFilter=~@resources" "$output" + assert_in "[+✓] SystemCallFilter=~@reboot" "$output" + assert_in "[+✓] SystemCallFilter=~@raw-io" "$output" + assert_in "[-✗] SystemCallFilter=~@privileged" "$output" + assert_in "[+✓] SystemCallFilter=~@obsolete" "$output" + assert_in "[+✓] SystemCallFilter=~@mount" "$output" + assert_in "[+✓] SystemCallFilter=~@module" "$output" + assert_in "[+✓] SystemCallFilter=~@debug" "$output" + assert_in "[+✓] SystemCallFilter=~@cpu-emulation" "$output" + assert_in "[-✗] SystemCallFilter=~@clock" "$output" +)} + +export -n SYSTEMD_LOG_LEVEL + +mkdir -p /run/systemd/system +cat >/run/systemd/system/allow-list.service </run/systemd/system/deny-list.service <&1) +name=$(echo "$output" | awk '{ print $4 }') + +check allow yes /run/systemd/transient/"$name" +check allow no "$name" + +output=$(systemd-run -p "SystemCallFilter=~@known" -p "SystemCallFilter=@system-service" -p "SystemCallFilter=~@resources:ENOANO @privileged" -p "SystemCallFilter=@clock" sleep 60 2>&1) +name=$(echo "$output" | awk '{ print $4 }') + +check deny yes /run/systemd/transient/"$name" +check deny no "$name" + systemd-analyze log-level info echo OK >/testok