Re: [RFC PATCH 4/8] core: add missing SELinux checks for dbus methods

Stephen Smalley <sds@xxxxxxxxxxxxx> · Wed, 18 Dec 2019 15:05:36 -0500

On 12/18/19 9:28 AM, Christian Göttsche wrote:
Add new SELinux permissions `modify` and `listdynusers` to the class `service`.
   - modfiy checks altering operations, like `systemctl log-level debug` or `systemctl add-wants foo bar`.
   - listdynusers checks obtaining dynamic users, which is very common on systems using nss-systemd.
     Add a new permission to avoid undermining the `status` permission.

Perform SELinux access checks for the following D-Bus exposed methods:

   D-Bus interface         | c function name                    | SELinux permission verb

   GetAfter / GetBefore    | bus_job_method_get_waiting_jobs    | status
   LogTarget               | property_set_log_target            | modify
   LogLevel                | property_set_log_level             | modify
   RuntimeWatchdocUSec     | property_set_runtime_watchdog      | modify
   ServiceWatchdogs        | bus_property_set_bool_wrapper      | modify
   GetUnit                 | method_get_unit                    | status
   GetUnitByPID            | method_get_unit_by_pid             | status
   GetUnitByControlGroup   | method_get_unit_by_control_group   | status
   LoadUnit                | method_load_unit                   | status
   ListUnitsByNames        | method_list_units_by_names         | status
   LookupDynamicUserByName | method_lookup_dynamic_user_by_name | listdynusers
   LookupDynamicUserByUID  | method_lookup_dynamic_user_by_uid  | listdynusers
   GetDynamicUsers         | method_get_dynamic_users           | listdynusers
   AddDependencyUnitFiles  | method_add_dependency_unit_files   | modify
   GetUnitFileLinks        | method_get_unit_file_links         | status
   Unref                   | bus_unit_method_unref              | modify

If we are going to introduce new permissions or change existing ones, we 
may want to consider just defining a separate permission for every 
logical interface.  Then we can let the policy writer decide what 
matters to them and at what granularity they want to distinguish things, 
using macros/interfaces as appropriate to avoid needing to specify them 
all individually.

Also, you may want to leverage the policy capability mechanism in 
userspace to permit compatible evolution of permission checks in the 
same manner it is presently used in the kernel for 
extended_socket_class, network_peer_controls, open_perms, 
nnp_nosuid_transition, etc.

---
  src/core/dbus-job.c     |  4 ++
  src/core/dbus-manager.c | 89 ++++++++++++++++++++++++++++++++++++-----
  src/core/dbus-unit.c    |  4 ++
  3 files changed, 88 insertions(+), 9 deletions(-)

diff --git a/src/core/dbus-job.c b/src/core/dbus-job.c
index a7d342257b..7b0b093757 100644
--- a/src/core/dbus-job.c
+++ b/src/core/dbus-job.c
@@ -71,6 +71,10 @@ int bus_job_method_get_waiting_jobs(sd_bus_message *message, void *userdata, sd_
          Job *j = userdata;
          int r, i, n;
  
+        r = mac_selinux_unit_access_check(j->unit, message, "status", error);
+        if (r < 0)
+                return r;
+
          if (strstr(sd_bus_message_get_member(message), "After"))
                  n = job_get_after(j, &list);
          else
diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
index c751e84253..d2db6e4a61 100644
--- a/src/core/dbus-manager.c
+++ b/src/core/dbus-manager.c
@@ -121,6 +121,10 @@ static int property_set_log_target(
          assert(bus);
          assert(value);
  
+        r = mac_selinux_access_check(value, "modify", error);
+        if (r < 0)
+                return r;
+
          r = sd_bus_message_read(value, "s", &t);
          if (r < 0)
                  return r;
@@ -178,6 +182,10 @@ static int property_set_log_level(
          assert(bus);
          assert(value);
  
+        r = mac_selinux_access_check(value, "modify", error);
+        if (r < 0)
+                return r;
+
          r = sd_bus_message_read(value, "s", &t);
          if (r < 0)
                  return r;
@@ -282,6 +290,10 @@ static int property_set_runtime_watchdog(
  
          assert_cc(sizeof(usec_t) == sizeof(uint64_t));
  
+        r = mac_selinux_access_check(value, "modify", error);
+        if (r < 0)
+                return r;
+
          r = sd_bus_message_read(value, "t", t);
          if (r < 0)
                  return r;
@@ -289,6 +301,24 @@ static int property_set_runtime_watchdog(
          return watchdog_set_timeout(t);
  }
  
+static int bus_property_set_bool_wrapper(
+                sd_bus *bus,
+                const char *path,
+                const char *interface,
+                const char *property,
+                sd_bus_message *value,
+                void *userdata,
+                sd_bus_error *error) {
+
+        int r;
+
+        r = mac_selinux_access_check(value, "modify", error);
+        if (r < 0)
+                return r;
+
+        return bus_property_set_bool(bus, path, interface, property, value, userdata, error);
+}
+
  static int bus_get_unit_by_name(Manager *m, sd_bus_message *message, const char *name, Unit **ret_unit, sd_bus_error *error) {
          Unit *u;
          int r;
@@ -375,6 +405,10 @@ static int method_get_unit(sd_bus_message *message, void *userdata, sd_bus_error
          if (r < 0)
                  return r;
  
+        r = mac_selinux_unit_access_check(u, message, "status", error);
+        if (r < 0)
+                return r;
+
          return reply_unit_path(u, message, error);
  }
  
@@ -413,6 +447,10 @@ static int method_get_unit_by_pid(sd_bus_message *message, void *userdata, sd_bu
          if (!u)
                  return sd_bus_error_setf(error, BUS_ERROR_NO_UNIT_FOR_PID, "PID "PID_FMT" does not belong to any loaded unit.", pid);
  
+        r = mac_selinux_unit_access_check(u, message, "status", error);
+        if (r < 0)
+                return r;
+
          return reply_unit_path(u, message, error);
  }
  
@@ -488,6 +526,10 @@ static int method_get_unit_by_control_group(sd_bus_message *message, void *userd
          if (!u)
                  return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_UNIT, "Control group '%s' is not valid or not managed by this instance", cgroup);
  
+        r = mac_selinux_unit_access_check(u, message, "status", error);
+        if (r < 0)
+                return r;
+
          return reply_unit_path(u, message, error);
  }
  
@@ -510,6 +552,10 @@ static int method_load_unit(sd_bus_message *message, void *userdata, sd_bus_erro
          if (r < 0)
                  return r;
  
+        r = mac_selinux_unit_access_check(u, message, "status", error);
+        if (r < 0)
+                return r;
+
          return reply_unit_path(u, message, error);
  }
  
@@ -529,6 +575,7 @@ static int method_start_unit_generic(sd_bus_message *message, Manager *m, JobTyp
          if (r < 0)
                  return r;
  
+        /* bus_unit_method_start_generic() includes a mac_selinux check */
          return bus_unit_method_start_generic(message, u, job_type, reload_if_possible, error);
  }
  
@@ -703,6 +750,10 @@ static int method_list_units_by_names(sd_bus_message *message, void *userdata, s
          assert(message);
          assert(m);
  
+        r = mac_selinux_access_check(message, "status", error);
+        if (r < 0)
+                return r;
+
          r = sd_bus_message_read_strv(message, &units);
          if (r < 0)
                  return r;
@@ -1263,11 +1314,11 @@ static int method_reload(sd_bus_message *message, void *userdata, sd_bus_error *
          assert(message);
          assert(m);
  
-        r = verify_run_space("Refusing to reload", error);
+        r = mac_selinux_access_check(message, "reload", error);
          if (r < 0)
                  return r;
  
-        r = mac_selinux_access_check(message, "reload", error);
+        r = verify_run_space("Refusing to reload", error);
          if (r < 0)
                  return r;
  
@@ -1299,11 +1350,11 @@ static int method_reexecute(sd_bus_message *message, void *userdata, sd_bus_erro
          assert(message);
          assert(m);
  
-        r = verify_run_space("Refusing to reexecute", error);
+        r = mac_selinux_access_check(message, "reload", error);
          if (r < 0)
                  return r;
  
-        r = mac_selinux_access_check(message, "reload", error);
+        r = verify_run_space("Refusing to reexecute", error);
          if (r < 0)
                  return r;
  
@@ -1428,6 +1479,10 @@ static int method_switch_root(sd_bus_message *message, void *userdata, sd_bus_er
          assert(message);
          assert(m);
  
+        r = mac_selinux_access_check(message, "reboot", error);
+        if (r < 0)
+                return r;
+
          if (statvfs("/run/systemd", &svfs) < 0)
                  return sd_bus_error_set_errnof(error, errno, "Failed to statvfs(/run/systemd): %m");
  
@@ -1441,10 +1496,6 @@ static int method_switch_root(sd_bus_message *message, void *userdata, sd_bus_er
                              format_bytes(fb_need, sizeof(fb_need), RELOAD_DISK_SPACE_MIN));
          }
  
-        r = mac_selinux_access_check(message, "reboot", error);
-        if (r < 0)
-                return r;
-
          if (!MANAGER_IS_SYSTEM(m))
                  return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, "Root switching is only supported by system manager.");
  
@@ -1636,6 +1687,10 @@ static int method_lookup_dynamic_user_by_name(sd_bus_message *message, void *use
          assert(message);
          assert(m);
  
+        r = mac_selinux_access_check(message, "listdynusers", error);
+        if (r < 0)
+                return r;
+
          r = sd_bus_message_read_basic(message, 's', &name);
          if (r < 0)
                  return r;
@@ -1663,6 +1718,10 @@ static int method_lookup_dynamic_user_by_uid(sd_bus_message *message, void *user
          assert(message);
          assert(m);
  
+        r = mac_selinux_access_check(message, "listdynusers", error);
+        if (r < 0)
+                return r;
+
          assert_cc(sizeof(uid) == sizeof(uint32_t));
          r = sd_bus_message_read_basic(message, 'u', &uid);
          if (r < 0)
@@ -1692,6 +1751,10 @@ static int method_get_dynamic_users(sd_bus_message *message, void *userdata, sd_
          assert(message);
          assert(m);
  
+        r = mac_selinux_access_check(message, "listdynusers", error);
+        if (r < 0)
+                return r;
+
          assert_cc(sizeof(uid_t) == sizeof(uint32_t));
  
          if (!MANAGER_IS_SYSTEM(m))
@@ -2264,6 +2327,10 @@ static int method_add_dependency_unit_files(sd_bus_message *message, void *userd
          assert(message);
          assert(m);
  
+        r = mac_selinux_access_check(message, "modify", error);
+        if (r < 0)
+                return r;
+
          r = bus_verify_manage_unit_files_async(m, message, error);
          if (r < 0)
                  return r;
@@ -2300,6 +2367,10 @@ static int method_get_unit_file_links(sd_bus_message *message, void *userdata, s
          char **p;
          int runtime, r;
  
+        r = mac_selinux_access_check(message, "status", error);
+        if (r < 0)
+                return r;
+
          r = sd_bus_message_read(message, "sb", &name, &runtime);
          if (r < 0)
                  return r;
@@ -2422,7 +2493,7 @@ const sd_bus_vtable bus_manager_vtable[] = {
          /* The following item is an obsolete alias */
          SD_BUS_WRITABLE_PROPERTY("ShutdownWatchdogUSec", "t", bus_property_get_usec, bus_property_set_usec, offsetof(Manager, reboot_watchdog), SD_BUS_VTABLE_HIDDEN),
          SD_BUS_WRITABLE_PROPERTY("KExecWatchdogUSec", "t", bus_property_get_usec, bus_property_set_usec, offsetof(Manager, kexec_watchdog), 0),
-        SD_BUS_WRITABLE_PROPERTY("ServiceWatchdogs", "b", bus_property_get_bool, bus_property_set_bool, offsetof(Manager, service_watchdogs), 0),
+        SD_BUS_WRITABLE_PROPERTY("ServiceWatchdogs", "b", bus_property_get_bool, bus_property_set_bool_wrapper, offsetof(Manager, service_watchdogs), 0),
          SD_BUS_PROPERTY("ControlGroup", "s", NULL, offsetof(Manager, cgroup_root), 0),
          SD_BUS_PROPERTY("SystemState", "s", property_get_system_state, 0, 0),
          SD_BUS_PROPERTY("ExitCode", "y", bus_property_get_unsigned, offsetof(Manager, return_value), 0),
diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c
index 9477c47140..f86efaac3a 100644
--- a/src/core/dbus-unit.c
+++ b/src/core/dbus-unit.c
@@ -645,6 +645,10 @@ int bus_unit_method_unref(sd_bus_message *message, void *userdata, sd_bus_error
          assert(message);
          assert(u);
  
+        r = mac_selinux_unit_access_check(u, message, "modify", error);
+        if (r < 0)
+                return r;
+
          r = bus_unit_track_remove_sender(u, message);
          if (r == -EUNATCH)
                  return sd_bus_error_setf(error, BUS_ERROR_NOT_REFERENCED, "Unit has not been referenced yet.");








