From bb28e68477a3a39796e4999a6cbc6ac6345a9159 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 6 Jul 2017 13:28:19 -0400 Subject: [PATCH 1/3] core/load-fragment: refuse units with errors in certain directives If an error is encountered in any of the Exec* lines, WorkingDirectory, SELinuxContext, ApparmorProfile, SmackProcessLabel, Service (in .socket units), User, or Group, refuse to load the unit. If the config stanza has support, ignore the failure if '-' is present. For those configuration directives, even if we started the unit, it's pretty likely that it'll do something unexpected (like write files in a wrong place, or with a wrong context, or run with wrong permissions, etc). It seems better to refuse to start the unit and have the admin clean up the configuration without giving the service a chance to mess up stuff. Note that all "security" options that restrict what the unit can do (Capabilities, AmbientCapabilities, Restrict*, SystemCallFilter, Limit*, PrivateDevices, Protect*, etc) are _not_ treated like this. Such options are only supplementary, and are not always available depending on the architecture and compilation options, so unit authors have to make sure that the service runs correctly without them anyway. Fixes #6237, #6277. --- src/core/load-fragment.c | 116 +++++++++++++++++++++++--------------- src/test/test-unit-file.c | 14 ++--- 2 files changed, 78 insertions(+), 52 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 6a6dadda2b..49a5c53f96 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -636,26 +636,36 @@ int config_parse_exec( r = unit_full_printf(u, f, &path); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers on %s, ignoring: %m", f); - return 0; + log_syntax(unit, LOG_ERR, filename, line, r, + "Failed to resolve unit specifiers on %s%s: %m", + f, ignore ? ", ignoring" : ""); + return ignore ? 0 : -ENOEXEC; } if (isempty(path)) { /* First word is either "-" or "@" with no command. */ - log_syntax(unit, LOG_ERR, filename, line, 0, "Empty path in command line, ignoring: \"%s\"", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, 0, + "Empty path in command line%s: \"%s\"", + ignore ? ", ignoring" : "", rvalue); + return ignore ? 0 : -ENOEXEC; } if (!string_is_safe(path)) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Executable path contains special characters, ignoring: %s", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, 0, + "Executable path contains special characters%s: %s", + ignore ? ", ignoring" : "", rvalue); + return ignore ? 0 : -ENOEXEC; } if (!path_is_absolute(path)) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Executable path is not absolute, ignoring: %s", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, 0, + "Executable path is not absolute%s: %s", + ignore ? ", ignoring" : "", rvalue); + return ignore ? 0 : -ENOEXEC; } if (endswith(path, "/")) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Executable path specifies a directory, ignoring: %s", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, 0, + "Executable path specifies a directory%s: %s", + ignore ? ", ignoring" : "", rvalue); + return ignore ? 0 : -ENOEXEC; } if (!separate_argv0) { @@ -708,12 +718,14 @@ int config_parse_exec( if (r == 0) break; if (r < 0) - return 0; + return ignore ? 0 : -ENOEXEC; r = unit_full_printf(u, word, &resolved); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Failed to resolve unit specifiers on %s, ignoring: %m", word); - return 0; + log_syntax(unit, LOG_ERR, filename, line, r, + "Failed to resolve unit specifiers on %s%s: %m", + word, ignore ? ", ignoring" : ""); + return ignore ? 0 : -ENOEXEC; } if (!GREEDY_REALLOC(n, nbufsize, nlen + 2)) @@ -724,8 +736,10 @@ int config_parse_exec( } if (!n || !n[0]) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Empty executable name or zeroeth argument, ignoring: %s", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, 0, + "Empty executable name or zeroeth argument%s: %s", + ignore ? ", ignoring" : "", rvalue); + return ignore ? 0 : -ENOEXEC; } nce = new0(ExecCommand, 1); @@ -1332,8 +1346,10 @@ int config_parse_exec_selinux_context( r = unit_full_printf(u, rvalue, &k); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %m"); - return 0; + log_syntax(unit, LOG_ERR, filename, line, r, + "Failed to resolve specifiers%s: %m", + ignore ? ", ignoring" : ""); + return ignore ? 0 : -ENOEXEC; } free(c->selinux_context); @@ -1380,8 +1396,10 @@ int config_parse_exec_apparmor_profile( r = unit_full_printf(u, rvalue, &k); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %m"); - return 0; + log_syntax(unit, LOG_ERR, filename, line, r, + "Failed to resolve specifiers%s: %m", + ignore ? ", ignoring" : ""); + return ignore ? 0 : -ENOEXEC; } free(c->apparmor_profile); @@ -1428,8 +1446,10 @@ int config_parse_exec_smack_process_label( r = unit_full_printf(u, rvalue, &k); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %m"); - return 0; + log_syntax(unit, LOG_ERR, filename, line, r, + "Failed to resolve specifiers%s: %m", + ignore ? ", ignoring" : ""); + return ignore ? 0 : -ENOEXEC; } free(c->smack_process_label); @@ -1647,19 +1667,19 @@ int config_parse_socket_service( r = unit_name_printf(UNIT(s), rvalue, &p); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers, ignoring: %s", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve specifiers: %s", rvalue); + return -ENOEXEC; } if (!endswith(p, ".service")) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Unit must be of type service, ignoring: %s", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, 0, "Unit must be of type service: %s", rvalue); + return -ENOEXEC; } r = manager_load_unit(UNIT(s)->manager, p, NULL, &error, &x); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to load unit %s, ignoring: %s", rvalue, bus_error_message(&error, r)); - return 0; + log_syntax(unit, LOG_ERR, filename, line, r, "Failed to load unit %s: %s", rvalue, bus_error_message(&error, r)); + return -ENOEXEC; } unit_ref_set(&s->service, x); @@ -1907,13 +1927,13 @@ int config_parse_user_group( r = unit_full_printf(u, rvalue, &k); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in %s, ignoring: %m", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in %s: %m", rvalue); + return -ENOEXEC; } if (!valid_user_group_name_or_id(k)) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid user/group name or numeric ID, ignoring: %s", k); - return 0; + log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid user/group name or numeric ID: %s", k); + return -ENOEXEC; } n = k; @@ -1971,19 +1991,19 @@ int config_parse_user_group_strv( if (r == -ENOMEM) return log_oom(); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Invalid syntax, ignoring: %s", rvalue); - break; + log_syntax(unit, LOG_ERR, filename, line, r, "Invalid syntax: %s", rvalue); + return -ENOEXEC; } r = unit_full_printf(u, word, &k); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in %s, ignoring: %m", word); - continue; + log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in %s: %m", word); + return -ENOEXEC; } if (!valid_user_group_name_or_id(k)) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid user/group name or numeric ID, ignoring: %s", k); - continue; + log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid user/group name or numeric ID: %s", k); + return -ENOEXEC; } r = strv_push(users, k); @@ -2142,25 +2162,28 @@ int config_parse_working_directory( r = unit_full_printf(u, rvalue, &k); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers in working directory path '%s', ignoring: %m", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, r, + "Failed to resolve unit specifiers in working directory path '%s'%s: %m", + rvalue, missing_ok ? ", ignoring" : ""); + return missing_ok ? 0 : -ENOEXEC; } path_kill_slashes(k); if (!utf8_is_valid(k)) { log_syntax_invalid_utf8(unit, LOG_ERR, filename, line, rvalue); - return 0; + return missing_ok ? 0 : -ENOEXEC; } if (!path_is_absolute(k)) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Working directory path '%s' is not absolute, ignoring.", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, 0, + "Working directory path '%s' is not absolute%s.", + rvalue, missing_ok ? ", ignoring" : ""); + return missing_ok ? 0 : -ENOEXEC; } - free_and_replace(c->working_directory, k); - c->working_directory_home = false; + free_and_replace(c->working_directory, k); } c->working_directory_missing_ok = missing_ok; @@ -4456,8 +4479,11 @@ int unit_load_fragment(Unit *u) { return r; r = load_from_path(u, k); - if (r < 0) + if (r < 0) { + if (r == -ENOEXEC) + log_unit_notice(u, "Unit configuration has fatal error, unit will not be started."); return r; + } if (u->load_state == UNIT_STUB) { SET_FOREACH(t, u->names, i) { diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index 12f48bf435..fd797b587e 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -146,7 +146,7 @@ static void test_config_parse_exec(void) { r = config_parse_exec(NULL, "fake", 4, "section", 1, "LValue", 0, "/RValue/ argv0 r1", &c, u); - assert_se(r == 0); + assert_se(r == -ENOEXEC); assert_se(c1->command_next == NULL); log_info("/* honour_argv0 */"); @@ -161,7 +161,7 @@ static void test_config_parse_exec(void) { r = config_parse_exec(NULL, "fake", 3, "section", 1, "LValue", 0, "@/RValue", &c, u); - assert_se(r == 0); + assert_se(r == -ENOEXEC); assert_se(c1->command_next == NULL); log_info("/* no command, whitespace only, reset */"); @@ -220,7 +220,7 @@ static void test_config_parse_exec(void) { "-@/RValue argv0 r1 ; ; " "/goo/goo boo", &c, u); - assert_se(r >= 0); + assert_se(r == -ENOEXEC); c1 = c1->command_next; check_execcommand(c1, "/RValue", "argv0", "r1", NULL, true); @@ -374,7 +374,7 @@ static void test_config_parse_exec(void) { r = config_parse_exec(NULL, "fake", 4, "section", 1, "LValue", 0, path, &c, u); - assert_se(r == 0); + assert_se(r == -ENOEXEC); assert_se(c1->command_next == NULL); } @@ -401,21 +401,21 @@ static void test_config_parse_exec(void) { r = config_parse_exec(NULL, "fake", 4, "section", 1, "LValue", 0, "/path\\", &c, u); - assert_se(r == 0); + assert_se(r == -ENOEXEC); assert_se(c1->command_next == NULL); log_info("/* missing ending ' */"); r = config_parse_exec(NULL, "fake", 4, "section", 1, "LValue", 0, "/path 'foo", &c, u); - assert_se(r == 0); + assert_se(r == -ENOEXEC); assert_se(c1->command_next == NULL); log_info("/* missing ending ' with trailing backslash */"); r = config_parse_exec(NULL, "fake", 4, "section", 1, "LValue", 0, "/path 'foo\\", &c, u); - assert_se(r == 0); + assert_se(r == -ENOEXEC); assert_se(c1->command_next == NULL); log_info("/* invalid space between modifiers */"); From 2c75fb7330394fbae95e4e429795f9854cea9069 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 6 Jul 2017 13:54:42 -0400 Subject: [PATCH 2/3] core/load-fragment: refuse units with errors in RootDirectory/RootImage/DynamicUser Behaviour of the service is completely different with the option off, so the service would probably mess up state on disk and do unexpected things. --- src/core/load-fragment-gperf.gperf.m4 | 6 +++--- src/core/load-fragment.c | 7 +++++-- src/shared/conf-parser.c | 16 +++++++++++----- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index c43b7885be..5b5a86250e 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -18,8 +18,8 @@ struct ConfigPerfItem; m4_dnl Define the context options only once m4_define(`EXEC_CONTEXT_CONFIG_ITEMS', `$1.WorkingDirectory, config_parse_working_directory, 0, offsetof($1, exec_context) -$1.RootDirectory, config_parse_unit_path_printf, 0, offsetof($1, exec_context.root_directory) -$1.RootImage, config_parse_unit_path_printf, 0, offsetof($1, exec_context.root_image) +$1.RootDirectory, config_parse_unit_path_printf, true, offsetof($1, exec_context.root_directory) +$1.RootImage, config_parse_unit_path_printf, true, offsetof($1, exec_context.root_image) $1.User, config_parse_user_group, 0, offsetof($1, exec_context.user) $1.Group, config_parse_user_group, 0, offsetof($1, exec_context.group) $1.SupplementaryGroups, config_parse_user_group_strv, 0, offsetof($1, exec_context.supplementary_groups) @@ -35,7 +35,7 @@ $1.UMask, config_parse_mode, 0, $1.Environment, config_parse_environ, 0, offsetof($1, exec_context.environment) $1.EnvironmentFile, config_parse_unit_env_file, 0, offsetof($1, exec_context.environment_files) $1.PassEnvironment, config_parse_pass_environ, 0, offsetof($1, exec_context.pass_environment) -$1.DynamicUser, config_parse_bool, 0, offsetof($1, exec_context.dynamic_user) +$1.DynamicUser, config_parse_bool, true, offsetof($1, exec_context.dynamic_user) $1.StandardInput, config_parse_exec_input, 0, offsetof($1, exec_context) $1.StandardOutput, config_parse_exec_output, 0, offsetof($1, exec_context) $1.StandardError, config_parse_exec_output, 0, offsetof($1, exec_context) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 49a5c53f96..9d5c39b3dd 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -242,6 +242,7 @@ int config_parse_unit_path_printf( _cleanup_free_ char *k = NULL; Unit *u = userdata; int r; + bool fatal = ltype; assert(filename); assert(lvalue); @@ -250,8 +251,10 @@ int config_parse_unit_path_printf( r = unit_full_printf(u, rvalue, &k); if (r < 0) { - log_syntax(unit, LOG_ERR, filename, line, r, "Failed to resolve unit specifiers on %s, ignoring: %m", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, r, + "Failed to resolve unit specifiers on %s%s: %m", + fatal ? "" : ", ignoring", rvalue); + return fatal ? -ENOEXEC : 0; } return config_parse_path(unit, filename, line, section, section_line, lvalue, ltype, k, data, userdata); diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index 44df7493e2..e08402e3d2 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -615,6 +615,7 @@ int config_parse_bool(const char* unit, int k; bool *b = data; + bool fatal = ltype; assert(filename); assert(lvalue); @@ -623,8 +624,10 @@ int config_parse_bool(const char* unit, k = parse_boolean(rvalue); if (k < 0) { - log_syntax(unit, LOG_ERR, filename, line, k, "Failed to parse boolean value, ignoring: %s", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, k, + "Failed to parse boolean value%s: %s", + fatal ? "" : ", ignoring", rvalue); + return fatal ? -ENOEXEC : 0; } *b = !!k; @@ -715,6 +718,7 @@ int config_parse_path( void *userdata) { char **s = data, *n; + bool fatal = ltype; assert(filename); assert(lvalue); @@ -723,12 +727,14 @@ int config_parse_path( if (!utf8_is_valid(rvalue)) { log_syntax_invalid_utf8(unit, LOG_ERR, filename, line, rvalue); - return 0; + return fatal ? -ENOEXEC : 0; } if (!path_is_absolute(rvalue)) { - log_syntax(unit, LOG_ERR, filename, line, 0, "Not an absolute path, ignoring: %s", rvalue); - return 0; + log_syntax(unit, LOG_ERR, filename, line, 0, + "Not an absolute path%s: %s", + fatal ? "" : ", ignoring", rvalue); + return fatal ? -ENOEXEC : 0; } n = strdup(rvalue); From b02385688476d1cc940ca682e7f3ddcee8d0b236 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 11 Jul 2017 13:36:15 -0400 Subject: [PATCH 3/3] man: add warnings that Private*= settings are not always applied --- man/systemd.exec.xml | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index c31ab980fc..bd259708f5 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -1038,14 +1038,19 @@ After= dependencies on all mount units necessary to access /tmp and /var/tmp. Moreover an implicitly After= ordering on systemd-tmpfiles-setup.service8 - is added. + is added. + + Note that the implementation of this setting might be impossible (for example if mount namespaces + are not available), and the unit should be written in a way that does not solely rely on this setting for + security. PrivateDevices= - Takes a boolean argument. If true, sets up a new /dev namespace for the executed processes and - only adds API pseudo devices such as /dev/null, /dev/zero or + Takes a boolean argument. If true, sets up a new /dev mount for the + executed processes and only adds API pseudo devices such as /dev/null, + /dev/zero or /dev/random (as well as the pseudo TTY subsystem) to it, but no physical devices such as /dev/sda, system memory /dev/mem, system ports /dev/port and others. This is useful to securely turn off physical device access by the @@ -1056,8 +1061,8 @@ systemd.resource-control5 for details). Note that using this setting will disconnect propagation of mounts from the service to the host (propagation in the opposite direction continues to work). This means that this setting may not be used for - services which shall be able to install mount points in the main mount namespace. The /dev namespace will be - mounted read-only and 'noexec'. The latter may break old programs which try to set up executable memory by + services which shall be able to install mount points in the main mount namespace. The new /dev + will be mounted read-only and 'noexec'. The latter may break old programs which try to set up executable memory by using mmap2 of /dev/zero instead of using MAP_ANON. This setting is implied if DynamicUser= is set. For this setting the same restrictions regarding mount propagation and @@ -1065,7 +1070,11 @@ If turned on and if running in user mode, or in system mode, but without the CAP_SYS_ADMIN capability (e.g. setting User=), NoNewPrivileges=yes is implied. - + + + Note that the implementation of this setting might be impossible (for example if mount namespaces + are not available), and the unit should be written in a way that does not solely rely on this setting for + security. @@ -1076,7 +1085,7 @@ configures only the loopback network device lo inside it. No other network devices will be available to the executed process. This is useful to - securely turn off network access by the executed process. + turn off network access by the executed process. Defaults to false. It is possible to run two or more units within the same private network namespace by using the JoinsNamespaceOf= directive, see @@ -1086,7 +1095,11 @@ The latter has the effect that AF_UNIX sockets in the abstract socket namespace will become unavailable to the processes (however, those located in the file system will continue to be - accessible). + accessible). + + Note that the implementation of this setting might be impossible (for example if network namespaces + are not available), and the unit should be written in a way that does not solely rely on this setting for + security. @@ -1108,7 +1121,11 @@ This setting is particularly useful in conjunction with RootDirectory=/RootImage=, as the need to synchronize the user and group databases in the root directory and on the host is reduced, as the only users and groups who need to be matched - are root, nobody and the unit's own user and group. + are root, nobody and the unit's own user and group. + + Note that the implementation of this setting might be impossible (for example if user namespaces + are not available), and the unit should be written in a way that does not solely rely on this setting for + security.