From 78e864e5b3cc11b72ae663f49f42f158cafbfedf Mon Sep 17 00:00:00 2001 From: Topi Miettinen Date: Tue, 4 Jul 2017 15:48:18 +0300 Subject: [PATCH 1/4] seccomp: LockPersonality boolean (#6193) Add LockPersonality boolean to allow locking down personality(2) system call so that the execution domain can't be changed. This may be useful to improve security because odd emulations may be poorly tested and source of vulnerabilities, while system services shouldn't need any weird personalities. --- man/systemd.exec.xml | 12 +++++++++ src/core/dbus-execute.c | 1 + src/core/execute.c | 33 +++++++++++++++++++++++- src/core/execute.h | 1 + src/core/load-fragment-gperf.gperf.m4 | 6 +++-- src/shared/seccomp-util.c | 19 ++++++++++++++ src/shared/seccomp-util.h | 1 + src/test/test-seccomp.c | 36 +++++++++++++++++++++++++++ 8 files changed, 106 insertions(+), 3 deletions(-) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index 7a7006b9a0..a9f1d8d74e 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -1653,6 +1653,18 @@ personality of the host system's kernel. + + LockPersonality= + + Locks down the personality2 system + call so that the kernel execution domain may not be changed from the default or the personality selected with + Personality= directive. This may be useful to improve security, because odd personality + emulations may be poorly tested and source of vulnerabilities. If running in user mode, or in system mode, but + without the CAP_SYS_ADMIN capability (e.g. setting User=), + NoNewPrivileges=yes is implied. + + RuntimeDirectory= diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 45497ca024..d28e8aafd6 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -853,6 +853,7 @@ const sd_bus_vtable bus_exec_vtable[] = { SD_BUS_PROPERTY("SystemCallArchitectures", "as", property_get_syscall_archs, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("SystemCallErrorNumber", "i", property_get_syscall_errno, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Personality", "s", property_get_personality, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("LockPersonality", "b", bus_property_get_bool, offsetof(ExecContext, lock_personality), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("RestrictAddressFamilies", "(bas)", property_get_address_families, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("RuntimeDirectoryPreserve", "s", property_get_exec_preserve_mode, offsetof(ExecContext, runtime_directory_preserve_mode), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("RuntimeDirectoryMode", "u", bus_property_get_mode, offsetof(ExecContext, directories[EXEC_DIRECTORY_RUNTIME].mode), SD_BUS_VTABLE_PROPERTY_CONST), diff --git a/src/core/execute.c b/src/core/execute.c index d192134b1c..4d285ff250 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1296,7 +1296,8 @@ static bool context_has_no_new_privileges(const ExecContext *c) { c->protect_kernel_modules || c->private_devices || context_has_syscall_filters(c) || - !set_isempty(c->syscall_archs); + !set_isempty(c->syscall_archs) || + c->lock_personality; } #ifdef HAVE_SECCOMP @@ -1455,6 +1456,25 @@ static int apply_restrict_namespaces(Unit *u, const ExecContext *c) { return seccomp_restrict_namespaces(c->restrict_namespaces); } +static int apply_lock_personality(const Unit* u, const ExecContext *c) { + unsigned long personality = c->personality; + + assert(u); + assert(c); + + if (!c->lock_personality) + return 0; + + if (skip_seccomp_unavailable(u, "LockPersonality=")) + return 0; + + /* If personality is not specified, use the default (Linux) */ + if (personality == PERSONALITY_INVALID) + personality = PER_LINUX; + + return seccomp_lock_personality(personality); +} + #endif static void do_idle_pipe_dance(int idle_pipe[4]) { @@ -2972,6 +2992,13 @@ static int exec_child( return r; } + r = apply_lock_personality(unit, context); + if (r < 0) { + *exit_status = EXIT_SECCOMP; + *error_message = strdup("Failed to lock personalities"); + return r; + } + /* This really should remain the last step before the execve(), to make sure our own code is unaffected * by the filter as little as possible. */ r = apply_syscall_filter(unit, context, needs_ambient_hack); @@ -3733,6 +3760,10 @@ void exec_context_dump(ExecContext *c, FILE* f, const char *prefix) { "%sPersonality: %s\n", prefix, strna(personality_to_string(c->personality))); + fprintf(f, + "%sLockPersonality: %s\n", + prefix, yes_no(c->lock_personality)); + if (c->syscall_filter) { #ifdef HAVE_SECCOMP Iterator j; diff --git a/src/core/execute.h b/src/core/execute.h index 9a28269283..8a7ce8449b 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -227,6 +227,7 @@ struct ExecContext { bool same_pgrp; unsigned long personality; + bool lock_personality; unsigned long restrict_namespaces; /* The CLONE_NEWxyz flags permitted to the unit's processes */ diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index 9a87f0acd3..94f3d657f6 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -60,14 +60,16 @@ $1.SystemCallErrorNumber, config_parse_syscall_errno, 0, $1.MemoryDenyWriteExecute, config_parse_bool, 0, offsetof($1, exec_context.memory_deny_write_execute) $1.RestrictNamespaces, config_parse_restrict_namespaces, 0, offsetof($1, exec_context) $1.RestrictRealtime, config_parse_bool, 0, offsetof($1, exec_context.restrict_realtime) -$1.RestrictAddressFamilies, config_parse_address_families, 0, offsetof($1, exec_context)', +$1.RestrictAddressFamilies, config_parse_address_families, 0, offsetof($1, exec_context) +$1.LockPersonality, config_parse_bool, 0, offsetof($1, exec_context.lock_personality)', `$1.SystemCallFilter, config_parse_warn_compat, DISABLED_CONFIGURATION, 0 $1.SystemCallArchitectures, config_parse_warn_compat, DISABLED_CONFIGURATION, 0 $1.SystemCallErrorNumber, config_parse_warn_compat, DISABLED_CONFIGURATION, 0 $1.MemoryDenyWriteExecute, config_parse_warn_compat, DISABLED_CONFIGURATION, 0 $1.RestrictNamespaces, config_parse_warn_compat, DISABLED_CONFIGURATION, 0 $1.RestrictRealtime, config_parse_warn_compat, DISABLED_CONFIGURATION, 0 -$1.RestrictAddressFamilies, config_parse_warn_compat, DISABLED_CONFIGURATION, 0') +$1.RestrictAddressFamilies, config_parse_warn_compat, DISABLED_CONFIGURATION, 0 +$1.LockPersonality, config_parse_warn_compat, DISABLED_CONFIGURATION, 0') $1.LimitCPU, config_parse_limit, RLIMIT_CPU, offsetof($1, exec_context.rlimit) $1.LimitFSIZE, config_parse_limit, RLIMIT_FSIZE, offsetof($1, exec_context.rlimit) $1.LimitDATA, config_parse_limit, RLIMIT_DATA, offsetof($1, exec_context.rlimit) diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index dd6d4fbdc7..bf2db28a82 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -29,6 +29,7 @@ #include "alloc-util.h" #include "macro.h" #include "nsflags.h" +#include "process-util.h" #include "seccomp-util.h" #include "set.h" #include "string-util.h" @@ -1402,3 +1403,21 @@ int seccomp_filter_set_add(Set *filter, bool add, const SyscallFilterSet *set) { return 0; } + +int seccomp_lock_personality(unsigned long personality) { + _cleanup_(seccomp_releasep) scmp_filter_ctx seccomp = NULL; + int r; + + seccomp = seccomp_init(SCMP_ACT_ALLOW); + if (!seccomp) + return -ENOMEM; + + r = seccomp_rule_add_exact(seccomp, SCMP_ACT_ERRNO(EPERM), + SCMP_SYS(personality), + 1, + SCMP_A0(SCMP_CMP_NE, personality)); + if (r < 0) + return r; + + return seccomp_load(seccomp); +} diff --git a/src/shared/seccomp-util.h b/src/shared/seccomp-util.h index 0edffa116d..ca43ba8659 100644 --- a/src/shared/seccomp-util.h +++ b/src/shared/seccomp-util.h @@ -78,6 +78,7 @@ int seccomp_protect_sysctl(void); int seccomp_restrict_address_families(Set *address_families, bool whitelist); int seccomp_restrict_realtime(void); int seccomp_memory_deny_write_execute(void); +int seccomp_lock_personality(unsigned long personality); extern const uint32_t seccomp_local_archs[]; diff --git a/src/test/test-seccomp.c b/src/test/test-seccomp.c index 28fe206507..7ffbc4754e 100644 --- a/src/test/test-seccomp.c +++ b/src/test/test-seccomp.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -565,6 +566,40 @@ static void test_load_syscall_filter_set_raw(void) { assert_se(wait_for_terminate_and_warn("syscallrawseccomp", pid, true) == EXIT_SUCCESS); } +static void test_lock_personality(void) { + pid_t pid; + + if (!is_seccomp_available()) + return; + if (geteuid() != 0) + return; + + pid = fork(); + assert_se(pid >= 0); + + if (pid == 0) { + assert_se(seccomp_lock_personality(PER_LINUX) >= 0); + + assert_se(personality(PER_LINUX) == PER_LINUX); + assert_se(personality(PER_LINUX | ADDR_NO_RANDOMIZE) == -1 && errno == EPERM); + assert_se(personality(PER_LINUX | MMAP_PAGE_ZERO) == -1 && errno == EPERM); + assert_se(personality(PER_LINUX | ADDR_COMPAT_LAYOUT) == -1 && errno == EPERM); + assert_se(personality(PER_LINUX | READ_IMPLIES_EXEC) == -1 && errno == EPERM); + assert_se(personality(PER_LINUX_32BIT) == -1 && errno == EPERM); + assert_se(personality(PER_SVR4) == -1 && errno == EPERM); + assert_se(personality(PER_BSD) == -1 && errno == EPERM); + assert_se(personality(PER_LINUX32) == -1 && errno == EPERM); + assert_se(personality(PER_LINUX32_3GB) == -1 && errno == EPERM); + assert_se(personality(PER_UW7) == -1 && errno == EPERM); + assert_se(personality(0x42) == -1 && errno == EPERM); + assert_se(personality(PERSONALITY_INVALID) == -1 && errno == EPERM); /* maybe remove this later */ + assert_se(personality(PER_LINUX) == PER_LINUX); + _exit(EXIT_SUCCESS); + } + + assert_se(wait_for_terminate_and_warn("lockpersonalityseccomp", pid, true) == EXIT_SUCCESS); +} + int main(int argc, char *argv[]) { log_set_max_level(LOG_DEBUG); @@ -581,6 +616,7 @@ int main(int argc, char *argv[]) { test_memory_deny_write_execute_shmat(); test_restrict_archs(); test_load_syscall_filter_set_raw(); + test_lock_personality(); return 0; } From e8132d63fea6986cb6bcb2b78d95b1ada3ada708 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Aug 2017 20:40:26 +0200 Subject: [PATCH 2/4] seccomp: default to something resembling the current personality when locking it Let's lock the personality to the currently set one, if nothing is specifically specified. But do so with a grain of salt, and never default to any exotic personality here, but only PER_LINUX or PER_LINUX32. --- src/basic/process-util.c | 19 +++++++++++++++++++ src/basic/process-util.h | 2 ++ src/core/execute.c | 15 +++++++++++---- src/test/test-seccomp.c | 38 ++++++++++++++++++++++++++++++++++---- 4 files changed, 66 insertions(+), 8 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 2b23ac36c0..056a8b82aa 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -904,6 +904,25 @@ const char* personality_to_string(unsigned long p) { return architecture_to_string(architecture); } +int opinionated_personality(unsigned long *ret) { + int current; + + /* Returns the current personality, or PERSONALITY_INVALID if we can't determine it. This function is a bit + * opinionated though, and ignores all the finer-grained bits and exotic personalities, only distinguishing the + * two most relevant personalities: PER_LINUX and PER_LINUX32. */ + + current = personality(PERSONALITY_INVALID); + if (current < 0) + return -errno; + + if (((unsigned long) current & 0xffff) == PER_LINUX32) + *ret = PER_LINUX32; + else + *ret = PER_LINUX; + + return 0; +} + void valgrind_summary_hack(void) { #ifdef HAVE_VALGRIND_VALGRIND_H if (getpid_cached() == 1 && RUNNING_ON_VALGRIND) { diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 913991b3cb..d71db2df7e 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -91,6 +91,8 @@ bool oom_score_adjust_is_valid(int oa); unsigned long personality_from_string(const char *p); const char *personality_to_string(unsigned long); +int opinionated_personality(unsigned long *ret); + int ioprio_class_to_string_alloc(int i, char **s); int ioprio_class_from_string(const char *s); diff --git a/src/core/execute.c b/src/core/execute.c index 4d285ff250..0cbaf9a00b 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1457,7 +1457,8 @@ static int apply_restrict_namespaces(Unit *u, const ExecContext *c) { } static int apply_lock_personality(const Unit* u, const ExecContext *c) { - unsigned long personality = c->personality; + unsigned long personality; + int r; assert(u); assert(c); @@ -1468,9 +1469,15 @@ static int apply_lock_personality(const Unit* u, const ExecContext *c) { if (skip_seccomp_unavailable(u, "LockPersonality=")) return 0; - /* If personality is not specified, use the default (Linux) */ - if (personality == PERSONALITY_INVALID) - personality = PER_LINUX; + personality = c->personality; + + /* If personality is not specified, use either PER_LINUX or PER_LINUX32 depending on what is currently set. */ + if (personality == PERSONALITY_INVALID) { + + r = opinionated_personality(&personality); + if (r < 0) + return r; + } return seccomp_lock_personality(personality); } diff --git a/src/test/test-seccomp.c b/src/test/test-seccomp.c index 7ffbc4754e..262d0b712b 100644 --- a/src/test/test-seccomp.c +++ b/src/test/test-seccomp.c @@ -567,6 +567,7 @@ static void test_load_syscall_filter_set_raw(void) { } static void test_lock_personality(void) { + unsigned long current; pid_t pid; if (!is_seccomp_available()) @@ -574,26 +575,55 @@ static void test_lock_personality(void) { if (geteuid() != 0) return; + assert_se(opinionated_personality(¤t) >= 0); + + log_info("current personality=%lu", current); + pid = fork(); assert_se(pid >= 0); if (pid == 0) { - assert_se(seccomp_lock_personality(PER_LINUX) >= 0); + assert_se(seccomp_lock_personality(current) >= 0); - assert_se(personality(PER_LINUX) == PER_LINUX); + assert_se((unsigned long) personality(current) == current); + + errno = EUCLEAN; assert_se(personality(PER_LINUX | ADDR_NO_RANDOMIZE) == -1 && errno == EPERM); + + errno = EUCLEAN; assert_se(personality(PER_LINUX | MMAP_PAGE_ZERO) == -1 && errno == EPERM); + + errno = EUCLEAN; assert_se(personality(PER_LINUX | ADDR_COMPAT_LAYOUT) == -1 && errno == EPERM); + + errno = EUCLEAN; assert_se(personality(PER_LINUX | READ_IMPLIES_EXEC) == -1 && errno == EPERM); + + errno = EUCLEAN; assert_se(personality(PER_LINUX_32BIT) == -1 && errno == EPERM); + + errno = EUCLEAN; assert_se(personality(PER_SVR4) == -1 && errno == EPERM); + + errno = EUCLEAN; assert_se(personality(PER_BSD) == -1 && errno == EPERM); - assert_se(personality(PER_LINUX32) == -1 && errno == EPERM); + + errno = EUCLEAN; + assert_se(personality(current == PER_LINUX ? PER_LINUX32 : PER_LINUX) == -1 && errno == EPERM); + + errno = EUCLEAN; assert_se(personality(PER_LINUX32_3GB) == -1 && errno == EPERM); + + errno = EUCLEAN; assert_se(personality(PER_UW7) == -1 && errno == EPERM); + + errno = EUCLEAN; assert_se(personality(0x42) == -1 && errno == EPERM); + + errno = EUCLEAN; assert_se(personality(PERSONALITY_INVALID) == -1 && errno == EPERM); /* maybe remove this later */ - assert_se(personality(PER_LINUX) == PER_LINUX); + + assert_se((unsigned long) personality(current) == current); _exit(EXIT_SUCCESS); } From 72eafe71597edfef84ec4a9822cb11e166c0c07f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Aug 2017 20:43:35 +0200 Subject: [PATCH 3/4] seccomp: rework seccomp_lock_personality() to apply filter to all archs --- src/shared/seccomp-util.c | 37 ++++++++++++++++++++++++++----------- src/test/test-seccomp.c | 1 - 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index bf2db28a82..29eb2b17d4 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -1405,19 +1405,34 @@ int seccomp_filter_set_add(Set *filter, bool add, const SyscallFilterSet *set) { } int seccomp_lock_personality(unsigned long personality) { - _cleanup_(seccomp_releasep) scmp_filter_ctx seccomp = NULL; + uint32_t arch; int r; - seccomp = seccomp_init(SCMP_ACT_ALLOW); - if (!seccomp) - return -ENOMEM; + if (personality >= PERSONALITY_INVALID) + return -EINVAL; - r = seccomp_rule_add_exact(seccomp, SCMP_ACT_ERRNO(EPERM), - SCMP_SYS(personality), - 1, - SCMP_A0(SCMP_CMP_NE, personality)); - if (r < 0) - return r; + SECCOMP_FOREACH_LOCAL_ARCH(arch) { + _cleanup_(seccomp_releasep) scmp_filter_ctx seccomp = NULL; - return seccomp_load(seccomp); + r = seccomp_init_for_arch(&seccomp, arch, SCMP_ACT_ALLOW); + if (r < 0) + return r; + + r = seccomp_rule_add_exact( + seccomp, + SCMP_ACT_ERRNO(EPERM), + SCMP_SYS(personality), + 1, + SCMP_A0(SCMP_CMP_NE, personality)); + if (r < 0) + return r; + + r = seccomp_load(seccomp); + if (IN_SET(r, -EPERM, -EACCES)) + return r; + if (r < 0) + log_debug_errno(r, "Failed to enable personality lock for architecture %s, skipping: %m", seccomp_arch_to_string(arch)); + } + + return 0; } diff --git a/src/test/test-seccomp.c b/src/test/test-seccomp.c index 262d0b712b..0632361d45 100644 --- a/src/test/test-seccomp.c +++ b/src/test/test-seccomp.c @@ -48,7 +48,6 @@ # define SECCOMP_RESTRICT_ADDRESS_FAMILIES_BROKEN 0 #endif - static void test_seccomp_arch_to_string(void) { uint32_t a, b; const char *name; From 3167f78a1100349aca1c8760c7f2c340ea60cf06 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Aug 2017 20:52:54 +0200 Subject: [PATCH 4/4] core: open up LockPersonality= for transient units Let's make "systemd-run -p LockPersonality=1 -t /bin/sh" work. --- src/core/dbus-execute.c | 4 +++- src/shared/bus-unit-util.c | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index d28e8aafd6..ac7cd7fd0c 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -1696,7 +1696,7 @@ int bus_exec_context_set_transient_property( "NoNewPrivileges", "SyslogLevelPrefix", "MemoryDenyWriteExecute", "RestrictRealtime", "DynamicUser", "RemoveIPC", "ProtectKernelTunables", "ProtectKernelModules", "ProtectControlGroups", "MountAPIVFS", - "CPUSchedulingResetOnFork", "NonBlocking")) { + "CPUSchedulingResetOnFork", "NonBlocking", "LockPersonality")) { int b; r = sd_bus_message_read(message, "b", &b); @@ -1744,6 +1744,8 @@ int bus_exec_context_set_transient_property( c->cpu_sched_reset_on_fork = b; else if (streq(name, "NonBlocking")) c->non_blocking = b; + else if (streq(name, "LockPersonality")) + c->lock_personality = b; unit_write_drop_in_private_format(u, mode, name, "%s=%s", name, yes_no(b)); } diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 1e720004c9..98a6ada9f2 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -214,7 +214,7 @@ int bus_append_unit_property_assignment(sd_bus_message *m, const char *assignmen "SyslogLevelPrefix", "Delegate", "RemainAfterElapse", "MemoryDenyWriteExecute", "RestrictRealtime", "DynamicUser", "RemoveIPC", "ProtectKernelTunables", "ProtectKernelModules", "ProtectControlGroups", "MountAPIVFS", - "CPUSchedulingResetOnFork")) { + "CPUSchedulingResetOnFork", "LockPersonality")) { r = parse_boolean(eq); if (r < 0)