From 668491327203f67eaa303e9ab7d210f165672411 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy Date: Tue, 28 Jan 2020 10:33:22 +0200 Subject: [PATCH] Add pre-req patches for KDB fixes --- freeipa.spec | 3 +- krb5-kdb-fixes.patch | 272 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 krb5-kdb-fixes.patch diff --git a/freeipa.spec b/freeipa.spec index ef21965..b61390b 100644 --- a/freeipa.spec +++ b/freeipa.spec @@ -154,7 +154,8 @@ Source1: https://releases.pagure.org/freeipa/freeipa-%{version}.tar.gz.as # https://github.com/freeipa/freeipa/pull/4045 # Fix bugs in the overlapping DNS zone check Patch0: 4045.patch -Patch1: krb5-1.18-support.patch +Patch1: krb5-kdb-fixes.patch +Patch2: krb5-1.18-support.patch # For the timestamp trick in patch application BuildRequires: diffstat diff --git a/krb5-kdb-fixes.patch b/krb5-kdb-fixes.patch new file mode 100644 index 0000000..0990a95 --- /dev/null +++ b/krb5-kdb-fixes.patch @@ -0,0 +1,272 @@ +From 86a8d9480aa402f885c72ccbcfeeb2bac488f268 Mon Sep 17 00:00:00 2001 +From: Robbie Harwood +Date: Wed, 31 Jul 2019 18:20:34 -0400 +Subject: [PATCH 1/3] Make the coding style explicit + +Signed-off-by: Robbie Harwood +Reviewed-By: Alexander Bokovoy +--- + daemons/ipa-kdb/README | 18 ++++++++++++++++++ + 1 file changed, 18 insertions(+) + +diff --git a/daemons/ipa-kdb/README b/daemons/ipa-kdb/README +index b0786853b..4075082ee 100644 +--- a/daemons/ipa-kdb/README ++++ b/daemons/ipa-kdb/README +@@ -1 +1,19 @@ + This is the ipa krb5kdc database backend. ++ ++As the KDB interfaces heavily with krb5, we inherit its code style as well. ++However, note the following changes: ++ ++- no modelines (and different file preamble) ++- return types don't require their own line ++- single-statement blocks may optionally be braced ++- /* and */ do not ever get their own line ++- C99 for-loops are permitted (and encouraged) ++- a restricted set of other C99 features are permitted ++ ++In particular, variable-length arrays, flexible array members, compound ++literals, universal character names, and //-style comments are not permitted. ++ ++Use of regular malloc/free is preferred over talloc for new code. ++ ++By and large, existing code mostly conforms to these requirements. New code ++must conform to them. +-- +2.24.1 + + +From 01c1b270cd83ab6573dc0a502ac37d0182503c3d Mon Sep 17 00:00:00 2001 +From: Robbie Harwood +Date: Fri, 1 Nov 2019 16:48:55 -0400 +Subject: [PATCH 2/3] Use separate variable for client fetch in kdcpolicy + +`client` is not intended to be modified as a parameter of the AS check +function. Fixes an "incompatible pointer type" compiler warning. + +Signed-off-by: Robbie Harwood +Reviewed-By: Alexander Bokovoy +--- + daemons/ipa-kdb/ipa_kdb_kdcpolicy.c | 6 ++++-- + 1 file changed, 4 insertions(+), 2 deletions(-) + +diff --git a/daemons/ipa-kdb/ipa_kdb_kdcpolicy.c b/daemons/ipa-kdb/ipa_kdb_kdcpolicy.c +index 0b8aa668f..9467b1ba1 100644 +--- a/daemons/ipa-kdb/ipa_kdb_kdcpolicy.c ++++ b/daemons/ipa-kdb/ipa_kdb_kdcpolicy.c +@@ -23,6 +23,7 @@ ipa_kdcpolicy_check_as(krb5_context context, krb5_kdcpolicy_moddata moddata, + struct ipadb_e_data *ied; + struct ipadb_e_pol_limits *pol_limits = NULL; + int valid_auth_indicators = 0; ++ krb5_db_entry *client_actual = NULL; + + *status = NULL; + *lifetime_out = 0; +@@ -32,13 +33,14 @@ ipa_kdcpolicy_check_as(krb5_context context, krb5_kdcpolicy_moddata moddata, + if (ied == NULL || ied->magic != IPA_E_DATA_MAGIC) { + /* e-data is not availble, getting user auth from LDAP */ + krb5_klog_syslog(LOG_INFO, "IPA kdcpolicy: client e_data not availble. Try fetching..."); +- kerr = ipadb_get_principal(context, request->client, KRB5_KDB_FLAG_ALIAS_OK, &client); ++ kerr = ipadb_get_principal(context, request->client, ++ KRB5_KDB_FLAG_ALIAS_OK, &client_actual); + if (kerr != 0) { + krb5_klog_syslog(LOG_ERR, "IPA kdcpolicy: ipadb_find_principal failed."); + return kerr; + } + +- ied = (struct ipadb_e_data *)client->e_data; ++ ied = (struct ipadb_e_data *)client_actual->e_data; + if (ied == NULL && ied->magic != IPA_E_DATA_MAGIC) { + krb5_klog_syslog(LOG_ERR, "IPA kdcpolicy: client e_data fetching failed."); + return EINVAL; +-- +2.24.1 + + +From 6bdd6b3d265ffc2f437e2a69707978758c2efdd8 Mon Sep 17 00:00:00 2001 +From: Robbie Harwood +Date: Thu, 9 Jan 2020 16:11:28 -0500 +Subject: [PATCH 3/3] Fix several leaks in ipadb_find_principal + +`vals` is often leaked during early exit. Refactor function to use a +single exit path to prevent this. + +Signed-off-by: Robbie Harwood +Reviewed-By: Alexander Bokovoy +--- + daemons/ipa-kdb/ipa_kdb_principals.c | 132 +++++++++++++-------------- + 1 file changed, 64 insertions(+), 68 deletions(-) + +diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c +index 9e711cea5..47e44f090 100644 +--- a/daemons/ipa-kdb/ipa_kdb_principals.c ++++ b/daemons/ipa-kdb/ipa_kdb_principals.c +@@ -1035,100 +1035,96 @@ krb5_error_code ipadb_find_principal(krb5_context kcontext, + struct ipadb_context *ipactx; + bool found = false; + LDAPMessage *le = NULL; +- struct berval **vals; +- int i, result; ++ struct berval **vals = NULL; ++ int result; ++ krb5_error_code ret; + + ipactx = ipadb_get_context(kcontext); + if (!ipactx) { +- return KRB5_KDB_DBNOTINITED; ++ ret = KRB5_KDB_DBNOTINITED; ++ goto done; + } + +- while (!found) { +- +- if (!le) { +- le = ldap_first_entry(ipactx->lcontext, res); +- } else { +- le = ldap_next_entry(ipactx->lcontext, le); +- } +- if (!le) { +- break; +- } +- ++ for (le = ldap_first_entry(ipactx->lcontext, res); le != NULL; ++ le = ldap_next_entry(ipactx->lcontext, le)) { + vals = ldap_get_values_len(ipactx->lcontext, le, "krbprincipalname"); +- if (vals == NULL) { ++ if (vals == NULL) + continue; +- } + +- /* we need to check for a strict match as a '*' in the name may have +- * caused the ldap server to return multiple entries */ +- for (i = 0; vals[i]; i++) { +- /* KDC will accept aliases when doing TGT lookup (ref_tgt_again in do_tgs_req.c */ +- /* Use case-insensitive comparison in such cases */ +- if ((flags & KRB5_KDB_FLAG_ALIAS_OK) != 0) { +- if (ulc_casecmp(vals[i]->bv_val, vals[i]->bv_len, +- (*principal), strlen(*principal), +- NULL, NULL, &result) != 0) +- return KRB5_KDB_INTERNAL_ERROR; +- found = (result == 0); +- if (found) { +- /* replace the incoming principal with the value having +- * the correct case. This ensures that valid name/alias +- * is returned even if krbCanonicalName is not present +- */ +- free(*principal); +- *principal = strdup(vals[i]->bv_val); +- if (!(*principal)) { +- return KRB5_KDB_INTERNAL_ERROR; +- } +- } +- } else { +- found = (strcmp(vals[i]->bv_val, (*principal)) == 0); ++ /* We need to check for a strict match as a '*' in the name may have ++ * caused the ldap server to return multiple entries. */ ++ for (int i = 0; vals[i]; i++) { ++ if ((flags & KRB5_KDB_FLAG_ALIAS_OK) == 0) { ++ found = strcmp(vals[i]->bv_val, *principal) == 0; ++ if (found) ++ break; ++ ++ continue; + } +- if (found) { +- break; ++ ++ /* The KDC will accept aliases when doing TGT lookup ++ * (ref_tgt_again in do_tgs_req.c), so use case-insensitive ++ * comparison. */ ++ if (ulc_casecmp(vals[i]->bv_val, vals[i]->bv_len, *principal, ++ strlen(*principal), NULL, NULL, &result) != 0) { ++ ret = KRB5_KDB_INTERNAL_ERROR; ++ goto done; + } ++ if (result != 0) ++ continue; ++ ++ /* Fix case on the incoming principal to ensure that a valid ++ * name/alias is returned even if krbCanonicalName is not ++ * present. */ ++ free(*principal); ++ *principal = strdup(vals[i]->bv_val); ++ if (!*principal) { ++ ret = KRB5_KDB_INTERNAL_ERROR; ++ goto done; ++ } ++ found = true; ++ break; + } +- +- ldap_value_free_len(vals); +- +- if (!found) { ++ if (!found) + continue; +- } + +- /* we need to check if this is the canonical name */ ++ /* We need to check if this is the canonical name. */ ++ ldap_value_free_len(vals); + vals = ldap_get_values_len(ipactx->lcontext, le, "krbcanonicalname"); +- if (vals == NULL) { +- continue; +- } +- +- /* Again, if aliases are accepted by KDC, use case-insensitive comparison */ +- if ((flags & KRB5_KDB_FLAG_ALIAS_OK) != 0) { +- found = true; +- } else { +- found = (strcmp(vals[0]->bv_val, (*principal)) == 0); +- } ++ if (vals == NULL) ++ break; + +- if (!found) { +- /* search does not allow aliases */ +- ldap_value_free_len(vals); +- continue; ++ /* If aliases aren't accepted by the KDC, use case-sensitive ++ * comparison. */ ++ if ((flags & KRB5_KDB_FLAG_ALIAS_OK) == 0) { ++ found = strcmp(vals[0]->bv_val, *principal) == 0; ++ if (!found) { ++ ldap_value_free_len(vals); ++ continue; ++ } + } + + free(*principal); + *principal = strdup(vals[0]->bv_val); +- if (!(*principal)) { +- return KRB5_KDB_INTERNAL_ERROR; ++ if (!*principal) { ++ ret = KRB5_KDB_INTERNAL_ERROR; ++ goto done; + } +- +- ldap_value_free_len(vals); ++ break; + } + + if (!found || !le) { +- return KRB5_KDB_NOENTRY; ++ ret = KRB5_KDB_NOENTRY; ++ goto done; + } + ++ ret = 0; + *entry = le; +- return 0; ++done: ++ if (vals) ++ ldap_value_free_len(vals); ++ ++ return ret; + } + + static krb5_flags maybe_require_preauth(struct ipadb_context *ipactx, +-- +2.24.1 +