From b49de9745a41ef938ccadbfaff260f46c509f4c7 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Mon, 25 Nov 2024 22:08:15 +0100 Subject: [PATCH] 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. --- Makefile.am | 10 +- contrib/sssd.spec.in | 2 +- src/providers/krb5/krb5_ccache.c | 191 +++++---------------------- src/providers/krb5/krb5_ccache.h | 2 +- src/providers/krb5/krb5_child.c | 17 +-- src/sysv/systemd/sssd-kcm.service.in | 2 +- src/tests/krb5_child-test.c | 7 - src/tests/krb5_utils-tests.c | 125 +----------------- 8 files changed, 45 insertions(+), 311 deletions(-) diff --git a/Makefile.am b/Makefile.am index 1b9484e1c66..83677945049 100644 --- 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_DAC_OVERRIDE 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 index 71d3285ffa2..66798a3fa68 100644 --- 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 index 2fd1c00742b..fb90f59ae16 100644 --- 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,17 +59,18 @@ 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; } } @@ -150,86 +78,13 @@ static errno_t check_parent_stat(struct stat *parent_stat, uid_t uid) 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 index f3928e644d7..8db2c3538f1 100644 --- 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 index d6b281cb8ac..18512672112 100644 --- 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 index ba9e27cd99f..3e48945aaed 100644 --- 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 index ceed041dee2..011a3bb1775 100644 --- 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: diff --git a/src/tests/krb5_utils-tests.c b/src/tests/krb5_utils-tests.c index 76742922997..04a3733d972 100644 --- a/src/tests/krb5_utils-tests.c +++ b/src/tests/krb5_utils-tests.c @@ -74,85 +74,6 @@ void teardown_create_dir(void) ck_assert_msg(ret == 0, "Cannot free talloc context."); } -static void check_dir(const char *dirname, uid_t uid, gid_t gid, mode_t mode) -{ - struct stat stat_buf; - int ret; - - ret = stat(dirname, &stat_buf); - ck_assert_msg(ret == EOK, "stat failed [%d][%s].", errno, strerror(errno)); - - ck_assert_msg(S_ISDIR(stat_buf.st_mode), "[%s] is not a directory.", dirname); - ck_assert_msg(stat_buf.st_uid == uid, "uid does not match, " - "expected [%d], got [%d].", - uid, stat_buf.st_uid); - ck_assert_msg(stat_buf.st_gid == gid, "gid does not match, " - "expected [%d], got [%d].", - gid, stat_buf.st_gid); - ck_assert_msg((stat_buf.st_mode & ~S_IFMT) == mode, - "mode of [%s] does not match, " - "expected [%o], got [%o].", dirname, - mode, (stat_buf.st_mode & ~S_IFMT)); -} - -START_TEST(test_private_ccache_dir_in_user_dir) -{ - int ret; - char *cwd; - char *user_dir; - char *dn1; - char *dn2; - char *dn3; - char *filename; - uid_t uid = getuid(); - gid_t gid = getgid(); - - if (uid == 0) { - uid = 12345; - gid = 12345; - } - - cwd = getcwd(NULL, 0); - ck_assert_msg(cwd != NULL, "getcwd failed."); - - user_dir = talloc_asprintf(tmp_ctx, "%s/%s/user", cwd, TESTS_PATH); - free(cwd); - ck_assert_msg(user_dir != NULL, "talloc_asprintf failed."); - ret = mkdir(user_dir, 0700); - ck_assert_msg(ret == EOK, "mkdir failed."); - ret = chown(user_dir, uid, gid); - ck_assert_msg(ret == EOK, "chown failed."); - - dn1 = talloc_asprintf(tmp_ctx, "%s/a", user_dir); - ck_assert_msg(dn1 != NULL, "talloc_asprintf failed."); - dn2 = talloc_asprintf(tmp_ctx, "%s/b", dn1); - ck_assert_msg(dn2 != NULL, "talloc_asprintf failed."); - dn3 = talloc_asprintf(tmp_ctx, "%s/c", dn2); - ck_assert_msg(dn3 != NULL, "talloc_asprintf failed."); - filename = talloc_asprintf(tmp_ctx, "%s/ccfile", dn3); - ck_assert_msg(filename != NULL, "talloc_asprintf failed."); - - ret = chmod(user_dir, 0600); - ck_assert_msg(ret == EOK, "chmod failed."); - ret = sss_krb5_precreate_ccache(filename, uid, gid); - ck_assert_msg(ret == EINVAL, "sss_krb5_precreate_ccache does not return EINVAL " - "while x-bit is missing."); - - ret = chmod(user_dir, 0700); - ck_assert_msg(ret == EOK, "chmod failed."); - ret = sss_krb5_precreate_ccache(filename, uid, gid); - ck_assert_msg(ret == EOK, "sss_krb5_precreate_ccache failed."); - - check_dir(dn3, uid, gid, 0700); - RMDIR(dn3); - check_dir(dn2, uid, gid, 0700); - RMDIR(dn2); - check_dir(dn1, uid, gid, 0700); - RMDIR(dn1); - RMDIR(user_dir); -} -END_TEST - START_TEST(test_private_ccache_dir_in_wrong_user_dir) { int ret; @@ -178,7 +99,7 @@ START_TEST(test_private_ccache_dir_in_wrong_user_dir) filename = talloc_asprintf(tmp_ctx, "%s/ccfile", subdirname); ck_assert_msg(filename != NULL, "talloc_asprintf failed."); - ret = sss_krb5_precreate_ccache(filename, 12345, 12345); + 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."); @@ -235,48 +156,6 @@ START_TEST(test_illegal_patterns) } END_TEST -START_TEST(test_cc_dir_create) -{ - char *residual; - char *dirname; - char *cwd; - uid_t uid = getuid(); - gid_t gid = getgid(); - errno_t ret; - - cwd = getcwd(NULL, 0); - ck_assert_msg(cwd != NULL, "getcwd failed."); - - dirname = talloc_asprintf(tmp_ctx, "%s/%s/user_dir", - cwd, TESTS_PATH); - ck_assert_msg(dirname != NULL, "talloc_asprintf failed."); - residual = talloc_asprintf(tmp_ctx, "DIR:%s/%s", dirname, "ccdir"); - ck_assert_msg(residual != NULL, "talloc_asprintf failed."); - - ret = sss_krb5_precreate_ccache(residual, uid, gid); - ck_assert_msg(ret == EOK, "sss_krb5_precreate_ccache failed\n"); - ret = rmdir(dirname); - if (ret < 0) ret = errno; - ck_assert_msg(ret == 0, "Cannot remove %s: %s\n", dirname, strerror(ret)); - talloc_free(residual); - - dirname = talloc_asprintf(tmp_ctx, "%s/%s/user_dir2", - cwd, TESTS_PATH); - ck_assert_msg(dirname != NULL, "talloc_asprintf failed."); - residual = talloc_asprintf(tmp_ctx, "DIR:%s/%s", dirname, "ccdir/"); - ck_assert_msg(residual != NULL, "talloc_asprintf failed."); - - ret = sss_krb5_precreate_ccache(residual, uid, gid); - ck_assert_msg(ret == EOK, "sss_krb5_precreate_ccache failed\n"); - ret = rmdir(dirname); - if (ret < 0) ret = errno; - ck_assert_msg(ret == 0, "Cannot remove %s: %s\n", dirname, strerror(ret)); - talloc_free(residual); - free(cwd); -} -END_TEST - - void setup_talloc_context(void) { int ret; @@ -771,9 +650,7 @@ 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); - tcase_add_test (tc_create_dir, test_cc_dir_create); if (getuid() == 0) { - tcase_add_test (tc_create_dir, test_private_ccache_dir_in_user_dir); tcase_add_test (tc_create_dir, test_private_ccache_dir_in_wrong_user_dir); } else { printf("Run as root to enable more tests.\n");