From: Maanya Goenka <t-magoenka@microsoft.com>
Date: Tue, 17 Aug 2021 17:25:38 +0000 (-0700)
Subject: systemd-analyze: 'security' option to perform offline reviews of the specified unit... 
X-Git-Tag: v250-rc1~794^2~1
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=bb43d853190052b3d2984ae08299ddf0a97b86f5;p=thirdparty%2Fsystemd.git

systemd-analyze: 'security' option to perform offline reviews of the specified unit file(s)

New option --offline which works with the 'security' command and takes in a boolean value. When set to true,
it performs an offline security review of the specified unit file(s). It does not rely on PID 1 to acquire
security information for the files like 'security' when used by itself does. It makes use of the refactored
security_info struct instead (commit #8cd669d3d3cf1b5e8667acc46ba290a9e8a8e529). This means that --offline can be
used with --image and --root as well. When used with --threshold, if a unit's overall exposure level is above
that set by the user, the default value being 100, --offline returns a non-zero exit status.

Example Run:

1. testcase.service is a unit file created for testing the --offline option

maanya-goenka@debian:~/systemd (systemd-security)$ cat<<EOF>testcase.service

> [Service]
> ExecStart = echo hello
> EOF

For the purposes of this demo, the security table outputted below has been cut to show only the first two security settings.

maanya-goenka@debian:~/systemd (systemd-security)$ sudo build/systemd-analyze security --offline=true testcase.service
/usr/lib/systemd/system/plymouth-start.service:15: Unit configured to use KillMode=none. This is unsafe, as it disables systemd's
process lifecycle management for the service. Please update your service to use a safer KillMode=, such as 'mixed' or 'control-group'.
Support for KillMode=none is deprecated and will eventually be removed.
/usr/lib/systemd/system/gdm.service:30: Standard output type syslog is obsolete, automatically updating to journal. Please update your
unit file, and consider removing the setting altogether.
/usr/lib/systemd/system/dbus.socket:5: ListenStream= references a path below legacy directory /var/run/, updating
/var/run/dbus/system_bus_socket → /run/dbus/system_bus_socket; please update the unit file accordingly.

  NAME                                        DESCRIPTION                                                       EXPOSURE
✗ PrivateNetwork=                             Service has access to the host's network                          0.5
✗ User=/DynamicUser=                          Service runs as root user                                         0.4

→ Overall exposure level for testcase.service: 9.6 UNSAFE 😨

maanya-goenka@debian:~/systemd (systemd-security)$ echo $? 0

2. The testcase.service unit file is modified to set PrivateNetwork to "yes". This reduces the exposure level from 9.6 to 9.1.

maanya-goenka@debian:~/systemd (systemd-security)$ nano testcase.service

> [Service]
> ExecStart = echo hello
> PrivateNetwork = yes
> EOF

maanya-goenka@debian:~/systemd (systemd-security)$ sudo build/systemd-analyze security --offline=true testcase.service
/usr/lib/systemd/system/plymouth-start.service:15: Unit configured to use KillMode=none. This is unsafe, as it disables systemd's
process lifecycle management for the service. Please update your service to use a safer KillMode=, such as 'mixed' or 'control-group'.
Support for KillMode=none is deprecated and will eventually be removed.
/usr/lib/systemd/system/gdm.service:30: Standard output type syslog is obsolete, automatically updating to journal. Please update your
unit file, and consider removing the setting altogether.
/usr/lib/systemd/system/dbus.socket:5: ListenStream= references a path below legacy directory /var/run/, updating
/var/run/dbus/system_bus_socket → /run/dbus/system_bus_socket; please update the unit file accordingly.

  NAME                                        DESCRIPTION                                                       EXPOSURE
✓ PrivateNetwork=                             Service has access to the host's network
✗ User=/DynamicUser=                          Service runs as root user                                         0.4

→ Overall exposure level for testcase.service: 9.1 UNSAFE 😨

maanya-goenka@debian:~/systemd (systemd-security)$ echo $? 0

3. Next, we use the same testcase.service unit file but add the additional --threshold=60 option to see how --threshold works with
--offline. Since the overall exposure level is 91 which is greater than the threshold value set by the user (= 60), we can expect
a non-zero exit status.

maanya-goenka@debian:~/systemd (systemd-security)$ sudo build/systemd-analyze security --offline=true --threshold=60 testcase.service
/usr/lib/systemd/system/plymouth-start.service:15: Unit configured to use KillMode=none. This is unsafe, as it disables systemd's
process lifecycle management for the service. Please update your service to use a safer KillMode=, such as 'mixed' or 'control-group'.
Support for KillMode=none is deprecated and will eventually be removed.
/usr/lib/systemd/system/gdm.service:30: Standard output type syslog is obsolete, automatically updating to journal. Please update your
unit file, and consider removing the setting altogether.
/usr/lib/systemd/system/dbus.socket:5: ListenStream= references a path below legacy directory /var/run/, updating
/var/run/dbus/system_bus_socket → /run/dbus/system_bus_socket; please update the unit file accordingly.

  NAME                                        DESCRIPTION                                                       EXPOSURE
✓ PrivateNetwork=                             Service has access to the host's network
✗ User=/DynamicUser=                          Service runs as root user                                         0.4

→ Overall exposure level for testcase.service: 9.1 UNSAFE 😨

maanya-goenka@debian:~/systemd (systemd-security)$ echo $? 1
---

diff --git a/man/systemd-analyze.xml b/man/systemd-analyze.xml
index 48976f52bf9..3c276360cd5 100644
--- a/man/systemd-analyze.xml
+++ b/man/systemd-analyze.xml
@@ -770,6 +770,18 @@ Service b@0.service not loaded, b.socket cannot be started.
         operate on files inside the specified image path <replaceable>PATH</replaceable>.</para></listitem>
       </varlistentry>
 
+      <varlistentry>
+        <term><option>--offline=<replaceable>BOOL</replaceable></option></term>
+
+        <listitem><para>With <command>security</command>, perform an offline security review
+        of the specified unit file(s), i.e. does not have to rely on PID 1 to acquire security
+        information for the files like the <command>security</command> verb when used by itself does.
+        This means that <option>--offline=</option> can be used with <option>--root=</option> and
+        <option>--image=</option> as well. If a unit's overall exposure level is above that set by
+        <option>--threshold=</option> (default value is 100), <option>--offline=</option> will return
+        an error.</para></listitem>
+      </varlistentry>
+
       <varlistentry>
         <term><option>--iterations=<replaceable>NUMBER</replaceable></option></term>
 
diff --git a/shell-completion/bash/systemd-analyze b/shell-completion/bash/systemd-analyze
index 1b9447a125b..b7b92f0e00e 100644
--- a/shell-completion/bash/systemd-analyze
+++ b/shell-completion/bash/systemd-analyze
@@ -144,7 +144,7 @@ _systemd_analyze() {
 
     elif __contains_word "$verb" ${VERBS[SECURITY]}; then
         if [[ $cur = -* ]]; then
-            comps='--help --version --no-pager --system --user -H --host -M --machine'
+            comps='--help --version --no-pager --system --user -H --host -M --machine --offline'
         else
             if __contains_word "--user" ${COMP_WORDS[*]}; then
                 mode=--user
diff --git a/shell-completion/zsh/_systemd-analyze b/shell-completion/zsh/_systemd-analyze
index 0dd080afb74..6db4da6a35e 100644
--- a/shell-completion/zsh/_systemd-analyze
+++ b/shell-completion/zsh/_systemd-analyze
@@ -90,6 +90,7 @@ _arguments \
     '--root=[Add support for root argument]:PATH' \
     '--image=[Add support for discrete images]:PATH' \
     '--recursive-errors=[When verifying a unit, control dependency verification]:MODE' \
+    '--offline=[Perform a security review of the specified unit file(s)]:BOOL' \
     '--no-pager[Do not pipe output into a pager]' \
     '--man=[Do (not) check for existence of man pages]:boolean:(1 0)' \
     '--order[When generating graph for dot, show only order]' \
diff --git a/src/analyze/analyze-security.c b/src/analyze/analyze-security.c
index 36b6e5183be..6daa18ac1fa 100644
--- a/src/analyze/analyze-security.c
+++ b/src/analyze/analyze-security.c
@@ -4,6 +4,7 @@
 
 #include "af-list.h"
 #include "analyze-security.h"
+#include "analyze-verify.h"
 #include "bus-error.h"
 #include "bus-map-properties.h"
 #include "bus-unit-util.h"
@@ -13,6 +14,7 @@
 #include "in-addr-util.h"
 #include "locale-util.h"
 #include "macro.h"
+#include "manager.h"
 #include "missing_capability.h"
 #include "missing_sched.h"
 #include "nulstr-util.h"
@@ -2388,12 +2390,100 @@ static int get_security_info(Unit *u, ExecContext *c, CGroupContext *g, Security
         return 0;
 }
 
-int analyze_security(sd_bus *bus, char **units, AnalyzeSecurityFlags flags) {
+static int offline_security_check(Unit *u) {
+        _cleanup_(table_unrefp) Table *overview_table = NULL;
+        AnalyzeSecurityFlags flags = 0;
+        _cleanup_(security_info_freep) SecurityInfo *info = NULL;
+        int r;
+
+        assert(u);
+
+        if (DEBUG_LOGGING)
+                unit_dump(u, stdout, "\t");
+
+        r = get_security_info(u, unit_get_exec_context(u), unit_get_cgroup_context(u), &info);
+        if (r < 0)
+              return r;
+
+        return assess(info, overview_table, flags);
+}
+
+static int offline_security_checks(char **filenames, UnitFileScope scope, bool check_man, bool run_generators, const char *root) {
+        const ManagerTestRunFlags flags =
+                MANAGER_TEST_RUN_MINIMAL |
+                MANAGER_TEST_RUN_ENV_GENERATORS |
+                run_generators * MANAGER_TEST_RUN_GENERATORS;
+
+        _cleanup_(manager_freep) Manager *m = NULL;
+        Unit *units[strv_length(filenames)];
+        _cleanup_free_ char *var = NULL;
+        int r, k;
+        size_t count = 0;
+        char **filename;
+
+        if (strv_isempty(filenames))
+                return 0;
+
+        /* set the path */
+        r = verify_generate_path(&var, filenames);
+        if (r < 0)
+                return log_error_errno(r, "Failed to generate unit load path: %m");
+
+        assert_se(set_unit_path(var) >= 0);
+
+        r = manager_new(scope, flags, &m);
+        if (r < 0)
+                return log_error_errno(r, "Failed to initialize manager: %m");
+
+        log_debug("Starting manager...");
+
+        r = manager_startup(m, /* serialization= */ NULL, /* fds= */ NULL, root);
+        if (r < 0)
+                return r;
+
+        log_debug("Loading remaining units from the command line...");
+
+        STRV_FOREACH(filename, filenames) {
+                _cleanup_free_ char *prepared = NULL;
+
+                log_debug("Handling %s...", *filename);
+
+                k = verify_prepare_filename(*filename, &prepared);
+                if (k < 0) {
+                        log_warning_errno(k, "Failed to prepare filename %s: %m", *filename);
+                        if (r == 0)
+                                r = k;
+                        continue;
+                }
+
+                k = manager_load_startable_unit_or_warn(m, NULL, prepared, &units[count]);
+                if (k < 0) {
+                        if (r == 0)
+                                r = k;
+                        continue;
+                }
+
+                count++;
+        }
+
+        for (size_t i = 0; i < count; i++) {
+                k = offline_security_check(units[i]);
+                if (k < 0 && r == 0)
+                        r = k;
+        }
+
+        return r;
+}
+
+int analyze_security(sd_bus *bus, char **units, UnitFileScope scope, bool check_man, bool run_generators, bool offline, const char *root, AnalyzeSecurityFlags flags) {
         _cleanup_(table_unrefp) Table *overview_table = NULL;
         int ret = 0, r;
 
         assert(bus);
 
+        if (offline)
+                return offline_security_checks(units, scope, check_man, run_generators, root);
+
         if (strv_length(units) != 1) {
                 overview_table = table_new("unit", "exposure", "predicate", "happy");
                 if (!overview_table)
diff --git a/src/analyze/analyze-security.h b/src/analyze/analyze-security.h
index e8de39f3bc2..b9ea2586b92 100644
--- a/src/analyze/analyze-security.h
+++ b/src/analyze/analyze-security.h
@@ -1,12 +1,16 @@
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 #pragma once
 
+#include <stdbool.h>
+
 #include "sd-bus.h"
 
+#include "unit-file.h"
+
 typedef enum AnalyzeSecurityFlags {
         ANALYZE_SECURITY_SHORT             = 1 << 0,
         ANALYZE_SECURITY_ONLY_LOADED       = 1 << 1,
         ANALYZE_SECURITY_ONLY_LONG_RUNNING = 1 << 2,
 } AnalyzeSecurityFlags;
 
-int analyze_security(sd_bus *bus, char **units, AnalyzeSecurityFlags flags);
+int analyze_security(sd_bus *bus, char **units, UnitFileScope scope, bool check_man, bool run_generators, bool offline, const char *root, AnalyzeSecurityFlags flags);
diff --git a/src/analyze/analyze-verify.c b/src/analyze/analyze-verify.c
index cd5377200b0..2a436c545ef 100644
--- a/src/analyze/analyze-verify.c
+++ b/src/analyze/analyze-verify.c
@@ -33,7 +33,7 @@ static void log_syntax_callback(const char *unit, int level, void *userdata) {
         }
 }
 
-static int prepare_filename(const char *filename, char **ret) {
+int verify_prepare_filename(const char *filename, char **ret) {
         int r;
         const char *name;
         _cleanup_free_ char *abspath = NULL;
@@ -70,7 +70,7 @@ static int prepare_filename(const char *filename, char **ret) {
         return 0;
 }
 
-static int generate_path(char **var, char **filenames) {
+int verify_generate_path(char **var, char **filenames) {
         const char *old;
         char **filename;
 
@@ -266,7 +266,7 @@ int verify_units(char **filenames, UnitFileScope scope, bool check_man, bool run
         set_log_syntax_callback(log_syntax_callback, &s);
 
         /* set the path */
-        r = generate_path(&var, filenames);
+        r = verify_generate_path(&var, filenames);
         if (r < 0)
                 return log_error_errno(r, "Failed to generate unit load path: %m");
 
@@ -291,7 +291,7 @@ int verify_units(char **filenames, UnitFileScope scope, bool check_man, bool run
 
                 log_debug("Handling %s...", *filename);
 
-                k = prepare_filename(*filename, &prepared);
+                k = verify_prepare_filename(*filename, &prepared);
                 if (k < 0) {
                         log_error_errno(k, "Failed to prepare filename %s: %m", *filename);
                         if (r == 0)
diff --git a/src/analyze/analyze-verify.h b/src/analyze/analyze-verify.h
index 09d3aea02c9..47b78a81589 100644
--- a/src/analyze/analyze-verify.h
+++ b/src/analyze/analyze-verify.h
@@ -14,6 +14,8 @@ typedef enum RecursiveErrors {
         _RECURSIVE_ERRORS_INVALID = -EINVAL,
 } RecursiveErrors;
 
+int verify_generate_path(char **var, char **filenames);
+int verify_prepare_filename(const char *filename, char **ret);
 int verify_executable(Unit *u, const ExecCommand *exec, const char *root);
 int verify_units(char **filenames, UnitFileScope scope, bool check_man, bool run_generators, RecursiveErrors recursive_errors, const char *root);
 
diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c
index ceb18db7406..8db03f0eda2 100644
--- a/src/analyze/analyze.c
+++ b/src/analyze/analyze.c
@@ -91,6 +91,7 @@ static bool arg_man = true;
 static bool arg_generators = false;
 static char *arg_root = NULL;
 static char *arg_image = NULL;
+static bool arg_offline = false;
 static unsigned arg_iterations = 1;
 static usec_t arg_base_time = USEC_INFINITY;
 
@@ -2160,7 +2161,7 @@ static int do_security(int argc, char *argv[], void *userdata) {
 
         (void) pager_open(arg_pager_flags);
 
-        return analyze_security(bus, strv_skip(argv, 1), 0);
+        return analyze_security(bus, strv_skip(argv, 1), arg_scope, arg_man, arg_generators, arg_offline, arg_root, 0);
 }
 
 static int help(int argc, char *argv[], void *userdata) {
@@ -2208,6 +2209,7 @@ static int help(int argc, char *argv[], void *userdata) {
                "\nOptions:\n"
                "  -h --help                  Show this help\n"
                "     --recursive-errors=MODE Control which units are verified\n"
+               "     --offline=BOOL          Perform a security review on unit file(s)\n"
                "     --version               Show package version\n"
                "     --no-pager              Do not pipe output into a pager\n"
                "     --system                Operate on system systemd instance\n"
@@ -2259,6 +2261,7 @@ static int parse_argv(int argc, char *argv[]) {
                 ARG_ITERATIONS,
                 ARG_BASE_TIME,
                 ARG_RECURSIVE_ERRORS,
+                ARG_OFFLINE,
         };
 
         static const struct option options[] = {
@@ -2269,6 +2272,7 @@ static int parse_argv(int argc, char *argv[]) {
                 { "root",             required_argument, NULL, ARG_ROOT             },
                 { "image",            required_argument, NULL, ARG_IMAGE            },
                 { "recursive-errors", required_argument, NULL, ARG_RECURSIVE_ERRORS },
+                { "offline",          required_argument, NULL, ARG_OFFLINE          },
                 { "system",           no_argument,       NULL, ARG_SYSTEM           },
                 { "user",             no_argument,       NULL, ARG_USER             },
                 { "global",           no_argument,       NULL, ARG_GLOBAL           },
@@ -2387,6 +2391,12 @@ static int parse_argv(int argc, char *argv[]) {
                                 return r;
                         break;
 
+                case ARG_OFFLINE:
+                        r = parse_boolean_argument("--offline", optarg, &arg_offline);
+                        if (r < 0)
+                                return r;
+                        break;
+
                 case ARG_ITERATIONS:
                         r = safe_atou(optarg, &arg_iterations);
                         if (r < 0)
@@ -2408,6 +2418,10 @@ static int parse_argv(int argc, char *argv[]) {
                         assert_not_reached();
                 }
 
+        if (arg_offline && !streq_ptr(argv[optind], "security"))
+                return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
+                                       "Option --offline= is only supported for security right now.");
+
         if (arg_scope == UNIT_FILE_GLOBAL &&
             !STR_IN_SET(argv[optind] ?: "time", "dot", "unit-paths", "verify"))
                 return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
@@ -2417,9 +2431,10 @@ static int parse_argv(int argc, char *argv[]) {
                 return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
                                        "Option --user is not supported for cat-config right now.");
 
-        if ((arg_root || arg_image) && !STRPTR_IN_SET(argv[optind], "cat-config", "verify"))
+        if ((arg_root || arg_image) && (!STRPTR_IN_SET(argv[optind], "cat-config", "verify")) &&
+           (!(streq_ptr(argv[optind], "security") && arg_offline)))
                 return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
-                                       "Options --root= and --image= are only supported for cat-config and verify right now.");
+                                       "Options --root= and --image= are only supported for cat-config, verify and security when used with --offline= right now.");
 
         /* Having both an image and a root is not supported by the code */
         if (arg_root && arg_image)