Merge pull request #6300 from keszybz/refuse-to-load-some-units

Refuse to load some units

5 files changed, 123 insertions, 71 deletions

diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml
index a4f92775a..d28de2d0f 100644
--- a/man/systemd.exec.xml
+++ b/man/systemd.exec.xml
@@ -1053,14 +1053,19 @@
         <varname>After=</varname> dependencies on all mount units necessary to access <filename>/tmp</filename> and
         <filename>/var/tmp</filename>. Moreover an implicitly <varname>After=</varname> ordering on
         <citerefentry><refentrytitle>systemd-tmpfiles-setup.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>
-        is added.</para></listitem>
+        is added.</para>
+
+        <para>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.</para></listitem>
       </varlistentry>
 
       <varlistentry>
         <term><varname>PrivateDevices=</varname></term>
 
-        <listitem><para>Takes a boolean argument. If true, sets up a new /dev namespace for the executed processes and
-        only adds API pseudo devices such as <filename>/dev/null</filename>, <filename>/dev/zero</filename> or
+        <listitem><para>Takes a boolean argument. If true, sets up a new <filename>/dev</filename> mount for the
+        executed processes and only adds API pseudo devices such as <filename>/dev/null</filename>,
+        <filename>/dev/zero</filename> or
         <filename>/dev/random</filename> (as well as the pseudo TTY subsystem) to it, but no physical devices such as
         <filename>/dev/sda</filename>, system memory <filename>/dev/mem</filename>, system ports
         <filename>/dev/port</filename> and others. This is useful to securely turn off physical device access by the
@@ -1071,8 +1076,8 @@
         <citerefentry><refentrytitle>systemd.resource-control</refentrytitle><manvolnum>5</manvolnum></citerefentry>
         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 <filename>/dev</filename>
+        will be mounted read-only and 'noexec'. The latter may break old programs which try to set up executable memory by
         using <citerefentry><refentrytitle>mmap</refentrytitle><manvolnum>2</manvolnum></citerefentry> of
         <filename>/dev/zero</filename> instead of using <constant>MAP_ANON</constant>. This setting is implied if
         <varname>DynamicUser=</varname> is set. For this setting the same restrictions regarding mount propagation and
@@ -1080,7 +1085,11 @@
         If turned on and if running in user mode, or in system mode, but without the <constant>CAP_SYS_ADMIN</constant>
         capability (e.g. setting <varname>User=</varname>), <varname>NoNewPrivileges=yes</varname>
         is implied.
-        </para></listitem>
+        </para>
+
+        <para>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.</para></listitem>
       </varlistentry>
 
       <varlistentry>
@@ -1091,7 +1100,7 @@
         configures only the loopback network device
         <literal>lo</literal> 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
         <varname>JoinsNamespaceOf=</varname> directive, see
@@ -1101,7 +1110,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).</para></listitem>
+        accessible).</para>
+
+        <para>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.</para></listitem>
       </varlistentry>
 
       <varlistentry>
@@ -1123,7 +1136,11 @@
         <para>This setting is particularly useful in conjunction with
         <varname>RootDirectory=</varname>/<varname>RootImage=</varname>, 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 <literal>root</literal>, <literal>nobody</literal> and the unit's own user and group.</para></listitem>
+        are <literal>root</literal>, <literal>nobody</literal> and the unit's own user and group.</para>
+
+        <para>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.</para></listitem>
       </varlistentry>
 
       <varlistentry>
diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4
index c43b7885b..5b5a86250 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 6a6dadda2..9d5c39b3d 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);
@@ -636,26 +639,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 +721,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 +739,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 +1349,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 +1399,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 +1449,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 +1670,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 +1930,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 +1994,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 +2165,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 +4482,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/shared/conf-parser.c b/src/shared/conf-parser.c
index 44df7493e..e08402e3d 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);
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 */");