ipa/krb5-kdb-fixes.patch

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