From 34acd361159df07c2433cfd1f59cd17cd8dc40a6 Mon Sep 17 00:00:00 2001
From: Greg Hudson <ghudson@mit.edu>
Date: Wed, 7 Mar 2018 15:00:56 +0100
Subject: [PATCH] Fix flaws in LDAP DN checking

KDB_TL_USER_INFO tl-data is intended to be internal to the LDAP KDB
module, and not used in disk or wire principal entries.  Prevent
kadmin clients from sending KDB_TL_USER_INFO tl-data by giving it a
type number less than 256 and filtering out type numbers less than 256
in kadm5_create_principal_3().  (We already filter out low type
numbers in kadm5_modify_principal()).

In the LDAP KDB module, if containerdn and linkdn are both specified
in a put_principal operation, check both linkdn and the computed
standalone_principal_dn for container membership.  To that end, factor
out the checks into helper functions and call them on all applicable
client-influenced DNs.

CVE-2018-5729:

In MIT krb5 1.6 or later, an authenticated kadmin user with permission
to add principals to an LDAP Kerberos database can cause a null
dereference in kadmind, or circumvent a DN container check, by
supplying tagged data intended to be internal to the database module.
Thanks to Sharwan Ram and Pooja Anil for discovering the potential
null dereference.

CVE-2018-5730:

In MIT krb5 1.6 or later, an authenticated kadmin user with permission
to add principals to an LDAP Kerberos database can circumvent a DN
containership check by supplying both a "linkdn" and "containerdn"
database argument, or by supplying a DN string which is a left
extension of a container DN string but is not hierarchically within
the container DN.

ticket: 8643 (new)
tags: pullup
target_version: 1.16-next
target_version: 1.15-next

This patch also contains part of bfd2a69193ddc1b324d48a7da6455cfbda54fc09:
Subject: [PATCH] Fix several memory leaks in LDAP KDB modules

diff --git a/src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h b/src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h
index 60caf3d..f6ed16b 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h
+++ b/src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h
@@ -151,7 +151,7 @@ extern void prepend_err_str (krb5_context ctx, const char *s, krb5_error_code er
 #define KRB5_CONF_ADMIN_BIND_DN "ldap_kadmind_dn"
 #define KRB5_CONF_PWD_BIND_DN "ldap_passwd_dn"
 
-#define  KDB_TL_USER_INFO      0x7ffe
+#define  KDB_TL_USER_INFO      0xff
 
 #define KDB_TL_PRINCTYPE          0x01
 #define KDB_TL_PRINCCOUNT         0x02
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
index bfa07c9..40ec87f 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
+++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
@@ -449,18 +449,18 @@ krb5_encode_krbsecretkey(krb5_key_data *key_data, int n_key_data,
     for (i = 0, last = 0, j = 0, currkvno = key_data[0].key_data_kvno; i < n_key_data; i++) {
         krb5_data *code;
         if (i == n_key_data - 1 || key_data[i + 1].key_data_kvno != currkvno) {
-            asn1_encode_sequence_of_keys (key_data+last,
-                                          (krb5_int16) i - last + 1,
-                                          mkvno,
-                                          &code);
-            ret[j] = malloc (sizeof (struct berval));
-            if (ret[j] == NULL) {
-                err = ENOMEM;
+            ret[j] = k5alloc(sizeof(struct berval), &err);
+            if (ret[j] == NULL)
+                goto cleanup;
+            err = asn1_encode_sequence_of_keys(key_data + last,
+                                               (krb5_int16)i - last + 1,
+                                               mkvno, &code);
+           if (err) 
                 goto cleanup;
-            }
             /*CHECK_NULL(ret[j]); */
             ret[j]->bv_len = code->length;
             ret[j]->bv_val = code->data;
+            free(code);
             j++;
             last = i + 1;
 
@@ -505,27 +505,130 @@ tl_data2berval (krb5_tl_data *in, struct berval **out)
     return 0;
 }
 
+static krb5_error_code
+check_dn_in_container(krb5_context context, const char *dn,
+                      char *const *subtrees, unsigned int ntrees)
+{
+    unsigned int i;
+    size_t dnlen = strlen(dn), stlen;
+
+    for (i = 0; i < ntrees; i++) {
+        if (subtrees[i] == NULL || *subtrees[i] == '\0')
+            return 0;
+        stlen = strlen(subtrees[i]);
+        if (dnlen >= stlen &&
+            strcasecmp(dn + dnlen - stlen, subtrees[i]) == 0 &&
+            (dnlen == stlen || dn[dnlen - stlen - 1] == ','))
+            return 0;
+    }
+
+    krb5_set_error_message(context, EINVAL, _("DN is out of the realm subtree"));
+    return EINVAL;
+}
+
+static krb5_error_code
+check_dn_exists(krb5_context context,
+                krb5_ldap_server_handle *ldap_server_handle,
+                const char *dn, krb5_boolean nonkrb_only)
+{
+    krb5_error_code st = 0, tempst;
+    krb5_ldap_context *ldap_context = context->dal_handle->db_context;
+    LDAP *ld = ldap_server_handle->ldap_handle;
+    LDAPMessage *result = NULL, *ent;
+    char *attrs[] = { "krbticketpolicyreference", "krbprincipalname", NULL };
+    char **values;
+
+    LDAP_SEARCH_1(dn, LDAP_SCOPE_BASE, 0, attrs, IGNORE_STATUS);
+    if (st != LDAP_SUCCESS)
+        return set_ldap_error(context, st, OP_SEARCH);
+
+    ent = ldap_first_entry(ld, result);
+    CHECK_NULL(ent);
+
+    values = ldap_get_values(ld, ent, "krbticketpolicyreference");
+    if (values != NULL)
+        ldap_value_free(values);
+
+    values = ldap_get_values(ld, ent, "krbprincipalname");
+    if (values != NULL) {
+        ldap_value_free(values);
+        if (nonkrb_only) {
+            st = EINVAL;
+            krb5_set_error_message(context, st, _("ldap object is already kerberized"));
+            goto cleanup;
+        }
+    }
+
+cleanup:
+    ldap_msgfree(result);
+    return st;
+}
+
+static krb5_error_code
+validate_xargs(krb5_context context,
+               krb5_ldap_server_handle *ldap_server_handle,
+               const xargs_t *xargs, const char *standalone_dn,
+               char *const *subtrees, unsigned int ntrees)
+{
+    krb5_error_code st;
+
+    if (xargs->dn != NULL) {
+        /* The supplied dn must be within a realm container. */
+        st = check_dn_in_container(context, xargs->dn, subtrees, ntrees);
+        if (st)
+            return st;
+        /* The supplied dn must exist without Kerberos attributes. */
+        st = check_dn_exists(context, ldap_server_handle, xargs->dn, TRUE);
+        if (st)
+            return st;
+    }
+
+    if (xargs->linkdn != NULL) {
+        /* The supplied linkdn must be within a realm container. */
+        st = check_dn_in_container(context, xargs->linkdn, subtrees, ntrees);
+        if (st)
+            return st;
+        /* The supplied linkdn must exist. */
+        st = check_dn_exists(context, ldap_server_handle, xargs->linkdn,
+                             FALSE);
+        if (st)
+            return st;
+    }
+
+    if (xargs->containerdn != NULL && standalone_dn != NULL) {
+        /* standalone_dn (likely composed using containerdn) must be within a
+         * container. */
+        st = check_dn_in_container(context, standalone_dn, subtrees, ntrees);
+        if (st)
+            return st;
+    }
+
+    return 0;
+}
+
 krb5_error_code
 krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entries,
                         int *nentries, char **db_args)
 {
     int                         i=0, l=0, kerberos_principal_object_type=0;
+    unsigned int                ntrees=0, tre=0;
     krb5_error_code             st=0, tempst=0;
     LDAP                        *ld=NULL;
     LDAPMessage                 *result=NULL, *ent=NULL;
+    char                        **subtreelist = NULL;
     char                        *user=NULL, *subtree=NULL, *principal_dn=NULL;
-    char                        **values=NULL, *strval[10]={NULL}, errbuf[1024];
+    char                        *strval[10]={NULL}, errbuf[1024];
     struct berval               **bersecretkey=NULL;
     LDAPMod                     **mods=NULL;
     krb5_boolean                create_standalone_prinicipal=FALSE;
-    krb5_boolean                krb_identity_exists=FALSE, establish_links=FALSE;
+    krb5_boolean                establish_links=FALSE;
     char                        *standalone_principal_dn=NULL;
     krb5_tl_data                *tl_data=NULL;
     krb5_key_data               **keys=NULL;
     kdb5_dal_handle             *dal_handle=NULL;
     krb5_ldap_context           *ldap_context=NULL;
     krb5_ldap_server_handle     *ldap_server_handle=NULL;
-    osa_princ_ent_rec           princ_ent;
+    osa_princ_ent_rec           princ_ent = {0};
     xargs_t                     xargs = {0};
     char                        *polname = NULL;
     OPERATION optype;
@@ -573,8 +676,8 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entries,
             goto cleanup;
 
         if (entries->mask & KADM5_LOAD) {
-            int              tree = 0, ntrees = 0, princlen = 0, numlentries = 0;
-            char             **subtreelist = NULL, *filter = NULL;
+            int              tree = 0, princlen = 0, numlentries = 0;
+            char             *filter = NULL;
 
             /*  A load operation is special, will do a mix-in (add krbprinc
              *  attrs to a non-krb object entry) if an object exists with a
@@ -603,7 +706,6 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entries,
             found_entry = FALSE;
             /* search for entry with matching krbprincipalname attribute */
             for (tree = 0; found_entry == FALSE && tree < ntrees; ++tree) {
-                result = NULL;
                 if (principal_dn == NULL) {
                     LDAP_SEARCH_1(subtreelist[tree], ldap_context->lrparams->search_scope, filter, principal_attributes, IGNORE_STATUS);
                 } else {
@@ -613,7 +715,6 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entries,
                 if (st == LDAP_SUCCESS) {
                     numlentries = ldap_count_entries(ld, result);
                     if (numlentries > 1) {
-                        ldap_msgfree(result);
                         free(filter);
                         st = EINVAL;
                         krb5_set_error_message(context, st,
@@ -636,14 +737,14 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entries,
                             }
                         }
                     }
-                    if (result)
-                        ldap_msgfree(result);
                 } else if (st != LDAP_NO_SUCH_OBJECT) {
                     /* could not perform search, return with failure */
                     st = set_ldap_error (context, st, 0);
                     free(filter);
                     goto cleanup;
                 }
+                ldap_msgfree(result);
+                result = NULL;
                 /*
                  * If it isn't found then assume a standalone princ entry is to
                  * be created.
@@ -717,106 +818,19 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entries,
          * any of the subtrees
          */
         if (xargs.dn_from_kbd == TRUE) {
-            /* make sure the DN falls in the subtree */
-            int              tre=0, dnlen=0, subtreelen=0, ntrees=0;
-            char             **subtreelist=NULL;
-            char             *dn=NULL;
-            krb5_boolean     outofsubtree=TRUE;
-
-            if (xargs.dn != NULL) {
-                dn = xargs.dn;
-            } else if (xargs.linkdn != NULL) {
-                dn = xargs.linkdn;
-            } else if (standalone_principal_dn != NULL) {
-                /*
-                 * Even though the standalone_principal_dn is constructed
-                 * within this function, there is the containerdn input
-                 * from the user that can become part of the it.
-                 */
-                dn = standalone_principal_dn;
-            }
 
             /* get the current subtree list */
-            if ((st = krb5_get_subtree_info(ldap_context, &subtreelist, &ntrees)) != 0)
-                goto cleanup;
-
-            for (tre=0; tre<ntrees; ++tre) {
-                if (subtreelist[tre] == NULL || strlen(subtreelist[tre]) == 0) {
-                    outofsubtree = FALSE;
-                    break;
-                } else {
-                    dnlen = strlen (dn);
-                    subtreelen = strlen(subtreelist[tre]);
-                    if ((dnlen >= subtreelen) && (strcasecmp((dn + dnlen - subtreelen), subtreelist[tre]) == 0)) {
-                        outofsubtree = FALSE;
-                        break;
-                    }
-                }
-            }
+            if (subtreelist == NULL) {
+                st = krb5_get_subtree_info(ldap_context, &subtreelist, &ntrees);
+                if (st)
+                    goto cleanup;
 
-            for (tre=0; tre < ntrees; ++tre) {
-                free(subtreelist[tre]);
             }
+            st = validate_xargs(context, ldap_server_handle, &xargs,
+                                standalone_principal_dn, subtreelist, ntrees);
 
-            if (outofsubtree == TRUE) {
-                st = EINVAL;
-                krb5_set_error_message(context, st, "DN is out of the realm subtree");
+            if (st)
                 goto cleanup;
-            }
-
-            /*
-             * dn value will be set either by dn, linkdn or the standalone_principal_dn
-             * In the first 2 cases, the dn should be existing and in the last case we
-             * are supposed to create the ldap object. so the below should not be
-             * executed for the last case.
-             */
-
-            if (standalone_principal_dn == NULL) {
-                /*
-                 * If the ldap object is missing, this results in an error.
-                 */
-
-                /*
-                 * Search for krbprincipalname attribute here.
-                 * This is to find if a kerberos identity is already present
-                 * on the ldap object, in which case adding a kerberos identity
-                 * on the ldap object should result in an error.
-                 */
-                char  *attributes[]={"krbticketpolicyreference", "krbprincipalname", NULL};
-
-                LDAP_SEARCH_1(dn, LDAP_SCOPE_BASE, 0, attributes, IGNORE_STATUS);
-                if (st == LDAP_SUCCESS) {
-                    ent = ldap_first_entry(ld, result);
-                    if (ent != NULL) {
-                        if ((values=ldap_get_values(ld, ent, "krbticketpolicyreference")) != NULL) {
-                            ldap_value_free(values);
-                        }
-
-                        if ((values=ldap_get_values(ld, ent, "krbprincipalname")) != NULL) {
-                            krb_identity_exists = TRUE;
-                            ldap_value_free(values);
-                        }
-                    }
-                    ldap_msgfree(result);
-                } else {
-                    st = set_ldap_error(context, st, OP_SEARCH);
-                    goto cleanup;
-                }
-            }
-        }
-
-        /*
-         * If xargs.dn is set then the request is to add a
-         * kerberos principal on a ldap object, but if
-         * there is one already on the ldap object this
-         * should result in an error.
-         */
-
-        if (xargs.dn != NULL && krb_identity_exists == TRUE) {
-            st = EINVAL;
-            snprintf(errbuf, sizeof(errbuf), "ldap object is already kerberized");
-            krb5_set_error_message(context, st, "%s", errbuf);
-            goto cleanup;
         }
 
         if (xargs.linkdn != NULL) {
@@ -1264,6 +1278,10 @@ cleanup:
     if (polname != NULL)
         free(polname);
 
+    for (tre = 0; tre < ntrees; tre++)
+        free(subtreelist[tre]);
+    free(subtreelist);
+
     if (subtree)
         free (subtree);
 
@@ -1280,6 +1298,8 @@ cleanup:
         free (keys);
 
     ldap_mods_free(mods, 1);
+    ldap_osa_free_princ_ent(&princ_ent);
+    ldap_msgfree(result);
     krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);
     *nentries = i;
     return(st);
-- 
2.16.2

