From 86a8d9480aa402f885c72ccbcfeeb2bac488f268 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharwood@redhat.com>
Date: Wed, 31 Jul 2019 18:20:34 -0400
Subject: [PATCH 1/3] Make the coding style explicit
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>
---
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 <rharwood@redhat.com>
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 <rharwood@redhat.com>
Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>
---
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 <rharwood@redhat.com>
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 <rharwood@redhat.com>
Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>
---
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