]> git.ipfire.org Git - thirdparty/systemd.git/commitdiff
core: cache unit file selinux label, and make decisions based on that
authorLennart Poettering <lennart@poettering.net>
Tue, 12 Jul 2022 14:13:40 +0000 (16:13 +0200)
committerLennart Poettering <lennart@poettering.net>
Wed, 20 Jul 2022 17:08:28 +0000 (19:08 +0200)
Do not go back to disk on each selinux access, but instead cache the
label off the inode we are actually reading. That way unit file contents
and unit file label we use for access checks are always in sync.

Based on discussions here:

https://github.com/systemd/systemd/pull/10023#issuecomment-1179835586

Replaces:

https://github.com/systemd/systemd/pull/23910

This changes behaviour a bit, because we'll reach and cache the label at
the moment of loading the unit (i.e. usually on boot and reload), but
not after relabelling. Thus, users must refresh the cache explicitly via
a "systemctl daemon-reload" if they relabelled things.

This makes the SELinux story a bit more debuggable, as it adds an
AccessSELinuxContext bus property to units that will report the label we are
using for a unit (or the empty string if not known).

This also drops using the "source" path of a unit as label source. if
there's value in it, then generators should manually copy the selinux
label from the source files onto the generated unit files, so that the
rule that "access labels are read when we read the definition files" is
upheld. But I am not convinced this is really a necessary, good idea.

man/org.freedesktop.systemd1.xml
src/core/dbus-unit.c
src/core/load-fragment.c
src/core/selinux-access.c
src/core/selinux-access.h
src/core/unit-serialize.c
src/core/unit.c
src/core/unit.h

index 7803cc9cf9334c125493029d7dbf0018bf139f78..f821d6562a64bfe075a089994fee9a25d7532f9f 100644 (file)
@@ -1650,6 +1650,11 @@ node /org/freedesktop/systemd1 {
       that the root path is encoded as the empty string here (not as <literal>/</literal>!), so that it can be
       appended to <filename>/sys/fs/cgroup/systemd</filename> easily. This value will be set to the empty
       string for the host instance and some other string for container instances.</para>
+
+      <para><varname>AccessSELinuxContext</varname> contains the SELinux context that is used to control
+      access to the unit. It's read from the unit file when it is loaded and cached until the service manager
+      is reloaded. This property contains an empty string if SELinux is not used or if no label could be read
+      (for example because the unit is not backed by a file on disk).</para>
     </refsect2>
 
     <refsect2>
@@ -1783,6 +1788,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice {
       @org.freedesktop.DBus.Property.EmitsChangedSignal("const")
       readonly s Description = '...';
       @org.freedesktop.DBus.Property.EmitsChangedSignal("const")
+      readonly s AccessSELinuxContext = '...';
+      @org.freedesktop.DBus.Property.EmitsChangedSignal("const")
       readonly s LoadState = '...';
       readonly s ActiveState = '...';
       readonly s FreezerState = '...';
@@ -2090,6 +2097,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice {
 
     <variablelist class="dbus-property" generated="True" extra-ref="Description"/>
 
+    <variablelist class="dbus-property" generated="True" extra-ref="AccessSELinuxContext"/>
+
     <variablelist class="dbus-property" generated="True" extra-ref="LoadState"/>
 
     <variablelist class="dbus-property" generated="True" extra-ref="ActiveState"/>
index aaa72cec80f9343dfe803695a2362d6b40ce5b22..3c5e62ae4bf8d058eeb749ccd890cf558738a510 100644 (file)
@@ -894,6 +894,7 @@ const sd_bus_vtable bus_unit_vtable[] = {
         SD_BUS_PROPERTY("RequiresMountsFor", "as", property_get_requires_mounts_for, offsetof(Unit, requires_mounts_for), SD_BUS_VTABLE_PROPERTY_CONST),
         SD_BUS_PROPERTY("Documentation", "as", NULL, offsetof(Unit, documentation), SD_BUS_VTABLE_PROPERTY_CONST),
         SD_BUS_PROPERTY("Description", "s", property_get_description, 0, SD_BUS_VTABLE_PROPERTY_CONST),
+        SD_BUS_PROPERTY("AccessSELinuxContext", "s", NULL, offsetof(Unit, access_selinux_context), SD_BUS_VTABLE_PROPERTY_CONST),
         SD_BUS_PROPERTY("LoadState", "s", property_get_load_state, offsetof(Unit, load_state), SD_BUS_VTABLE_PROPERTY_CONST),
         SD_BUS_PROPERTY("ActiveState", "s", property_get_active_state, 0, SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
         SD_BUS_PROPERTY("FreezerState", "s", property_get_freezer_state, 0, SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
index 92e99e7d536d88e6af3a3a7a3c5f28563e563d4f..d16c7b1df2b6919b9c71b5774700d4592c8ba975 100644 (file)
@@ -58,6 +58,7 @@
 #include "seccomp-util.h"
 #endif
 #include "securebits-util.h"
+#include "selinux-util.h"
 #include "signal-util.h"
 #include "socket-netlink.h"
 #include "specifier.h"
@@ -6096,6 +6097,7 @@ int unit_load_fragment(Unit *u) {
         assert(u->id);
 
         if (u->transient) {
+                u->access_selinux_context = mfree(u->access_selinux_context);
                 u->load_state = UNIT_LOADED;
                 return 0;
         }
@@ -6141,7 +6143,24 @@ int unit_load_fragment(Unit *u) {
 
                         u->load_state = u->perpetual ? UNIT_LOADED : UNIT_MASKED; /* don't allow perpetual units to ever be masked */
                         u->fragment_mtime = 0;
+                        u->access_selinux_context = mfree(u->access_selinux_context);
                 } else {
+#if HAVE_SELINUX
+                        if (mac_selinux_use()) {
+                                _cleanup_freecon_ char *selcon = NULL;
+
+                                /* Cache the SELinux context of the unit file here. We'll make use of when checking access permissions to loaded units */
+                                r = fgetfilecon_raw(fileno(f), &selcon);
+                                if (r < 0)
+                                        log_unit_warning_errno(u, r, "Failed to read SELinux context of '%s', ignoring: %m", fragment);
+
+                                r = free_and_strdup(&u->access_selinux_context, selcon);
+                                if (r < 0)
+                                        return r;
+                        } else
+#endif
+                                u->access_selinux_context = mfree(u->access_selinux_context);
+
                         u->load_state = UNIT_LOADED;
                         u->fragment_mtime = timespec_load(&st.st_mtim);
 
index d77901caa5f2c3d992162f5b1ad62f225e9d61ec..878dea13f13888db4c9c8823a7c26a6e4aa651fb 100644 (file)
@@ -179,13 +179,14 @@ static int access_init(sd_bus_error *error) {
 */
 int mac_selinux_access_check_internal(
                 sd_bus_message *message,
-                const char *path,
+                const char *unit_path,
+                const char *unit_context,
                 const char *permission,
                 const char *function,
                 sd_bus_error *error) {
 
         _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL;
-        const char *tclass, *scon;
+        const char *tclass, *scon, *acon;
         _cleanup_free_ char *cl = NULL;
         _cleanup_freecon_ char *fcon = NULL;
         char **cmdline = NULL;
@@ -214,37 +215,22 @@ int mac_selinux_access_check_internal(
         if (r < 0)
                 return r;
 
-        /* The SELinux context is something we really should have
-         * gotten directly from the message or sender, and not be an
-         * augmented field. If it was augmented we cannot use it for
-         * authorization, since this is racy and vulnerable. Let's add
-         * an extra check, just in case, even though this really
-         * shouldn't be possible. */
+        /* The SELinux context is something we really should have gotten directly from the message or sender,
+         * and not be an augmented field. If it was augmented we cannot use it for authorization, since this
+         * is racy and vulnerable. Let's add an extra check, just in case, even though this really shouldn't
+         * be possible. */
         assert_return((sd_bus_creds_get_augmented_mask(creds) & SD_BUS_CREDS_SELINUX_CONTEXT) == 0, -EPERM);
 
         r = sd_bus_creds_get_selinux_context(creds, &scon);
         if (r < 0)
                 return r;
 
-        if (path) {
-                /* Get the file context of the unit file */
-
-                if (getfilecon_raw(path, &fcon) < 0) {
-                        r = -errno;
-
-                        log_warning_errno(r, "SELinux getfilecon_raw() on '%s' failed%s (perm=%s): %m",
-                                          path,
-                                          enforce ? "" : ", ignoring",
-                                          permission);
-                        if (!enforce)
-                                return 0;
-
-                        return sd_bus_error_setf(error, SD_BUS_ERROR_ACCESS_DENIED, "Failed to get file context on %s.", path);
-                }
-
+        if (unit_context) {
+                /* Nice! The unit comes with a SELinux context read from the unit file */
+                acon = unit_context;
                 tclass = "service";
-
         } else {
+                /* If no unit context is known, use our own */
                 if (getcon_raw(&fcon) < 0) {
                         r = -errno;
 
@@ -257,6 +243,7 @@ int mac_selinux_access_check_internal(
                         return sd_bus_error_set(error, SD_BUS_ERROR_ACCESS_DENIED, "Failed to get current context.");
                 }
 
+                acon = fcon;
                 tclass = "system";
         }
 
@@ -265,12 +252,12 @@ int mac_selinux_access_check_internal(
 
         struct audit_info audit_info = {
                 .creds = creds,
-                .path = path,
+                .path = unit_path,
                 .cmdline = cl,
                 .function = function,
         };
 
-        r = selinux_check_access(scon, fcon, tclass, permission, &audit_info);
+        r = selinux_check_access(scon, acon, tclass, permission, &audit_info);
         if (r < 0) {
                 r = errno_or_else(EPERM);
 
@@ -280,7 +267,7 @@ int mac_selinux_access_check_internal(
 
         log_full_errno_zerook(LOG_DEBUG, r,
                               "SELinux access check scon=%s tcon=%s tclass=%s perm=%s state=%s function=%s path=%s cmdline=%s: %m",
-                              scon, fcon, tclass, permission, enforce ? "enforcing" : "permissive", function, strna(path), isempty(cl) ? "n/a" : cl);
+                              scon, acon, tclass, permission, enforce ? "enforcing" : "permissive", function, strna(unit_path), strna(empty_to_null(cl)));
         return enforce ? r : 0;
 }
 
@@ -288,7 +275,8 @@ int mac_selinux_access_check_internal(
 
 int mac_selinux_access_check_internal(
                 sd_bus_message *message,
-                const char *path,
+                const char *unit_path,
+                const char *unit_label,
                 const char *permission,
                 const char *function,
                 sd_bus_error *error) {
index e5ebcb13a20cfdb70961823f918dc09e8d29980b..dc8da9e97bde56af842f40820d8f84d1cd22e620 100644 (file)
@@ -5,14 +5,10 @@
 
 #include "manager.h"
 
-int mac_selinux_access_check_internal(sd_bus_message *message,
-                                      const char *path,
-                                      const char *permission,
-                                      const char *function,
-                                      sd_bus_error *error);
+int mac_selinux_access_check_internal(sd_bus_message *message, const char *unit_path, const char *unit_label, const char *permission, const char *function, sd_bus_error *error);
 
 #define mac_selinux_access_check(message, permission, error) \
-        mac_selinux_access_check_internal((message), NULL, (permission), __func__, (error))
+        mac_selinux_access_check_internal((message), NULL, NULL, (permission), __func__, (error))
 
 #define mac_selinux_unit_access_check(unit, message, permission, error) \
-        mac_selinux_access_check_internal((message), unit_label_path(unit), (permission), __func__, (error))
+        mac_selinux_access_check_internal((message), (unit)->fragment_path, (unit)->access_selinux_context, (permission), __func__, (error))
index 65ed110ce63a6dcf5c804dd083b9ca2c75262434..3897bdee7c4215681bcd0c19be7524bb0c82500b 100644 (file)
@@ -732,6 +732,9 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) {
         STRV_FOREACH(j, u->documentation)
                 fprintf(f, "%s\tDocumentation: %s\n", prefix, *j);
 
+        if (u->access_selinux_context)
+                fprintf(f, "%s\tAccess SELinux Context: %s\n", prefix, u->access_selinux_context);
+
         following = unit_following(u);
         if (following)
                 fprintf(f, "%s\tFollowing: %s\n", prefix, following->id);
index 40fc79995adc63910c4bc9763a0cc0cd006a388f..a082741bf84f1aca2a3901d8fd88a87ba14ed2d2 100644 (file)
@@ -804,6 +804,8 @@ Unit* unit_free(Unit *u) {
         free(u->job_timeout_reboot_arg);
         free(u->reboot_arg);
 
+        free(u->access_selinux_context);
+
         set_free_free(u->aliases);
         free(u->id);
 
@@ -5550,34 +5552,6 @@ bool unit_needs_console(Unit *u) {
         return exec_context_may_touch_console(ec);
 }
 
-const char *unit_label_path(const Unit *u) {
-        const char *p;
-
-        assert(u);
-
-        /* Returns the file system path to use for MAC access decisions, i.e. the file to read the SELinux label off
-         * when validating access checks. */
-
-        if (IN_SET(u->load_state, UNIT_MASKED, UNIT_NOT_FOUND, UNIT_MERGED))
-                return NULL; /* Shortcut things if we know there is no real, relevant unit file around */
-
-        p = u->source_path ?: u->fragment_path;
-        if (!p)
-                return NULL;
-
-        if (IN_SET(u->load_state, UNIT_LOADED, UNIT_BAD_SETTING, UNIT_ERROR))
-                return p; /* Shortcut things, if we successfully loaded at least some stuff from the unit file */
-
-        /* Not loaded yet, we need to go to disk */
-        assert(u->load_state == UNIT_STUB);
-
-        /* If a unit is masked, then don't read the SELinux label of /dev/null, as that really makes no sense */
-        if (null_or_empty_path(p) > 0)
-                return NULL;
-
-        return p;
-}
-
 int unit_pid_attachable(Unit *u, pid_t pid, sd_bus_error *error) {
         int r;
 
index c6a2fadf528024c31c8a7a90010f65ef4e4f0ce6..33884fc9916ec556879a42fdd4df1ef311fc83ed 100644 (file)
@@ -151,6 +151,11 @@ typedef struct Unit {
         char *description;
         char **documentation;
 
+        /* The SELinux context used for checking access to this unit read off the unit file at load time (do
+         * not confuse with the selinux_context field in ExecContext which is the SELinux context we'll set
+         * for processes) */
+        char *access_selinux_context;
+
         char *fragment_path; /* if loaded from a config file this is the primary path to it */
         char *source_path; /* if converted, the source file */
         char **dropin_paths;
@@ -944,8 +949,6 @@ int unit_warn_leftover_processes(Unit *u, cg_kill_log_func_t log_func);
 
 bool unit_needs_console(Unit *u);
 
-const char *unit_label_path(const Unit *u);
-
 int unit_pid_attachable(Unit *unit, pid_t pid, sd_bus_error *error);
 
 static inline bool unit_has_job_type(Unit *u, JobType type) {