URL: https://github.com/freeipa/freeipa/pull/6080 Author: abbra Title: #6080: Harden PAC processing leftovers Action: opened
PR body: """ Two tests failed in nightlies after PAC hardening was merged. These patches address those changes. """ To pull the PR as Git branch: git remote add ghfreeipa https://github.com/freeipa/freeipa git fetch ghfreeipa pull/6080/head:pr6080 git checkout pr6080
From 4338c4ed8df804955c527e750ef4e082194ab58f Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Tue, 28 Sep 2021 10:24:32 +0300 Subject: [PATCH 1/9] ipa-kdb: store SID in the principal entry If the principal entry in LDAP has SID associated with it, store it to be able to quickly assess the SID when processing PAC. Also rename string_to_sid to IPA-specific version as it uses different prototype than Samba version. Fixes: https://pagure.io/freeipa/issue/9031 Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> Reviewed-by: Andreas Schneider <a...@samba.org> Reviewed-by: Robert Crittenden <rcrit...@redhat.com> --- daemons/ipa-kdb/ipa_kdb.h | 7 ++++++ daemons/ipa-kdb/ipa_kdb_mspac.c | 31 ++++++++++++++++++------- daemons/ipa-kdb/ipa_kdb_mspac_private.h | 1 - daemons/ipa-kdb/ipa_kdb_principals.c | 25 ++++++++++++++++++++ daemons/ipa-kdb/tests/ipa_kdb_tests.c | 30 ++++++++++++------------ 5 files changed, 69 insertions(+), 25 deletions(-) diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h index 66a1d74f138..884dff9500b 100644 --- a/daemons/ipa-kdb/ipa_kdb.h +++ b/daemons/ipa-kdb/ipa_kdb.h @@ -79,6 +79,7 @@ #define IPA_USER_AUTH_TYPE "ipaUserAuthType" struct ipadb_mspac; +struct dom_sid; enum ipadb_user_auth { IPADB_USER_AUTH_NONE = 0, @@ -155,6 +156,8 @@ struct ipadb_e_data { bool has_tktpolaux; enum ipadb_user_auth user_auth; struct ipadb_e_pol_limits pol_limits[IPADB_USER_AUTH_IDX_MAX]; + bool has_sid; + struct dom_sid *sid; }; struct ipadb_context *ipadb_get_context(krb5_context kcontext); @@ -366,3 +369,7 @@ int ipadb_get_enc_salt_types(struct ipadb_context *ipactx, LDAPMessage *entry, /* CERTAUTH PLUGIN */ void ipa_certauth_free_moddata(krb5_certauth_moddata *moddata); #endif + +int ipadb_string_to_sid(const char *str, struct dom_sid *sid); +void alloc_sid(struct dom_sid **sid); +void free_sid(struct dom_sid **sid); \ No newline at end of file diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c index 47b12a16f33..f3e8657c27d 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c @@ -80,7 +80,20 @@ static char *memberof_pac_attrs[] = { #define AUTHZ_DATA_TYPE_PAD "PAD" #define AUTHZ_DATA_TYPE_NONE "NONE" -int string_to_sid(const char *str, struct dom_sid *sid) +void alloc_sid(struct dom_sid **sid) +{ + *sid = malloc(sizeof(struct dom_sid)); +} + +void free_sid(struct dom_sid **sid) +{ + if (sid != NULL && *sid != NULL) { + free(*sid); + *sid = NULL; + } +} + +int ipadb_string_to_sid(const char *str, struct dom_sid *sid) { unsigned long val; const char *s; @@ -372,7 +385,7 @@ static krb5_error_code ipadb_add_asserted_identity(struct ipadb_context *ipactx, /* For S4U2Self, add Service Asserted Identity SID * otherwise, add Authentication Authority Asserted Identity SID */ - ret = string_to_sid((flags & KRB5_KDB_FLAG_PROTOCOL_TRANSITION) ? + ret = ipadb_string_to_sid((flags & KRB5_KDB_FLAG_PROTOCOL_TRANSITION) ? "S-1-18-2" : "S-1-18-1", arr[sidcount].sid); if (ret) { @@ -655,7 +668,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, /* SID is mandatory */ return ret; } - ret = string_to_sid(strres, &sid); + ret = ipadb_string_to_sid(strres, &sid); free(strres); if (ret) { return ret; @@ -700,7 +713,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, } } if (strcasecmp(dval->type, "ipaNTSecurityIdentifier") == 0) { - ret = string_to_sid((char *)dval->vals[0].bv_val, &gsid); + ret = ipadb_string_to_sid((char *)dval->vals[0].bv_val, &gsid); if (ret) { continue; } @@ -1189,7 +1202,7 @@ static int map_groups(TALLOC_CTX *memctx, krb5_context kcontext, } if (strcasecmp(dval->type, "ipaNTSecurityIdentifier") == 0) { - kerr = string_to_sid((char *)dval->vals[0].bv_val, &sid); + kerr = ipadb_string_to_sid((char *)dval->vals[0].bv_val, &sid); if (kerr != 0) { continue; } @@ -2434,7 +2447,7 @@ ipadb_adtrusts_fill_sid_blacklist(char **source_sid_blacklist, } for (i = 0; i < len; i++) { - (void) string_to_sid(source[i], &sid_blacklist[i]); + (void) ipadb_string_to_sid(source[i], &sid_blacklist[i]); } *result_sids = sid_blacklist; @@ -2594,7 +2607,7 @@ ipadb_mspac_get_trusted_domains(struct ipadb_context *ipactx) goto done; } - ret = string_to_sid(t[n].domain_sid, &t[n].domsid); + ret = ipadb_string_to_sid(t[n].domain_sid, &t[n].domsid); if (ret && t[n].domain_sid != NULL) { ret = EINVAL; goto done; @@ -2812,7 +2825,7 @@ krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx, bool force_rein goto done; } - ret = string_to_sid(resstr, &ipactx->mspac->domsid); + ret = ipadb_string_to_sid(resstr, &ipactx->mspac->domsid); if (ret) { kerr = ret; free(resstr); @@ -2865,7 +2878,7 @@ krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx, bool force_rein goto done; } if (ret == 0) { - ret = string_to_sid(resstr, &gsid); + ret = ipadb_string_to_sid(resstr, &gsid); if (ret) { free(resstr); kerr = ret; diff --git a/daemons/ipa-kdb/ipa_kdb_mspac_private.h b/daemons/ipa-kdb/ipa_kdb_mspac_private.h index 8c8a3a00111..3696c3c6ce6 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac_private.h +++ b/daemons/ipa-kdb/ipa_kdb_mspac_private.h @@ -51,7 +51,6 @@ struct ipadb_adtrusts { size_t *upn_suffixes_len; }; -int string_to_sid(const char *str, struct dom_sid *sid); char *dom_sid_string(TALLOC_CTX *memctx, const struct dom_sid *dom_sid); krb5_error_code filter_logon_info(krb5_context context, TALLOC_CTX *memctx, krb5_data realm, struct PAC_LOGON_INFO_CTR *info); diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c index 0a98ff054ab..15f3df4fee8 100644 --- a/daemons/ipa-kdb/ipa_kdb_principals.c +++ b/daemons/ipa-kdb/ipa_kdb_principals.c @@ -79,6 +79,8 @@ static char *std_principal_attrs[] = { "ipatokenRadiusConfigLink", "krbAuthIndMaxTicketLife", "krbAuthIndMaxRenewableAge", + "ipaNTSecurityIdentifier", + "ipaUniqueID", "objectClass", NULL @@ -594,6 +596,7 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext, char *restring; char *uidstring; char **authz_data_list; + char *princ_sid; krb5_timestamp restime; bool resbool; int result; @@ -963,6 +966,27 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext, ipadb_parse_authind_policies(kcontext, lcontext, lentry, entry, ua); } + /* Add SID if it is associated with the principal account */ + ied->has_sid = false; + ied->sid = NULL; + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, + "ipaNTSecurityIdentifier", &princ_sid); + if (ret == 0 && princ_sid != NULL) { + alloc_sid(&ied->sid); + if (ied->sid == NULL) { + kerr = KRB5_KDB_INTERNAL_ERROR; + free(princ_sid); + goto done; + } + ret = ipadb_string_to_sid(princ_sid, ied->sid); + free(princ_sid); + if (ret != 0) { + kerr = ret; + goto done; + } + ied->has_sid = true; + } + kerr = 0; done: @@ -1568,6 +1592,7 @@ void ipadb_free_principal_e_data(krb5_context kcontext, krb5_octet *e_data) } free(ied->authz_data); free(ied->pol); + free_sid(&ied->sid); free(ied); } } diff --git a/daemons/ipa-kdb/tests/ipa_kdb_tests.c b/daemons/ipa-kdb/tests/ipa_kdb_tests.c index 0b51ffb96e9..d38dfd841a0 100644 --- a/daemons/ipa-kdb/tests/ipa_kdb_tests.c +++ b/daemons/ipa-kdb/tests/ipa_kdb_tests.c @@ -105,7 +105,7 @@ static int setup(void **state) /* make sure data is not read from LDAP */ ipa_ctx->mspac->last_update = time(NULL) - 1; - ret = string_to_sid(DOM_SID, &ipa_ctx->mspac->domsid); + ret = ipadb_string_to_sid(DOM_SID, &ipa_ctx->mspac->domsid); assert_int_equal(ret, 0); ipa_ctx->mspac->num_trusts = 1; @@ -121,7 +121,7 @@ static int setup(void **state) ipa_ctx->mspac->trusts[0].domain_sid = strdup(DOM_SID_TRUST); assert_non_null(ipa_ctx->mspac->trusts[0].domain_sid); - ret = string_to_sid(DOM_SID_TRUST, &ipa_ctx->mspac->trusts[0].domsid); + ret = ipadb_string_to_sid(DOM_SID_TRUST, &ipa_ctx->mspac->trusts[0].domsid); assert_int_equal(ret, 0); ipa_ctx->mspac->trusts[0].len_sid_blocklist_incoming = 1; @@ -129,7 +129,7 @@ static int setup(void **state) ipa_ctx->mspac->trusts[0].len_sid_blocklist_incoming, sizeof(struct dom_sid)); assert_non_null(ipa_ctx->mspac->trusts[0].sid_blocklist_incoming); - ret = string_to_sid(BLOCKLIST_SID, + ret = ipadb_string_to_sid(BLOCKLIST_SID, &ipa_ctx->mspac->trusts[0].sid_blocklist_incoming[0]); assert_int_equal(ret, 0); @@ -216,7 +216,7 @@ static void test_filter_logon_info(void **state) assert_int_equal(kerr, EINVAL); /* wrong domain SID */ - ret = string_to_sid("S-1-5-21-1-1-1", &dom_sid); + ret = ipadb_string_to_sid("S-1-5-21-1-1-1", &dom_sid); assert_int_equal(ret, 0); info->info->info3.base.domain_sid = &dom_sid; @@ -224,7 +224,7 @@ static void test_filter_logon_info(void **state) assert_int_equal(kerr, EINVAL); /* matching domain SID */ - ret = string_to_sid(DOM_SID_TRUST, &dom_sid); + ret = ipadb_string_to_sid(DOM_SID_TRUST, &dom_sid); assert_int_equal(ret, 0); info->info->info3.base.domain_sid = &dom_sid; @@ -292,7 +292,7 @@ static void test_filter_logon_info(void **state) } for (d = 0; d < info->info->info3.sidcount; d++) { - ret = string_to_sid(test_data[c].sids[d], + ret = ipadb_string_to_sid(test_data[c].sids[d], info->info->info3.sids[d].sid); assert_int_equal(ret, 0); } @@ -434,7 +434,7 @@ static void test_get_authz_data_types(void **state) krb5_free_principal(test_ctx->krb5_ctx, non_nfs_princ); } -static void test_string_to_sid(void **state) +static void test_ipadb_string_to_sid(void **state) { int ret; struct dom_sid sid; @@ -442,25 +442,25 @@ static void test_string_to_sid(void **state) {21, 2127521184, 1604012920, 1887927527, 72713, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}; - ret = string_to_sid(NULL, &sid); + ret = ipadb_string_to_sid(NULL, &sid); assert_int_equal(ret, EINVAL); - ret = string_to_sid("abc", &sid); + ret = ipadb_string_to_sid("abc", &sid); assert_int_equal(ret, EINVAL); - ret = string_to_sid("S-", &sid); + ret = ipadb_string_to_sid("S-", &sid); assert_int_equal(ret, EINVAL); - ret = string_to_sid("S-ABC", &sid); + ret = ipadb_string_to_sid("S-ABC", &sid); assert_int_equal(ret, EINVAL); - ret = string_to_sid("S-123", &sid); + ret = ipadb_string_to_sid("S-123", &sid); assert_int_equal(ret, EINVAL); - ret = string_to_sid("S-1-123-1-2-3-4-5-6-7-8-9-0-1-2-3-4-5-6", &sid); + ret = ipadb_string_to_sid("S-1-123-1-2-3-4-5-6-7-8-9-0-1-2-3-4-5-6", &sid); assert_int_equal(ret, EINVAL); - ret = string_to_sid("S-1-5-21-2127521184-1604012920-1887927527-72713", + ret = ipadb_string_to_sid("S-1-5-21-2127521184-1604012920-1887927527-72713", &sid); assert_int_equal(ret, 0); assert_memory_equal(&exp_sid, &sid, sizeof(struct dom_sid)); @@ -531,7 +531,7 @@ int main(int argc, const char *argv[]) setup, teardown), cmocka_unit_test_setup_teardown(test_filter_logon_info, setup, teardown), - cmocka_unit_test(test_string_to_sid), + cmocka_unit_test(test_ipadb_string_to_sid), cmocka_unit_test_setup_teardown(test_dom_sid_string, setup, teardown), cmocka_unit_test_setup_teardown(test_check_trusted_realms, From f81c8b89f0da45929208abbdfe5bd85b7ba41f90 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Tue, 28 Sep 2021 10:26:30 +0300 Subject: [PATCH 2/9] ipa-kdb: enforce SID checks when generating PAC Check that a domain SID and a user SID in the PAC passed to us are what they should be for the local realm's principal. Fixes: https://pagure.io/freeipa/issue/9031 Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> Reviewed-by: Andreas Schneider <a...@samba.org> Reviewed-by: Robert Crittenden <rcrit...@redhat.com> --- daemons/ipa-kdb/ipa_kdb.h | 3 +- daemons/ipa-kdb/ipa_kdb_mspac.c | 112 ++++++++++++++++++++++++++++---- 2 files changed, 100 insertions(+), 15 deletions(-) diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h index 884dff9500b..708e9545e86 100644 --- a/daemons/ipa-kdb/ipa_kdb.h +++ b/daemons/ipa-kdb/ipa_kdb.h @@ -372,4 +372,5 @@ void ipa_certauth_free_moddata(krb5_certauth_moddata *moddata); int ipadb_string_to_sid(const char *str, struct dom_sid *sid); void alloc_sid(struct dom_sid **sid); -void free_sid(struct dom_sid **sid); \ No newline at end of file +void free_sid(struct dom_sid **sid); +bool dom_sid_check(const struct dom_sid *sid1, const struct dom_sid *sid2, bool exact_check); \ No newline at end of file diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c index f3e8657c27d..59b77f5d819 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c @@ -236,7 +236,7 @@ static struct dom_sid *dom_sid_dup(TALLOC_CTX *memctx, * dom_sid_check() is supposed to be used with sid1 representing domain SID * and sid2 being either domain or resource SID in the domain */ -static bool dom_sid_check(const struct dom_sid *sid1, const struct dom_sid *sid2, bool exact_check) +bool dom_sid_check(const struct dom_sid *sid1, const struct dom_sid *sid2, bool exact_check) { int c, num; @@ -1404,6 +1404,82 @@ static void filter_logon_info_log_message_rid(struct dom_sid *sid, uint32_t rid) } } +static krb5_error_code check_logon_info_consistent(krb5_context context, + TALLOC_CTX *memctx, + krb5_const_principal client_princ, + struct PAC_LOGON_INFO_CTR *info) +{ + krb5_error_code kerr = 0; + struct ipadb_context *ipactx; + bool result; + krb5_db_entry *client_actual = NULL; + struct ipadb_e_data *ied = NULL; + int flags = 0; +#ifdef KRB5_KDB_FLAG_ALIAS_OK + flags = KRB5_KDB_FLAG_ALIAS_OK; +#endif + + ipactx = ipadb_get_context(context); + if (!ipactx || !ipactx->mspac) { + return KRB5_KDB_DBNOTINITED; + } + + /* check exact sid */ + result = dom_sid_check(&ipactx->mspac->domsid, + info->info->info3.base.domain_sid, true); + if (!result) { + /* memctx is freed by the caller */ + char *sid = dom_sid_string(memctx, info->info->info3.base.domain_sid); + char *dom = dom_sid_string(memctx, &ipactx->mspac->domsid); + krb5_klog_syslog(LOG_ERR, "PAC issue: PAC record claims domain SID different " + "to local domain SID: local [%s], PAC [%s]", + dom ? dom : "<failed to display>", + sid ? sid : "<failed to display>"); + return KRB5KDC_ERR_POLICY; + } + + kerr = ipadb_get_principal(context, client_princ, flags, &client_actual); + if (kerr != 0) { + krb5_klog_syslog(LOG_ERR, "PAC issue: ipadb_get_principal failed."); + return KRB5KDC_ERR_POLICY; + } + + ied = (struct ipadb_e_data *)client_actual->e_data; + if (ied == NULL || ied->magic != IPA_E_DATA_MAGIC) { + krb5_klog_syslog(LOG_ERR, "PAC issue: client e_data fetching failed."); + kerr = EINVAL; + goto done; + } + + if (!ied->has_sid || ied->sid == NULL) { + /* Kerberos principal might have no SID associated in the DB entry. + * If this is host or service, we'll associate RID -515 or -516 in PAC + * depending on whether this is a domain member or domain controller + * but since this is not recorded in the DB entry, we the check for + * SID is not needed */ + goto done; + } + + result = dom_sid_check(ied->sid, info->info->info3.sids[0].sid, true); + if (!result) { + /* memctx is freed by the caller */ + char *local_sid = dom_sid_string(memctx, ied->sid); + char *pac_sid = dom_sid_string(memctx, info->info->info3.sids[0].sid); + krb5_klog_syslog(LOG_ERR, "PAC issue: client principal has a SID " + "different from what PAC claims. " + "local [%s] vs PAC [%s]", + local_sid ? local_sid : "<failed to display>", + pac_sid ? pac_sid : "<failed to display>"); + kerr = KRB5KDC_ERR_POLICY; + goto done; + } + +done: + ipadb_free_principal(context, client_actual); + + return kerr; +} + krb5_error_code filter_logon_info(krb5_context context, TALLOC_CTX *memctx, krb5_data realm, @@ -1631,12 +1707,14 @@ krb5_error_code filter_logon_info(krb5_context context, static krb5_error_code ipadb_check_logon_info(krb5_context context, - krb5_data origin_realm, + krb5_const_principal client_princ, + krb5_boolean is_cross_realm, krb5_data *pac_blob) { struct PAC_LOGON_INFO_CTR info; krb5_error_code kerr; TALLOC_CTX *tmpctx; + krb5_data origin_realm = client_princ->realm; tmpctx = talloc_new(NULL); if (!tmpctx) { @@ -1648,6 +1726,13 @@ static krb5_error_code ipadb_check_logon_info(krb5_context context, goto done; } + if (!is_cross_realm) { + /* For local realm case we need to check whether the PAC is for our user + * but we don't need to process further */ + kerr = check_logon_info_consistent(context, tmpctx, client_princ, &info); + goto done; + } + kerr = filter_logon_info(context, tmpctx, origin_realm, &info); if (kerr) { goto done; @@ -1873,19 +1958,18 @@ static krb5_error_code ipadb_verify_pac(krb5_context context, goto done; } - /* Now that the PAC is verified augment it with additional info if - * it is coming from a different realm */ - if (is_cross_realm) { - kerr = krb5_pac_get_buffer(context, old_pac, - KRB5_PAC_LOGON_INFO, &pac_blob); - if (kerr != 0) { - goto done; - } + /* Now that the PAC is verified, do additional checks. + * Augment it with additional info if it is coming from a different realm */ + kerr = krb5_pac_get_buffer(context, old_pac, + KRB5_PAC_LOGON_INFO, &pac_blob); + if (kerr != 0) { + goto done; + } - kerr = ipadb_check_logon_info(context, client_princ->realm, &pac_blob); - if (kerr != 0) { - goto done; - } + kerr = ipadb_check_logon_info(context, + client_princ, is_cross_realm, &pac_blob); + if (kerr != 0) { + goto done; } /* extract buffers and rebuilt pac from scratch so that when re-signing * with a different cksum type does not cause issues due to mismatching From 98b9d6e9629b19b71f5429364d3abb54529e4d6e Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Thu, 14 Oct 2021 11:28:02 +0300 Subject: [PATCH 3/9] ipa-kdb: use entry DN to compare aliased entries in S4U operations When working with aliased entries, we need a reliable way to detect whether two principals reference the same database entry. This is important in S4U checks. Ideally, we should be using SIDs for these checks as S4U requires PAC record presence which cannot be issued without a SID associated with an entry. This is true for user principals and a number of host/service principals associated with Samba. Other service principals do not have SIDs because we do not allocate POSIX IDs to them in FreeIPA. When PAC is issued for these principals, they get SID of a domain computer or domain controller depending on their placement (IPA client or IPA server). Since 389-ds always returns unique entry DN for the same entry, rely on this value instead. We could have used ipaUniqueID but for Kerberos principals created through the KDB (kadmin/kdb5_util) we don't have ipaUniqueID in the entry. Fixes: https://pagure.io/freeipa/issue/9031 Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> Reviewed-by: Rob Crittenden <rcrit...@redhat.com> --- daemons/ipa-kdb/ipa_kdb_delegation.c | 36 +++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/daemons/ipa-kdb/ipa_kdb_delegation.c b/daemons/ipa-kdb/ipa_kdb_delegation.c index 5ae5e0d9d09..bfa344b9fd1 100644 --- a/daemons/ipa-kdb/ipa_kdb_delegation.c +++ b/daemons/ipa-kdb/ipa_kdb_delegation.c @@ -21,6 +21,8 @@ */ #include "ipa_kdb.h" +#include <strings.h> +#include <unicase.h> static char *acl_attrs[] = { "objectClass", @@ -188,10 +190,41 @@ krb5_error_code ipadb_check_allowed_to_delegate(krb5_context kcontext, const krb5_db_entry *server, krb5_const_principal proxy) { - krb5_error_code kerr; + krb5_error_code kerr, result; char *srv_principal = NULL; + krb5_db_entry *proxy_entry = NULL; + struct ipadb_e_data *ied_server, *ied_proxy; LDAPMessage *res = NULL; + /* Handle the case where server == proxy, this is allowed in S4U*/ + kerr = ipadb_get_principal(kcontext, proxy, + KRB5_KDB_FLAG_CLIENT_REFERRALS_ONLY, + &proxy_entry); + if (kerr) { + goto done; + } + + ied_server = (struct ipadb_e_data *) server->e_data; + ied_proxy = (struct ipadb_e_data *) proxy_entry->e_data; + + /* If we have SIDs for both entries, compare SIDs */ + if ((ied_server->has_sid && ied_server->sid != NULL) && + (ied_proxy->has_sid && ied_proxy->sid != NULL)) { + + if (dom_sid_check(ied_server->sid, ied_proxy->sid, true)) { + kerr = 0; + goto done; + } + } + + /* Otherwise, compare entry DNs */ + kerr = ulc_casecmp(ied_server->entry_dn, strlen(ied_server->entry_dn), + ied_proxy->entry_dn, strlen(ied_proxy->entry_dn), + NULL, NULL, &result); + if (kerr == 0 && result == 0) { + goto done; + } + kerr = krb5_unparse_name(kcontext, server->princ, &srv_principal); if (kerr) { goto done; @@ -208,6 +241,7 @@ krb5_error_code ipadb_check_allowed_to_delegate(krb5_context kcontext, } done: + ipadb_free_principal(kcontext, proxy_entry); krb5_free_unparsed_name(kcontext, srv_principal); ldap_msgfree(res); return kerr; From 5ae6261683fbd7d83982078397f4580a6663a0b1 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Thu, 28 Oct 2021 11:01:08 +0300 Subject: [PATCH 4/9] ipa-kdb: S4U2Proxy target should use a service name without realm According to new Samba Kerberos tests and [MS-SFU] 3.2.5.2.4 'KDC Replies with Service Ticket', the target should not include the realm. Fixes: https://pagure.io/freeipa/issue/9031 Pair-programmed-with: Andreas Schneider <a...@redhat.com> Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> Signed-off-by: Andreas Schneider <a...@redhat.com> Reviewed-by: Rob Crittenden <rcrit...@redhat.com> --- daemons/ipa-kdb/ipa_kdb_mspac.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c index 59b77f5d819..f729b9f2ecd 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c @@ -1847,7 +1847,10 @@ static krb5_error_code ipadb_add_transited_service(krb5_context context, krb5_free_data_contents(context, &pac_blob); memset(&pac_blob, 0, sizeof(krb5_data)); - kerr = krb5_unparse_name(context, proxy->princ, &tmpstr); + kerr = krb5_unparse_name_flags(context, proxy->princ, + KRB5_PRINCIPAL_UNPARSE_NO_REALM | + KRB5_PRINCIPAL_UNPARSE_DISPLAY, + &tmpstr); if (kerr != 0) { goto done; } From 933b958786f9c9d414a318ae1c10df8b9101e296 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Sat, 30 Oct 2021 10:08:34 +0300 Subject: [PATCH 5/9] ipa-kdb: add support for PAC_UPN_DNS_INFO_EX CVE-2020-25721 mitigation: KDC must provide the new HAS_SAM_NAME_AND_SID buffer with sAMAccountName and ObjectSID values associated with the principal. The mitigation only works if NDR library supports the PAC_UPN_DNS_INFO_EX buffer type. In case we cannot detect it at compile time, a warning will be displayed at configure stage. Fixes: https://pagure.io/freeipa/issue/9031 Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> Reviewed-by: Rob Crittenden <rcrit...@redhat.com> --- daemons/ipa-kdb/ipa_kdb_mspac.c | 41 +++++++++++++++++++++++++++++++-- server.m4 | 7 ++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c index f729b9f2ecd..75cb4f3b766 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c @@ -812,6 +812,25 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, return ret; } +static krb5_error_code ipadb_get_sid_from_pac(TALLOC_CTX *ctx, + struct PAC_LOGON_INFO *info, + struct dom_sid *sid) +{ + struct dom_sid *client_sid = NULL; + /* Construct SID from the PAC */ + if (info->info3.base.rid == 0) { + client_sid = info->info3.sids[0].sid; + } else { + client_sid = dom_sid_dup(ctx, info->info3.base.domain_sid); + if (!client_sid) { + return ENOMEM; + } + sid_append_rid(client_sid, info->info3.base.rid); + } + *sid = *client_sid; + return 0; +} + static krb5_error_code ipadb_get_pac(krb5_context kcontext, krb5_db_entry *client, unsigned int flags, @@ -830,6 +849,7 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext, enum ndr_err_code ndr_err; union PAC_INFO pac_upn; char *principal = NULL; + struct dom_sid client_sid; /* When no client entry is there, we cannot generate MS-PAC */ if (!client) { @@ -930,6 +950,18 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext, pac_upn.upn_dns_info.flags |= PAC_UPN_DNS_FLAG_CONSTRUCTED; } + kerr = ipadb_get_sid_from_pac(tmpctx, pac_info.logon_info.info, &client_sid); + if (kerr) { + goto done; + } + +#ifdef HAVE_PAC_UPN_DNS_INFO_EX + /* Add samAccountName and a SID */ + pac_upn.upn_dns_info.flags |= PAC_UPN_DNS_FLAG_HAS_SAM_NAME_AND_SID; + pac_upn.upn_dns_info.ex.sam_name_and_sid.samaccountname = pac_info.logon_info.info->info3.base.account_name.string; + pac_upn.upn_dns_info.ex.sam_name_and_sid.objectsid = &client_sid; +#endif + ndr_err = ndr_push_union_blob(&pac_data, tmpctx, &pac_upn, PAC_TYPE_UPN_DNS_INFO, (ndr_push_flags_fn_t)ndr_push_PAC_INFO); @@ -1415,6 +1447,7 @@ static krb5_error_code check_logon_info_consistent(krb5_context context, krb5_db_entry *client_actual = NULL; struct ipadb_e_data *ied = NULL; int flags = 0; + struct dom_sid client_sid; #ifdef KRB5_KDB_FLAG_ALIAS_OK flags = KRB5_KDB_FLAG_ALIAS_OK; #endif @@ -1460,11 +1493,15 @@ static krb5_error_code check_logon_info_consistent(krb5_context context, goto done; } - result = dom_sid_check(ied->sid, info->info->info3.sids[0].sid, true); + kerr = ipadb_get_sid_from_pac(memctx, info->info, &client_sid); + if (kerr) { + goto done; + } + result = dom_sid_check(ied->sid, &client_sid, true); if (!result) { /* memctx is freed by the caller */ char *local_sid = dom_sid_string(memctx, ied->sid); - char *pac_sid = dom_sid_string(memctx, info->info->info3.sids[0].sid); + char *pac_sid = dom_sid_string(memctx, &client_sid); krb5_klog_syslog(LOG_ERR, "PAC issue: client principal has a SID " "different from what PAC claims. " "local [%s] vs PAC [%s]", diff --git a/server.m4 b/server.m4 index 3a2c3ae336c..65c82d25af7 100644 --- a/server.m4 +++ b/server.m4 @@ -101,6 +101,13 @@ AC_CHECK_MEMBER( [AC_MSG_NOTICE([struct PAC_DOMAIN_GROUP_MEMBERSHIP is not available])], [[#include <ndr.h> #include <gen_ndr/krb5pac.h>]]) +AC_CHECK_MEMBER( + [struct PAC_UPN_DNS_INFO.ex], + [AC_DEFINE([HAVE_PAC_UPN_DNS_INFO_EX], [1], + [union PAC_UPN_DNS_INFO_EX is available.])], + [AC_MSG_NOTICE([union PAC_UPN_DNS_INFO_EX is not available, account protection is not active])], + [[#include <ndr.h> + #include <gen_ndr/krb5pac.h>]]) CFLAGS="$bck_cflags" From 1db8424858136675091f63054bafceaa96a9fdd8 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Sat, 30 Oct 2021 10:09:27 +0300 Subject: [PATCH 6/9] ipa-kdb: add support for PAC_REQUESTER_SID buffer CVE-2020-25721 mitigation: KDC must provide the new PAC_REQUESTER_SID buffer with ObjectSID value associated with the requester's principal. The mitigation only works if NDR library supports the PAC_REQUESTER_SID buffer type. In case we cannot detect it at compile time, a warning will be displayed at configure stage. Fixes: https://pagure.io/freeipa/issue/9031 Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> Reviewed-by: Rob Crittenden <rcrit...@redhat.com> --- daemons/ipa-kdb/ipa_kdb_mspac.c | 131 +++++++++++++++++++++++++++++++- server.m4 | 7 ++ 2 files changed, 134 insertions(+), 4 deletions(-) diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c index 75cb4f3b766..ca6daccf136 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c @@ -812,6 +812,55 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, return ret; } +#ifdef HAVE_PAC_REQUESTER_SID +static krb5_error_code ipadb_get_requester_sid(krb5_context context, + krb5_pac pac, + struct dom_sid *sid) +{ + enum ndr_err_code ndr_err; + krb5_error_code ret; + DATA_BLOB pac_requester_sid_in; + krb5_data k5pac_requester_sid_in; + union PAC_INFO info; + TALLOC_CTX *tmp_ctx; + struct ipadb_context *ipactx; + + ipactx = ipadb_get_context(context); + if (!ipactx) { + return KRB5_KDB_DBNOTINITED; + } + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + ret = krb5_pac_get_buffer(context, pac, PAC_TYPE_REQUESTER_SID, + &k5pac_requester_sid_in); + if (ret != 0) { + talloc_free(tmp_ctx); + return ret; + } + + pac_requester_sid_in = data_blob_const(k5pac_requester_sid_in.data, + k5pac_requester_sid_in.length); + + ndr_err = ndr_pull_union_blob(&pac_requester_sid_in, tmp_ctx, &info, + PAC_TYPE_REQUESTER_SID, + (ndr_pull_flags_fn_t)ndr_pull_PAC_INFO); + krb5_free_data_contents(context, &k5pac_requester_sid_in); + if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { + talloc_free(tmp_ctx); + return EINVAL; + } + + *sid = info.requester_sid.sid; + + talloc_free(tmp_ctx); + return 0; +} +#endif + static krb5_error_code ipadb_get_sid_from_pac(TALLOC_CTX *ctx, struct PAC_LOGON_INFO *info, struct dom_sid *sid) @@ -976,6 +1025,33 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext, kerr = krb5_pac_add_buffer(kcontext, *pac, KRB5_PAC_UPN_DNS_INFO, &data); +#ifdef HAVE_PAC_REQUESTER_SID + { + union PAC_INFO pac_requester_sid; + /* == Package PAC_REQUESTER_SID == */ + memset(&pac_requester_sid, 0, sizeof(pac_requester_sid)); + + pac_requester_sid.requester_sid.sid = client_sid; + + ndr_err = ndr_push_union_blob(&pac_data, tmpctx, &pac_requester_sid, + PAC_TYPE_REQUESTER_SID, + (ndr_push_flags_fn_t)ndr_push_PAC_INFO); + if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { + kerr = KRB5_KDB_INTERNAL_ERROR; + goto done; + } + + data.magic = KV5M_DATA; + data.data = (char *)pac_data.data; + data.length = pac_data.length; + + kerr = krb5_pac_add_buffer(kcontext, *pac, PAC_TYPE_REQUESTER_SID, &data); + if (kerr) { + goto done; + } + } +#endif + done: ldap_msgfree(results); talloc_free(tmpctx); @@ -1457,7 +1533,19 @@ static krb5_error_code check_logon_info_consistent(krb5_context context, return KRB5_KDB_DBNOTINITED; } - /* check exact sid */ + /* We are asked to verify the PAC for our own principal, + * check that our own view on the PAC details is up to date */ + if (ipactx->mspac->domsid.num_auths == 0) { + /* Force re-init of KDB's view on our domain */ + kerr = ipadb_reinit_mspac(ipactx, true); + if (kerr != 0) { + krb5_klog_syslog(LOG_ERR, + "PAC issue: unable to update realm's view on PAC info"); + return KRB5KDC_ERR_POLICY; + } + } + + /* check exact domain SID */ result = dom_sid_check(&ipactx->mspac->domsid, info->info->info3.base.domain_sid, true); if (!result) { @@ -1466,7 +1554,7 @@ static krb5_error_code check_logon_info_consistent(krb5_context context, char *dom = dom_sid_string(memctx, &ipactx->mspac->domsid); krb5_klog_syslog(LOG_ERR, "PAC issue: PAC record claims domain SID different " "to local domain SID: local [%s], PAC [%s]", - dom ? dom : "<failed to display>", + dom ? dom : "<failed to display>", sid ? sid : "<failed to display>"); return KRB5KDC_ERR_POLICY; } @@ -1746,12 +1834,14 @@ krb5_error_code filter_logon_info(krb5_context context, static krb5_error_code ipadb_check_logon_info(krb5_context context, krb5_const_principal client_princ, krb5_boolean is_cross_realm, - krb5_data *pac_blob) + krb5_data *pac_blob, + struct dom_sid *requester_sid) { struct PAC_LOGON_INFO_CTR info; krb5_error_code kerr; TALLOC_CTX *tmpctx; krb5_data origin_realm = client_princ->realm; + bool result; tmpctx = talloc_new(NULL); if (!tmpctx) { @@ -1763,6 +1853,28 @@ static krb5_error_code ipadb_check_logon_info(krb5_context context, goto done; } + /* Check that requester SID is the same as in the PAC entry */ + if (requester_sid != NULL) { + struct dom_sid client_sid; + kerr = ipadb_get_sid_from_pac(tmpctx, info.info, &client_sid); + if (kerr) { + goto done; + } + result = dom_sid_check(&client_sid, requester_sid, true); + if (!result) { + /* memctx is freed by the caller */ + char *pac_sid = dom_sid_string(tmpctx, &client_sid); + char *req_sid = dom_sid_string(tmpctx, requester_sid); + krb5_klog_syslog(LOG_ERR, "PAC issue: PAC has a SID " + "different from what PAC requester claims. " + "PAC [%s] vs PAC requester [%s]", + pac_sid ? pac_sid : "<failed to display>", + req_sid ? req_sid : "<failed to display>"); + kerr = KRB5KDC_ERR_POLICY; + goto done; + } + } + if (!is_cross_realm) { /* For local realm case we need to check whether the PAC is for our user * but we don't need to process further */ @@ -1962,6 +2074,8 @@ static krb5_error_code ipadb_verify_pac(krb5_context context, krb5_data pac_blob = { 0 , 0, NULL}; bool is_cross_realm = false; size_t i; + struct dom_sid *requester_sid = NULL; + struct dom_sid req_sid; kerr = krb5_pac_parse(context, authdata[0]->contents, @@ -2006,8 +2120,17 @@ static krb5_error_code ipadb_verify_pac(krb5_context context, goto done; } + memset(&req_sid, '\0', sizeof(struct dom_sid)); +#ifdef HAVE_PAC_REQUESTER_SID + kerr = ipadb_get_requester_sid(context, old_pac, &req_sid); + if (kerr == 0) { + requester_sid = &req_sid; + } +#endif + kerr = ipadb_check_logon_info(context, - client_princ, is_cross_realm, &pac_blob); + client_princ, is_cross_realm, &pac_blob, + requester_sid); if (kerr != 0) { goto done; } diff --git a/server.m4 b/server.m4 index 65c82d25af7..648423dd43e 100644 --- a/server.m4 +++ b/server.m4 @@ -109,6 +109,13 @@ AC_CHECK_MEMBER( [[#include <ndr.h> #include <gen_ndr/krb5pac.h>]]) +AC_CHECK_MEMBER( + [struct PAC_REQUESTER_SID.sid], + [AC_DEFINE([HAVE_PAC_REQUESTER_SID], [1], + [struct PAC_REQUESTER_SID is available.])], + [AC_MSG_NOTICE([struct PAC_REQUESTER_SID is not available, account protection is not active])], + [[#include <ndr.h> + #include <gen_ndr/krb5pac.h>]]) CFLAGS="$bck_cflags" LIBPDB_NAME="" From ce4ac4265c4a4f1378ca2067d537bc8cc3c6a7c8 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Sat, 30 Oct 2021 09:10:09 +0300 Subject: [PATCH 7/9] ipa-kdb: add PAC_ATTRIBUTES_INFO PAC buffer support PAC_ATTRIBUTES_INFO PAC buffer allows both client and KDC to tell whether a PAC structure was requested by the client or it was provided by the KDC implicitly. Kerberos service then can continue processing or deny access in case client explicitly requested to operate without PAC. Fixes: https://pagure.io/freeipa/issue/9031 Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> Signed-off-by: Andrew Bartlett <abart...@samba.org> --- daemons/ipa-kdb/Makefile.am | 2 + daemons/ipa-kdb/ipa_kdb_mspac.c | 143 ++++++++++++++++++++++++++++++++ server.m4 | 8 ++ 3 files changed, 153 insertions(+) diff --git a/daemons/ipa-kdb/Makefile.am b/daemons/ipa-kdb/Makefile.am index 14c0546e0a5..5775d408642 100644 --- a/daemons/ipa-kdb/Makefile.am +++ b/daemons/ipa-kdb/Makefile.am @@ -61,6 +61,7 @@ ipadb_la_LIBADD = \ $(UNISTRING_LIBS) \ $(SSSCERTMAP_LIBS) \ $(top_builddir)/util/libutil.la \ + -lsamba-errors \ $(NULL) if HAVE_CMOCKA @@ -104,6 +105,7 @@ ipa_kdb_tests_LDADD = \ -lkdb5 \ -lsss_idmap \ -lsamba-security-samba4 \ + -lsamba-errors \ $(NULL) appdir = $(libexecdir)/ipa diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c index ca6daccf136..8a77a3538e2 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c @@ -880,6 +880,87 @@ static krb5_error_code ipadb_get_sid_from_pac(TALLOC_CTX *ctx, return 0; } +#ifdef HAVE_PAC_ATTRIBUTES_INFO +static krb5_error_code ipadb_client_requested_pac(krb5_context context, + krb5_pac pac, + TALLOC_CTX *mem_ctx, + krb5_boolean *requested_pac) +{ + enum ndr_err_code ndr_err; + krb5_data k5pac_attrs_in; + DATA_BLOB pac_attrs_in; + union PAC_INFO pac_attrs; + krb5_error_code ret; + + *requested_pac = true; + + ret = krb5_pac_get_buffer(context, pac, PAC_TYPE_ATTRIBUTES_INFO, + &k5pac_attrs_in); + if (ret != 0) { + return ret == ENOENT ? 0 : ret; + } + + pac_attrs_in = data_blob_const(k5pac_attrs_in.data, + k5pac_attrs_in.length); + + ndr_err = ndr_pull_union_blob(&pac_attrs_in, mem_ctx, &pac_attrs, + PAC_TYPE_ATTRIBUTES_INFO, + (ndr_pull_flags_fn_t)ndr_pull_PAC_INFO); + krb5_free_data_contents(context, &k5pac_attrs_in); + if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { + NTSTATUS nt_status = ndr_map_error2ntstatus(ndr_err); + krb5_klog_syslog(LOG_ERR, "can't parse the PAC ATTRIBUTES_INFO: %s\n", + nt_errstr(nt_status)); + return KRB5_KDB_INTERNAL_ERROR; + } + + if (pac_attrs.attributes_info.flags & (PAC_ATTRIBUTE_FLAG_PAC_WAS_GIVEN_IMPLICITLY + | PAC_ATTRIBUTE_FLAG_PAC_WAS_REQUESTED)) { + *requested_pac = true; + } else { + *requested_pac = false; + } + + return 0; +} + +static krb5_error_code ipadb_get_pac_attrs_blob(TALLOC_CTX *mem_ctx, + const krb5_boolean *pac_request, + DATA_BLOB *pac_attrs_data) +{ + union PAC_INFO pac_attrs; + enum ndr_err_code ndr_err; + + memset(&pac_attrs, 0, sizeof(pac_attrs)); + + *pac_attrs_data = data_blob_null; + + /* Set the length of the flags in bits. */ + pac_attrs.attributes_info.flags_length = 2; + + if (pac_request == NULL) { + pac_attrs.attributes_info.flags + |= PAC_ATTRIBUTE_FLAG_PAC_WAS_GIVEN_IMPLICITLY; + } else if (*pac_request) { + pac_attrs.attributes_info.flags + |= PAC_ATTRIBUTE_FLAG_PAC_WAS_REQUESTED; + } + + ndr_err = ndr_push_union_blob(pac_attrs_data, mem_ctx, &pac_attrs, + PAC_TYPE_ATTRIBUTES_INFO, + (ndr_push_flags_fn_t)ndr_push_PAC_INFO); + if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { + NTSTATUS nt_status = ndr_map_error2ntstatus(ndr_err); + krb5_klog_syslog(LOG_ERR, "can't create PAC ATTRIBUTES_INFO: %s\n", + nt_errstr(nt_status)); + return KRB5_KDB_INTERNAL_ERROR; + } + + return 0; +} + +#endif + static krb5_error_code ipadb_get_pac(krb5_context kcontext, krb5_db_entry *client, unsigned int flags, @@ -1025,6 +1106,26 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext, kerr = krb5_pac_add_buffer(kcontext, *pac, KRB5_PAC_UPN_DNS_INFO, &data); +#ifdef HAVE_PAC_ATTRIBUTES_INFO + /* == Add implicit PAC type attributes info as we always try to generate PAC == */ + { + DATA_BLOB pac_attrs_data; + + kerr = ipadb_get_pac_attrs_blob(tmpctx, NULL, &pac_attrs_data); + if (kerr) { + goto done; + } + data.magic = KV5M_DATA; + data.data = (char *)pac_attrs_data.data; + data.length = pac_attrs_data.length; + + kerr = krb5_pac_add_buffer(kcontext, *pac, PAC_TYPE_ATTRIBUTES_INFO, &data); + if (kerr) { + goto done; + } + } +#endif + #ifdef HAVE_PAC_REQUESTER_SID { union PAC_INFO pac_requester_sid; @@ -2165,6 +2266,48 @@ static krb5_error_code ipadb_verify_pac(krb5_context context, continue; } +#ifdef HAVE_PAC_ATTRIBUTES_INFO + if (types[i] == PAC_TYPE_ATTRIBUTES_INFO && + pac_blob.length != 0) { + /* == Check whether PAC was requested or given implicitly == */ + DATA_BLOB pac_attrs_data; + krb5_boolean pac_requested; + + TALLOC_CTX *tmpctx = talloc_new(NULL); + if (tmpctx == NULL) { + kerr = ENOMEM; + krb5_pac_free(context, new_pac); + goto done; + } + + kerr = ipadb_client_requested_pac(context, old_pac, tmpctx, &pac_requested); + if (kerr != 0) { + talloc_free(tmpctx); + krb5_pac_free(context, new_pac); + goto done; + } + + kerr = ipadb_get_pac_attrs_blob(tmpctx, &pac_requested, &pac_attrs_data); + if (kerr) { + talloc_free(tmpctx); + krb5_pac_free(context, new_pac); + goto done; + } + data.magic = KV5M_DATA; + data.data = (char *)pac_attrs_data.data; + data.length = pac_attrs_data.length; + + kerr = krb5_pac_add_buffer(context, new_pac, PAC_TYPE_ATTRIBUTES_INFO, &data); + if (kerr) { + talloc_free(tmpctx); + krb5_pac_free(context, new_pac); + goto done; + } + + continue; + } +#endif + if (types[i] == KRB5_PAC_DELEGATION_INFO && (flags & KRB5_KDB_FLAG_CONSTRAINED_DELEGATION)) { /* skip it here, we will add it explicitly later */ diff --git a/server.m4 b/server.m4 index 648423dd43e..5a14af06aaa 100644 --- a/server.m4 +++ b/server.m4 @@ -116,6 +116,14 @@ AC_CHECK_MEMBER( [AC_MSG_NOTICE([struct PAC_REQUESTER_SID is not available, account protection is not active])], [[#include <ndr.h> #include <gen_ndr/krb5pac.h>]]) + +AC_CHECK_MEMBER( + [struct PAC_ATTRIBUTES_INFO.flags], + [AC_DEFINE([HAVE_PAC_ATTRIBUTES_INFO], [1], + [struct PAC_ATTRIBUTES_INFO is available.])], + [AC_MSG_NOTICE([struct PAC_ATTRIBUTES_INFO is not available, account protection is not active])], + [[#include <ndr.h> + #include <gen_ndr/krb5pac.h>]]) CFLAGS="$bck_cflags" LIBPDB_NAME="" From f14df66cc2b4deece440e42b265f38ede207a5d6 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Fri, 29 Oct 2021 22:30:53 +0300 Subject: [PATCH 8/9] ipa-kdb: Use proper account flags for Kerberos principal in PAC As part of CVE-2020-25717 mitigations, Samba expects correct user account flags in the PAC. This means for services and host principals we should be using ACB_WSTRUST or ACB_SVRTRUST depending on whether they run on IPA clients ("workstation" or "domain member") or IPA servers ("domain controller"). Fixes: https://pagure.io/freeipa/issue/9031 Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> --- daemons/ipa-kdb/ipa_kdb_mspac.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c index 8a77a3538e2..0e0ee36166e 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c @@ -648,6 +648,11 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, info3->base.logon_count = 0; /* we do not have this info yet */ info3->base.bad_password_count = 0; /* we do not have this info yet */ + /* Use AES keys by default to detect changes. + * This bit is not used by Windows clients and servers so we can + * clear it after detecting the changes */ + info3->base.acct_flags = ACB_USE_AES_KEYS; + if ((is_host || is_service)) { /* it is either host or service, so get the hostname first */ char *sep = strchr(info3->base.account_name.string, '/'); @@ -655,11 +660,13 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, ipactx, sep ? sep + 1 : info3->base.account_name.string); if (is_master) { - /* Well know RID of domain controllers group */ + /* Well known RID of domain controllers group */ info3->base.rid = 516; + info3->base.acct_flags |= ACB_SVRTRUST; } else { - /* Well know RID of domain computers group */ + /* Well known RID of domain computers group */ info3->base.rid = 515; + info3->base.acct_flags |= ACB_WSTRUST; } } else { ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, @@ -799,9 +806,13 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, /* always zero out, not used for Krb, only NTLM */ memset(&info3->base.LMSessKey, '\0', sizeof(info3->base.LMSessKey)); - /* TODO: fill based on objectclass, user vs computer, etc... */ - info3->base.acct_flags = ACB_NORMAL; /* samr_AcctFlags */ + /* If account type was not set before, default to ACB_NORMAL */ + if (!(info3->base.acct_flags & ~ACB_USE_AES_KEYS)) { + info3->base.acct_flags |= ACB_NORMAL; /* samr_AcctFlags */ + } + /* Clear ACB_USE_AES_KEYS as it is not used by Windows */ + info3->base.acct_flags &= ~ACB_USE_AES_KEYS; info3->base.sub_auth_status = 0; info3->base.last_successful_logon = 0; info3->base.last_failed_logon = 0; From cae0633a7488ea8ed1aea6a2d74b31b0e4ea2068 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Sat, 30 Oct 2021 10:49:37 +0300 Subject: [PATCH 9/9] SMB: switch IPA domain controller role As a part of CVE-2020-25717 mitigations, Samba now assumes 'CLASSIC PRIMARY DOMAIN CONTROLLER' server role does not support Kerberos operations. This is the role that IPA domain controller was using for its hybrid NT4/AD-like operation. Instead, 'IPA PRIMARY DOMAIN CONTROLLER' server role was introduced in Samba. Switch to this role for new installations and during the upgrade of servers running ADTRUST role. Fixes: https://pagure.io/freeipa/issue/9031 Signed-off-by: Alexander Bokovoy <aboko...@redhat.com> Reviewed-by: Rob Crittenden <rcrit...@redhat.com> --- install/share/smb.conf.registry.template | 2 +- ipaserver/install/adtrustinstance.py | 14 +++++++++++++- ipaserver/install/server/upgrade.py | 15 +++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/install/share/smb.conf.registry.template b/install/share/smb.conf.registry.template index c55a0c3079e..1d1d1216166 100644 --- a/install/share/smb.conf.registry.template +++ b/install/share/smb.conf.registry.template @@ -5,7 +5,7 @@ realm = $REALM kerberos method = dedicated keytab dedicated keytab file = /etc/samba/samba.keytab create krb5 conf = no -server role = CLASSIC PRIMARY DOMAIN CONTROLLER +server role = $SERVER_ROLE security = user domain master = yes domain logons = yes diff --git a/ipaserver/install/adtrustinstance.py b/ipaserver/install/adtrustinstance.py index 96a47ec0b29..bf0cc3b0976 100644 --- a/ipaserver/install/adtrustinstance.py +++ b/ipaserver/install/adtrustinstance.py @@ -146,6 +146,8 @@ class ADTRUSTInstance(service.Service): OBJC_GROUP = "ipaNTGroupAttrs" OBJC_DOMAIN = "ipaNTDomainAttrs" FALLBACK_GROUP_NAME = u'Default SMB Group' + SERVER_ROLE_OLD = "CLASSIC PRIMARY DOMAIN CONTROLLER" + SERVER_ROLE_NEW = "IPA PRIMARY DOMAIN CONTROLLER" def __init__(self, fstore=None, fulltrust=True): self.netbios_name = None @@ -573,7 +575,16 @@ def __write_smb_registry(self): with tempfile.NamedTemporaryFile(mode='w') as tmp_conf: tmp_conf.write(conf) tmp_conf.flush() - ipautil.run([paths.NET, "conf", "import", tmp_conf.name]) + try: + ipautil.run([paths.NET, "conf", "import", tmp_conf.name]) + except ipautil.CalledProcessError as e: + if e.returncode == 255: + # We have old Samba that doesn't support IPA DC server role + # re-try again with the older variant, upgrade code will + # take care to change the role later when Samba is upgraded + # as well. + self.sub_dict['SERVER_ROLE'] = self.SERVER_ROLE_OLD + self.__write_smb_registry() def __map_Guests_to_nobody(self): map_Guests_to_nobody() @@ -775,6 +786,7 @@ def __setup_sub_dict(self): LDAPI_SOCKET=self.ldapi_socket, FQDN=self.fqdn, SAMBA_DIR=paths.SAMBA_DIR, + SERVER_ROLE=self.SERVER_ROLE_NEW, ) def setup(self, fqdn, realm_name, netbios_name, diff --git a/ipaserver/install/server/upgrade.py b/ipaserver/install/server/upgrade.py index 50980d209bc..add8d8fb9a8 100644 --- a/ipaserver/install/server/upgrade.py +++ b/ipaserver/install/server/upgrade.py @@ -358,6 +358,21 @@ def upgrade_adtrust_config(): else: logger.warning("Error updating Samba registry: %s", e) + logger.info("[Change 'server role' from " + "'CLASSIC PRIMARY DOMAIN CONTROLLER' " + "to 'IPA PRIMARY DOMAIN CONTROLLER' in Samba configuration]") + + args = [paths.NET, "conf", "setparm", "global", + "server role", "IPA PRIMARY DOMAIN CONTROLLER"] + + try: + ipautil.run(args) + except ipautil.CalledProcessError as e: + # Only report an error if return code is not 255 + # which indicates that the new server role is not supported + # and we don't need to do anything + if e.returncode != 255: + logger.warning("Error updating Samba registry: %s", e) def ca_configure_profiles_acl(ca): logger.info('[Authorizing RA Agent to modify profiles]')
_______________________________________________ FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/freeipa-devel@lists.fedorahosted.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
