sssd: rework sss_ini_access_check

When sssd runs in unprivileged mode, the config file should, from a security perspective, ideally be owned by root:sssd and have mode rw-r-----, such that sssd can read, but not write to it. The current implementation of sss_ini_access_check rejects this security-conscious choice, so rewrite it. Fixes: 2.9.0-132-gae3bac934
SSSD · Oct 18, 2024 · 70d3ae9 · 70d3ae9
1 parent 217b3fa
commit 70d3ae9
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 15 deletions.
diff --git a/src/sysv/systemd/sssd-kcm.service.in b/src/sysv/systemd/sssd-kcm.service.in
@@ -9,9 +9,9 @@ Also=sssd-kcm.socket
 
 [Service]
 Environment=DEBUG_LOGGER=--logger=files
-ExecStartPre=+-/bin/chown -f @SSSD_USER@:@SSSD_USER@ @sssdconfdir@
-ExecStartPre=+-/bin/chown -f @SSSD_USER@:@SSSD_USER@ @sssdconfdir@/sssd.conf
-ExecStartPre=+-/bin/chown -f -R @SSSD_USER@:@SSSD_USER@ @sssdconfdir@/conf.d
+ExecStartPre=+-/bin/chown -f root:@SSSD_USER@ @sssdconfdir@
+ExecStartPre=+-/bin/chown -f root:@SSSD_USER@ @sssdconfdir@/sssd.conf
+ExecStartPre=+-/bin/chown -f -R root:@SSSD_USER@ @sssdconfdir@/conf.d
 ExecStartPre=+-/bin/sh -c "/bin/chown -f @SSSD_USER@:@SSSD_USER@ @secdbpath@/*.ldb"
 ExecStartPre=+-/bin/chown -f @SSSD_USER@:@SSSD_USER@ @logpath@/sssd_kcm.log
 ExecStart=@libexecdir@/sssd/sssd_kcm ${DEBUG_LOGGER}

diff --git a/src/sysv/systemd/sssd.service.in b/src/sysv/systemd/sssd.service.in
@@ -10,10 +10,10 @@ StartLimitBurst=5
 [Service]
 Environment=DEBUG_LOGGER=--logger=files
 EnvironmentFile=-@environment_file@
-ExecStartPre=+-/bin/chown -f @SSSD_USER@:@SSSD_USER@ @sssdconfdir@
-ExecStartPre=+-/bin/chown -f @SSSD_USER@:@SSSD_USER@ @sssdconfdir@/sssd.conf
-ExecStartPre=+-/bin/chown -f -R @SSSD_USER@:@SSSD_USER@ @sssdconfdir@/conf.d
-ExecStartPre=+-/bin/chown -f -R @SSSD_USER@:@SSSD_USER@ @sssdconfdir@/pki
+ExecStartPre=+-/bin/chown -f root:@SSSD_USER@ @sssdconfdir@
+ExecStartPre=+-/bin/chown -f root:@SSSD_USER@ @sssdconfdir@/sssd.conf
+ExecStartPre=+-/bin/chown -f -R root:@SSSD_USER@ @sssdconfdir@/conf.d
+ExecStartPre=+-/bin/chown -f -R root:@SSSD_USER@ @sssdconfdir@/pki
 ExecStartPre=+-/bin/sh -c "/bin/chown -f @SSSD_USER@:@SSSD_USER@ @dbpath@/*.ldb"
 ExecStartPre=+-/bin/sh -c "/bin/chown -f @SSSD_USER@:@SSSD_USER@ @gpocachepath@/*"
 ExecStartPre=+-/bin/sh -c "/bin/chown -f @SSSD_USER@:@SSSD_USER@ @logpath@/*.log"

diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
@@ -169,7 +169,6 @@ static bool is_running_sssd(void)
 
 static int sss_ini_access_check(struct sss_ini *self)
 {
-    int ret;
     uint32_t flags = INI_ACCESS_CHECK_MODE;
 
     if (!self->main_config_exists) {
@@ -180,14 +179,29 @@ static int sss_ini_access_check(struct sss_ini *self)
         flags |= INI_ACCESS_CHECK_UID | INI_ACCESS_CHECK_GID;
     }
 
-    ret = ini_config_access_check(self->file,
-                                  flags,
-                                  geteuid(),
-                                  getegid(),
-                                  S_IRUSR, /* r**------ */
-                                  ALLPERMS & ~(S_IWUSR|S_IXUSR));
+    if (geteuid() == 0)
+        return EOK;
 
-    return ret;
+    /* There is no legit reason to have any write/modify permissions on the config file. */
+    const char *name = ini_config_get_filename(self->file);
+    if (name != NULL) {
+        if (access(name, W_OK) == 0) {
+            DEBUG(SSSDBG_CRIT_FAILURE, "%s is writable to sssd, which it should not be.\n", name);
+            return EACCES;
+        } else if (access(name, R_OK) != 0) {
+            DEBUG(SSSDBG_CRIT_FAILURE, "%s is not readable.\n", name);
+            return EACCES;
+        }
+    }
+
+    /* Also ensure we are barred from calling chmod. */
+    const struct stat *sb = ini_config_get_stat(self->file);
+    if (sb != NULL && sb->st_uid == geteuid()) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Ownership on \"%s\" should not belong to sssd.\n", name);
+        return EACCES;
+    }
+
+    return EOK;
 }