| If a user is created with a strictly-speaking invalid name such as '0day' and a |
| unit created to run as that user, systemd rejects the username and runs the unit |
| as root. |
| |
| CVE: CVE-2017-1000082 |
| Upstream-Status: Backport |
| Signed-off-by: Ross Burton <ross.burton@intel.com> |
| |
| From d8e1310e1ed7b6f122bc7eb8ba061fbd088783c0 Mon Sep 17 00:00:00 2001 |
| From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl> |
| Date: Thu, 6 Jul 2017 13:28:19 -0400 |
| Subject: [PATCH] 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. |
| |
| Signed-off-by: Ross Burton <ross.burton@intel.com> |
| --- |
| src/core/load-fragment.c | 104 ++++++++++++++++++++++++++++------------------ |
| src/test/test-unit-file.c | 14 +++---- |
| 2 files changed, 70 insertions(+), 48 deletions(-) |
| |
| diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c |
| index cbc826809..2047974f4 100644 |
| --- a/src/core/load-fragment.c |
| +++ b/src/core/load-fragment.c |
| @@ -630,20 +630,28 @@ int config_parse_exec( |
| |
| if (isempty(f)) { |
| /* 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(f)) { |
| - 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(f)) { |
| - 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(f, "/")) { |
| - 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 (f == firstword) { |
| @@ -699,7 +707,7 @@ int config_parse_exec( |
| if (r == 0) |
| break; |
| else if (r < 0) |
| - return 0; |
| + return ignore ? 0 : -ENOEXEC; |
| |
| if (!GREEDY_REALLOC(n, nbufsize, nlen + 2)) |
| return log_oom(); |
| @@ -709,8 +717,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); |
| @@ -1315,8 +1325,10 @@ int config_parse_exec_selinux_context( |
| |
| r = unit_name_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); |
| @@ -1363,8 +1375,10 @@ int config_parse_exec_apparmor_profile( |
| |
| r = unit_name_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); |
| @@ -1411,8 +1425,10 @@ int config_parse_exec_smack_process_label( |
| |
| r = unit_name_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); |
| @@ -1630,19 +1646,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); |
| @@ -1893,13 +1909,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; |
| @@ -1957,19 +1973,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); |
| @@ -2128,25 +2144,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; |
| @@ -4228,8 +4247,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 12f48bf43..fd797b587 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 */"); |
| -- |
| 2.11.0 |