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