KRB5: don't pre-create parent dir(s) of wanted DIR:/FILE:

to match 'kinit' behavior and avoid the need for cap_chown and cap_dac_override.
SSSD · Nov 25, 2024 · 7283042 · 7283042
1 parent cdf1eaa
commit 7283042
Show file tree

Hide file tree

Showing 8 changed files with 45 additions and 311 deletions.
diff --git a/Makefile.am b/Makefile.am
@@ -103,13 +103,11 @@ condconfigexists = ConditionPathExists=\|/etc/sssd/sssd.conf\nConditionDirectory
 # Capabilities usage by binaries:
 #   - 'ldap_child': read keytab (dac_read_search)
 #   - 'krb5_child':
-#         - store TGT for a given user (set*id);
-#         - create path components of DIR:/FILE: cache, for example: /run/user/$UID (dac_override, chown)
-#         - read keytab (dac_read_search could be enough but dac_override due to above)
-#     If system doesn't need to support DIR:/FILE: then 'cap_chown' can be stripped and 'cap_dac_override' replaced with 'dac_read_search'
+#         - store TGT for a given user (set*id)
+#         - read keytab (dac_read_search)
 #   - 'selinux_child': currently chown, dac_override, set*id  --  to be narrowed
 #   - 'sssd_pam': read keytab in gss ops (dac_read_search)
-capabilities = CapabilityBoundingSet= CAP_CHOWN CAP_DAC_OVERRIDE CAP_SETGID CAP_SETUID CAP_DAC_READ_SEARCH
+capabilities = CapabilityBoundingSet= CAP_SETGID CAP_SETUID CAP_DAC_READ_SEARCH
 
 if BUILD_CONF_SERVICE_USER_SUPPORT
 # If non-root service user is supported, monitor might need SET-ID to switch user (deprecated 'sssd.conf::user' option)
@@ -5584,7 +5582,7 @@ if SSSD_USER
 	-$(SETCAP) cap_dac_read_search=p $(DESTDIR)$(sssdlibexecdir)/ldap_child
 	-chgrp $(SSSD_USER) $(DESTDIR)$(sssdlibexecdir)/krb5_child
 	chmod 750 $(DESTDIR)$(sssdlibexecdir)/krb5_child
-	-$(SETCAP) cap_chown,cap_dac_override,cap_setuid,cap_setgid=ep $(DESTDIR)$(sssdlibexecdir)/krb5_child
+	-$(SETCAP) cap_dac_read_search,cap_setuid,cap_setgid=ep $(DESTDIR)$(sssdlibexecdir)/krb5_child
 	-chgrp $(SSSD_USER) $(DESTDIR)$(sssdlibexecdir)/proxy_child
 	chmod 750 $(DESTDIR)$(sssdlibexecdir)/proxy_child
 	-chgrp $(SSSD_USER) $(DESTDIR)$(sssdlibexecdir)/sssd_pam

diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
@@ -914,7 +914,7 @@ install -D -p -m 0644 %{SOURCE1} %{buildroot}%{_sysusersdir}/sssd.conf
 %license COPYING
 %attr(775,%{sssd_user},%{sssd_user}) %dir %{pubconfpath}/krb5.include.d
 %attr(0750,root,%{sssd_user}) %caps(cap_dac_read_search=p) %{_libexecdir}/%{servicename}/ldap_child
-%attr(0750,root,%{sssd_user}) %caps(cap_chown,cap_dac_override,cap_setuid,cap_setgid=ep) %{_libexecdir}/%{servicename}/krb5_child
+%attr(0750,root,%{sssd_user}) %caps(cap_dac_read_search,cap_setuid,cap_setgid=ep) %{_libexecdir}/%{servicename}/krb5_child
 
 %files krb5 -f sssd_krb5.lang
 %license COPYING

diff --git a/src/providers/krb5/krb5_ccache.c b/src/providers/krb5/krb5_ccache.c
@@ -49,79 +49,6 @@ struct string_list {
     char *s;
 };
 
-static errno_t find_ccdir_parent_data(TALLOC_CTX *mem_ctx,
-                                      const char *ccdirname,
-                                      struct stat *parent_stat,
-                                      struct string_list **missing_parents)
-{
-    int ret = EFAULT;
-    char *parent = NULL;
-    char *end;
-    struct string_list *li;
-
-    ret = stat(ccdirname, parent_stat);
-    if (ret == EOK) {
-        if ( !S_ISDIR(parent_stat->st_mode) ) {
-            DEBUG(SSSDBG_MINOR_FAILURE,
-                  "[%s] is not a directory.\n", ccdirname);
-            return EINVAL;
-        }
-        return EOK;
-    } else {
-        if (errno != ENOENT) {
-            ret = errno;
-            DEBUG(SSSDBG_MINOR_FAILURE,
-                  "stat for [%s] failed: [%d][%s].\n", ccdirname, ret,
-                   strerror(ret));
-            return ret;
-        }
-    }
-
-    li = talloc_zero(mem_ctx, struct string_list);
-    if (li == NULL) {
-        DEBUG(SSSDBG_CRIT_FAILURE,
-              "talloc_zero failed.\n");
-        return ENOMEM;
-    }
-
-    li->s = talloc_strdup(li, ccdirname);
-    if (li->s == NULL) {
-        DEBUG(SSSDBG_CRIT_FAILURE,
-              "talloc_strdup failed.\n");
-        return ENOMEM;
-    }
-
-    DLIST_ADD(*missing_parents, li);
-
-    parent = talloc_strdup(mem_ctx, ccdirname);
-    if (parent == NULL) {
-        DEBUG(SSSDBG_CRIT_FAILURE,
-              "talloc_strdup failed.\n");
-        return ENOMEM;
-    }
-
-    /* We'll remove all trailing slashes from the back so that
-     * we only pass /some/path to find_ccdir_parent_data, not
-     * /some/path */
-    do {
-        end = strrchr(parent, '/');
-        if (end == NULL || end == parent) {
-            DEBUG(SSSDBG_MINOR_FAILURE,
-                  "Cannot find parent directory of [%s], / is not allowed.\n",
-                   ccdirname);
-            ret = EINVAL;
-            goto done;
-        }
-        *end = '\0';
-    } while (*(end+1) == '\0');
-
-    ret = find_ccdir_parent_data(mem_ctx, parent, parent_stat, missing_parents);
-
-done:
-    talloc_free(parent);
-    return ret;
-}
-
 static errno_t check_parent_stat(struct stat *parent_stat, uid_t uid)
 {
     if (parent_stat->st_uid != 0 && parent_stat->st_uid != uid) {
@@ -132,104 +59,32 @@ static errno_t check_parent_stat(struct stat *parent_stat, uid_t uid)
     }
 
     if (parent_stat->st_uid == uid) {
-        if (!(parent_stat->st_mode & S_IXUSR)) {
+        if ( (parent_stat->st_mode & (S_IXUSR|S_IRUSR|S_IWUSR)) !=
+            (S_IXUSR|S_IRUSR|S_IWUSR) ) {
             DEBUG(SSSDBG_CRIT_FAILURE,
-                  "Parent directory does not have the search bit set for "
-                   "the owner.\n");
+                  "Parent directory is owned but not accessible by user?!\n");
             return EINVAL;
         }
     } else {
-        if (!(parent_stat->st_mode & S_IXOTH)) {
+        if ( (parent_stat->st_mode & (S_IXOTH|S_IROTH|S_IWOTH)) !=
+            (S_IXOTH|S_IROTH|S_IWOTH) ) {
             DEBUG(SSSDBG_CRIT_FAILURE,
-                  "Parent directory does not have the search bit set for "
-                   "others.\n");
+                  "Parent directory is owned by root and not accessible to "
+                  "user\n");
             return EINVAL;
         }
     }
 
     return EOK;
 }
 
-static errno_t create_ccache_dir(const char *ccdirname, uid_t uid, gid_t gid)
-{
-    int ret = EFAULT;
-    struct stat parent_stat;
-    struct string_list *missing_parents = NULL;
-    struct string_list *li = NULL;
-    mode_t old_umask;
-    mode_t new_dir_mode;
-    TALLOC_CTX *tmp_ctx = NULL;
-
-    tmp_ctx = talloc_new(NULL);
-    if (tmp_ctx == NULL) {
-        DEBUG(SSSDBG_CRIT_FAILURE,
-              "talloc_new failed.\n");
-        return ENOMEM;
-    }
-
-    if (*ccdirname != '/') {
-        DEBUG(SSSDBG_MINOR_FAILURE,
-              "Only absolute paths are allowed, not [%s] .\n", ccdirname);
-        ret = EINVAL;
-        goto done;
-    }
-
-    DEBUG(SSSDBG_TRACE_ALL, "Processing  [%s] for [%"SPRIuid"][%"SPRIgid"]\n",
-          ccdirname, uid, gid);
-
-    ret = find_ccdir_parent_data(tmp_ctx, ccdirname, &parent_stat,
-                                 &missing_parents);
-    if (ret != EOK) {
-        DEBUG(SSSDBG_MINOR_FAILURE,
-              "find_ccdir_parent_data failed.\n");
-        goto done;
-    }
-
-    ret = check_parent_stat(&parent_stat, uid);
-    if (ret != EOK) {
-        DEBUG(SSSDBG_FATAL_FAILURE,
-              "Check the ownership and permissions of krb5_ccachedir: [%s].\n",
-              ccdirname);
-        goto done;
-    }
-
-    DLIST_FOR_EACH(li, missing_parents) {
-        DEBUG(SSSDBG_TRACE_INTERNAL,
-              "Creating directory [%s].\n", li->s);
-        new_dir_mode = 0700;
-
-        old_umask = umask(0000);
-        ret = mkdir(li->s, new_dir_mode);
-        umask(old_umask);
-        if (ret != EOK) {
-            ret = errno;
-            DEBUG(SSSDBG_MINOR_FAILURE,
-                  "mkdir [%s] failed: [%d][%s].\n", li->s, ret,
-                   strerror(ret));
-            goto done;
-        }
-        ret = chown(li->s, uid, gid);
-        if (ret != EOK) {
-            ret = errno;
-            DEBUG(SSSDBG_MINOR_FAILURE,
-                  "chown failed [%d][%s].\n", ret, strerror(ret));
-            goto done;
-        }
-    }
-
-    ret = EOK;
-
-done:
-    talloc_free(tmp_ctx);
-    return ret;
-}
-
-errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid)
+errno_t sss_krb5_precheck_ccache(const char *ccname, uid_t uid, gid_t gid)
 {
     TALLOC_CTX *tmp_ctx = NULL;
     const char *filename;
     char *ccdirname;
     char *end;
+    struct stat parent_stat;
     errno_t ret;
 
     if (ccname[0] == '/') {
@@ -239,9 +94,9 @@ errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid)
     } else if (strncmp(ccname, "DIR:", 4) == 0) {
         filename = ccname + 4;
     } else {
-        /* only FILE and DIR types need precreation so far, we ignore any
+        /* only FILE and DIR types need pre-checks, we ignore any
          * other type */
-        DEBUG(SSSDBG_TRACE_ALL, "No pre-creation needed for [%s]\n", ccname);
+        DEBUG(SSSDBG_TRACE_ALL, "No checks needed for [%s]\n", ccname);
         return EOK;
     }
 
@@ -255,9 +110,9 @@ errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid)
         goto done;
     }
 
-    /* We'll remove all trailing slashes from the back so that
-     * we only pass /some/path to find_ccdir_parent_data, not
-     * /some/path/ */
+    /* Get parent directory of wanted ccache directory/file,
+     * removing trailing slashes from the back
+     */
     do {
         end = strrchr(ccdirname, '/');
         if (end == NULL || end == ccdirname) {
@@ -269,7 +124,23 @@ errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid)
         *end = '\0';
     } while (*(end+1) == '\0');
 
-    ret = create_ccache_dir(ccdirname, uid, gid);
+    ret = stat(ccdirname, &parent_stat);
+    if (ret != 0) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot stat() [%s]\n", ccdirname);
+        ret = EINVAL;
+        goto done;
+    }
+
+    ret = check_parent_stat(&parent_stat, uid);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_FATAL_FAILURE,
+              "Check the ownership and permissions of krb5_ccachedir: [%s].\n",
+              ccdirname);
+        goto done;
+    }
+
+    ret = EOK;
+
 done:
     talloc_free(tmp_ctx);
     return ret;

diff --git a/src/providers/krb5/krb5_ccache.h b/src/providers/krb5/krb5_ccache.h
@@ -35,7 +35,7 @@ struct tgt_times {
     time_t renew_till;
 };
 
-errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid);
+errno_t sss_krb5_precheck_ccache(const char *ccname, uid_t uid, gid_t gid);
 
 errno_t sss_krb5_cc_destroy(const char *ccname, uid_t uid, gid_t gid);
 

diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
@@ -3775,11 +3775,11 @@ static int k5c_check_old_ccache(struct krb5_req *kr)
     return EOK;
 }
 
-static int k5c_precreate_ccache(struct krb5_req *kr, uint32_t offline)
+static int k5c_precheck_ccache(struct krb5_req *kr, uint32_t offline)
 {
     errno_t ret;
 
-    /* The ccache file should be (re)created if one of the following conditions
+    /* The ccache path should be checked if one of the following conditions
      * is true:
      * - it doesn't exist (kr->old_ccname == NULL)
      * - the backend is online and the current ccache file is not used, i.e
@@ -3793,8 +3793,8 @@ static int k5c_precreate_ccache(struct krb5_req *kr, uint32_t offline)
     if (kr->old_ccname == NULL ||
             (offline && !kr->old_cc_active && !kr->old_cc_valid) ||
             (!offline && !kr->old_cc_active && kr->pd->cmd != SSS_CMD_RENEW)) {
-        DEBUG(SSSDBG_TRACE_ALL, "Recreating ccache [%s]\n", kr->ccname);
-        ret = sss_krb5_precreate_ccache(kr->ccname, kr->uid, kr->gid);
+        DEBUG(SSSDBG_TRACE_ALL, "Pre-checking ccache [%s]\n", kr->ccname);
+        ret = sss_krb5_precheck_ccache(kr->ccname, kr->uid, kr->gid);
         if (ret != EOK) {
             DEBUG(SSSDBG_OP_FAILURE, "ccache creation failed.\n");
             return ret;
@@ -3823,14 +3823,9 @@ static int k5c_ccache_setup(struct krb5_req *kr, uint32_t offline)
                                    kr->old_ccname, ret, sss_strerror(ret));
     }
 
-    /* Pre-creating the ccache must be done as root, otherwise we can't mkdir
-     * some of the DIR: cache components. One example is /run/user/$UID because
-     * logind doesn't create the directory until the session phase, whereas
-     * we need the directory during the auth phase already
-     */
-    ret = k5c_precreate_ccache(kr, offline);
+    ret = k5c_precheck_ccache(kr, offline);
     if (ret != 0) {
-        DEBUG(SSSDBG_OP_FAILURE, "Cannot precreate ccache\n");
+        DEBUG(SSSDBG_OP_FAILURE, "ccache pre-check failed\n");
         return ret;
     }
 

diff --git a/src/sysv/systemd/sssd-kcm.service.in b/src/sysv/systemd/sssd-kcm.service.in
@@ -14,7 +14,7 @@ ExecStartPre=+-/bin/chmod -f -R g+r @sssdconfdir@
 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}
-CapabilityBoundingSet= CAP_DAC_OVERRIDE CAP_CHOWN CAP_SETGID CAP_SETUID
+CapabilityBoundingSet= CAP_DAC_READ_SEARCH CAP_SETGID CAP_SETUID
 SecureBits=noroot noroot-locked
 User=@SSSD_USER@
 Group=@SSSD_USER@

diff --git a/src/tests/krb5_child-test.c b/src/tests/krb5_child-test.c
@@ -243,13 +243,6 @@ create_dummy_req(TALLOC_CTX *mem_ctx, const char *user,
     DEBUG(SSSDBG_FUNC_DATA, "ccname [%s] uid [%u] gid [%u]\n",
             kr->ccname, kr->uid, kr->gid);
 
-    ret = sss_krb5_precreate_ccache(kr->ccname,
-                                    kr->uid, kr->gid);
-    if (ret != EOK) {
-        DEBUG(SSSDBG_OP_FAILURE, "create_ccache_dir failed.\n");
-        goto fail;
-    }
-
     return kr;
 
 fail: