KRB5_CHILD: don't check if FILE:/DIR: path accessible in advance

Inaccessible FILE:/DIR: path is a system configuration error that
needs to be fixed anyway. It doesn't make much difference to fail
before or after credentials check in this case.

Author: alexey-tikhonov

Makefile.am

@@ -108,7 +108,7 @@ condconfigexists = ConditionPathExists=\|/etc/sssd/sssd.conf\nConditionDirectory
 #   - 'ldap_child': read keytab (dac_read_search)
 #   - 'krb5_child':
 #         - read keytab (dac_read_search)
-#         - check old ccache / pre-check ccache path / store TGT for a given user (set*id)
+#         - check old ccache / store TGT for a given user (set*id)
 #   - 'selinux_child': use libsemanage (set*id)
 #   - 'sssd_pam': read keytab in gss ops (dac_read_search)
 capabilities = CapabilityBoundingSet= CAP_SETGID CAP_SETUID CAP_DAC_READ_SEARCH

src/providers/krb5/krb5_ccache.c

@@ -102,108 +102,6 @@ static errno_t switch_to_service(void)
     return EOK;
 }
 
-static errno_t check_parent_stat(struct stat *parent_stat, uid_t uid)
-{
-    if (parent_stat->st_uid != 0 && parent_stat->st_uid != uid) {
-        DEBUG(SSSDBG_CRIT_FAILURE,
-              "Private directory can only be created below a directory "
-              "belonging to root or to [%"SPRIuid"].\n", uid);
-        return EINVAL;
-    }
-
-    if (parent_stat->st_uid == uid) {
-        if ( (parent_stat->st_mode & (S_IXUSR|S_IRUSR|S_IWUSR)) !=
-            (S_IXUSR|S_IRUSR|S_IWUSR) ) {
-            DEBUG(SSSDBG_CRIT_FAILURE,
-                  "Parent directory is owned but not accessible by user?!\n");
-            return EINVAL;
-        }
-    } else {
-        if ( (parent_stat->st_mode & (S_IXOTH|S_IROTH|S_IWOTH)) !=
-            (S_IXOTH|S_IROTH|S_IWOTH) ) {
-            DEBUG(SSSDBG_CRIT_FAILURE,
-                  "Parent directory is owned by root and not accessible to "
-                  "user\n");
-            return EINVAL;
-        }
-    }
-
-    return EOK;
-}
-
-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] == '/') {
-        filename = ccname;
-    } else if (strncmp(ccname, "FILE:", 5) == 0) {
-        filename = ccname + 5;
-    } else if (strncmp(ccname, "DIR:", 4) == 0) {
-        filename = ccname + 4;
-    } else {
-        /* only FILE and DIR types need pre-checks, we ignore any
-         * other type */
-        DEBUG(SSSDBG_TRACE_ALL, "No checks needed for [%s]\n", ccname);
-        return EOK;
-    }
-
-    tmp_ctx = talloc_new(NULL);
-    if (!tmp_ctx) return ENOMEM;
-
-    ccdirname = talloc_strdup(tmp_ctx, filename);
-    if (ccdirname == NULL) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup failed.\n");
-        ret = ENOMEM;
-        goto done;
-    }
-
-    /* Get parent directory of wanted ccache directory/file,
-     * removing trailing slashes from the back
-     */
-    do {
-        end = strrchr(ccdirname, '/');
-        if (end == NULL || end == ccdirname) {
-            DEBUG(SSSDBG_CRIT_FAILURE, "Cannot find parent directory of [%s], "
-                  "/ is not allowed.\n", ccdirname);
-            ret = EINVAL;
-            goto done;
-        }
-        *end = '\0';
-    } while (*(end+1) == '\0');
-
-    /* This function is currently called from `privileged_krb5_setup()` only
-     * so 'krb5_child' runs under service user at this point.
-     */
-    switch_to_user();
-    ret = stat(ccdirname, &parent_stat);
-    switch_to_service();
-    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;
-}
-
 struct sss_krb5_ccache {
     krb5_context context;
     krb5_ccache ccache;

src/providers/krb5/krb5_ccache.h

@@ -35,8 +35,6 @@ struct tgt_times {
     time_t renew_till;
 };
 
-errno_t sss_krb5_precheck_ccache(const char *ccname, uid_t uid, gid_t gid);
-
 errno_t sss_krb5_check_ccache_princ(krb5_context kctx,
                                     const char *ccname,
                                     krb5_principal user_princ);

src/providers/krb5/krb5_child.c

@@ -3873,88 +3873,53 @@ static errno_t old_ccache_valid(struct krb5_req *kr, bool *_valid)
     return EOK;
 }
 
-static int k5c_check_old_ccache(struct krb5_req *kr)
+static void k5c_ccache_check(struct krb5_req *kr, uint32_t offline)
 {
     errno_t ret;
 
-    if (kr->old_ccname) {
-        ret = old_ccache_valid(kr, &kr->old_cc_valid);
-        if (ret != EOK) {
-            DEBUG(SSSDBG_OP_FAILURE, "old_ccache_valid() failed.\n");
-            return ret;
-        }
+    kr->old_cc_valid = false;
+    kr->old_cc_active = false;
+
+    if (kr->pd->cmd == SSS_PAM_ACCT_MGMT) {
+        return;
+    }
+
+    if (kr->old_ccname == NULL) {
+        return;
+    }
 
+    ret = old_ccache_valid(kr, &kr->old_cc_valid);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, "old_ccache_valid() failed.\n");
+    } else {
         ret = check_if_uid_is_active(kr->uid, &kr->old_cc_active);
         if (ret != EOK) {
             DEBUG(SSSDBG_OP_FAILURE, "check_if_uid_is_active() failed.\n");
-            return ret;
+            kr->old_cc_valid = false;
         }
-
-        DEBUG(SSSDBG_TRACE_ALL,
-                "Old ccache is [%s] and is %s active and TGT is %s valid.\n",
-                kr->old_ccname ? kr->old_ccname : "not set",
-                kr->old_cc_active ? "" : "not",
-                kr->old_cc_valid ? "" : "not");
     }
 
-    return EOK;
-}
-
-static int k5c_precheck_ccache(struct krb5_req *kr, uint32_t offline)
-{
-    errno_t ret;
+    DEBUG(SSSDBG_TRACE_ALL,
+            "Old ccache is [%s] and is %s active and TGT is %s valid.\n",
+            kr->old_ccname ? kr->old_ccname : "not set",
+            kr->old_cc_active ? "" : "not",
+            kr->old_cc_valid ? "" : "not");
 
-    /* The ccache path should be checked if one of the following conditions
+    /* Old ccache can't be used if one of the following conditions
      * is true:
-     * - it doesn't exist (kr->old_ccname == NULL)
+     * - the backend is offline and the current cache file not used and
+     * it does not contain a valid TGT (1)
      * - the backend is online and the current ccache file is not used, i.e
      * the related user is currently not logged in and it is not a renewal
-     * request
-     * (offline && !kr->old_cc_active && kr->pd->cmd != SSS_CMD_RENEW)
-     * - the backend is offline and the current cache file not used and
-     * it does not contain a valid TGT
-     * (offline && !kr->old_cc_active && !kr->valid_tgt)
+     * request (2)
      */
-    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, "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 check failed.\n");
-            return ret;
-        }
+    if ((offline && !kr->old_cc_active && !kr->old_cc_valid) || /* (1) */
+        (!offline && !kr->old_cc_active && kr->pd->cmd != SSS_CMD_RENEW)) /* (2) */ {
+        DEBUG(SSSDBG_TRACE_ALL, "Ignoring old ccache\n");
     } else {
         DEBUG(SSSDBG_TRACE_ALL, "Reusing old ccache [%s]\n", kr->old_ccname);
         kr->ccname = kr->old_ccname;
     }
-
-    return EOK;
-}
-
-static int k5c_ccache_check(struct krb5_req *kr, uint32_t offline)
-{
-    errno_t ret;
-
-    if (kr->pd->cmd == SSS_PAM_ACCT_MGMT) {
-        return EOK;
-    }
-
-    ret = k5c_check_old_ccache(kr);
-    if (ret != 0) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot check old ccache [%s]: [%d][%s]. " \
-                                   "Assuming old cache is invalid " \
-                                   "and not used.\n",
-                                   kr->old_ccname, ret, sss_strerror(ret));
-    }
-
-    ret = k5c_precheck_ccache(kr, offline);
-    if (ret != 0) {
-        DEBUG(SSSDBG_OP_FAILURE, "ccache pre-check failed\n");
-        return ret;
-    }
-
-    return EOK;
 }
 
 static int k5c_setup(struct krb5_req *kr, uint32_t offline)
@@ -4163,15 +4128,9 @@ static krb5_error_code privileged_krb5_setup(struct krb5_req *kr,
         return ret;
     }
 
-    /* Determine if old FILE:/DIR: ccache needs to be re-used and if
-     * path is accessible.
-     */
+    /* Determine if old ccache name can and needs to be re-used */
     if (kr->krb5_child_has_setid_caps) {
-        ret = k5c_ccache_check(kr, offline);
-        if (ret != EOK) {
-            DEBUG(SSSDBG_CRIT_FAILURE, "k5c_ccache_check() failed.\n");
-            return ret;
-        }
+        k5c_ccache_check(kr, offline);
     }
 
     if (!(offline ||

src/tests/krb5_utils-tests.c

@@ -74,39 +74,6 @@ void teardown_create_dir(void)
     ck_assert_msg(ret == 0, "Cannot free talloc context.");
 }
 
-START_TEST(test_private_ccache_dir_in_wrong_user_dir)
-{
-    int ret;
-    char *cwd;
-    char *dirname;
-    char *subdirname;
-    char *filename;
-
-    ck_assert_msg(getuid() == 0, "This test must be run as root.");
-
-    cwd = getcwd(NULL, 0);
-    ck_assert_msg(cwd != NULL, "getcwd failed.");
-
-    dirname = talloc_asprintf(tmp_ctx, "%s/%s/priv_ccdir", cwd, TESTS_PATH);
-    free(cwd);
-    ck_assert_msg(dirname != NULL, "talloc_asprintf failed.");
-    ret = mkdir(dirname, 0700);
-    ck_assert_msg(ret == EOK, "mkdir failed.\n");
-    ret = chown(dirname, 12346, 12346);
-    ck_assert_msg(ret == EOK, "chown failed.\n");
-    subdirname = talloc_asprintf(tmp_ctx, "%s/subdir", dirname);
-    ck_assert_msg(subdirname != NULL, "talloc_asprintf failed.");
-    filename = talloc_asprintf(tmp_ctx, "%s/ccfile", subdirname);
-    ck_assert_msg(filename != NULL, "talloc_asprintf failed.");
-
-    ret = sss_krb5_precheck_ccache(filename, 12345, 12345);
-    ck_assert_msg(ret == EINVAL, "Creating private ccache dir in wrong user "
-                               "dir does not failed with EINVAL.");
-
-    RMDIR(dirname);
-}
-END_TEST
-
 START_TEST(test_illegal_patterns)
 {
     char *cwd;
@@ -650,11 +617,6 @@ Suite *krb5_utils_suite (void)
     tcase_add_checked_fixture (tc_create_dir, setup_create_dir,
                                teardown_create_dir);
     tcase_add_test (tc_create_dir, test_illegal_patterns);
-    if (getuid() == 0) {
-        tcase_add_test (tc_create_dir, test_private_ccache_dir_in_wrong_user_dir);
-    } else {
-        printf("Run as root to enable more tests.\n");
-    }
     suite_add_tcase (s, tc_create_dir);
 
     TCase *tc_krb5_helpers = tcase_create("Helper functions");