krb5 commit [krb5-1.15]: Fix flaws in LDAP DN checking

https://github.com/krb5/krb5/commit/a2df94b6bb22ecf6e77c5044cff0f627b4fb30cf
commit a2df94b6bb22ecf6e77c5044cff0f627b4fb30cf
Author: Greg Hudson <[email protected]>
Date:   Fri Jan 12 11:43:01 2018 -0500

    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.
    
    (cherry picked from commit e1caf6fb74981da62039846931ebdffed71309d1)
    
    ticket: 8643
    version_fixed: 1.15.3

 src/lib/kadm5/srv/svr_principal.c                  |    7 +
 src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h        |    2 +-
 src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c |  200 +++++++++++---------
 src/tests/t_kdb.py                                 |   14 ++
 4 files changed, 128 insertions(+), 95 deletions(-)

diff --git a/src/lib/kadm5/srv/svr_principal.c b/src/lib/kadm5/srv/svr_principal.c
index 8f4da0e..a35c249 100644
--- a/src/lib/kadm5/srv/svr_principal.c
+++ b/src/lib/kadm5/srv/svr_principal.c
@@ -330,6 +330,13 @@ kadm5_create_principal_3(void *server_handle,
         return KADM5_BAD_MASK;
     if((mask & ~ALL_PRINC_MASK))
         return KADM5_BAD_MASK;
+    if (mask & KADM5_TL_DATA) {
+        for (tl_data_tail = entry->tl_data; tl_data_tail != NULL;
+             tl_data_tail = tl_data_tail->tl_data_next) {
+            if (tl_data_tail->tl_data_type < 256)
+                return KADM5_BAD_TL_TYPE;
+        }
+    }
 
     /*
      * Check to see if the principal exists
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h b/src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h
index 06b4775..0c19804 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h
+++ b/src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h
@@ -141,7 +141,7 @@ extern int set_ldap_error (krb5_context ctx, int st, int op);
 #define UNSTORE16_INT(ptr, val) (val = load_16_be(ptr))
 #define UNSTORE32_INT(ptr, val) (val = load_32_be(ptr))
 
-#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 7ba53f9..0f32cc1 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
+++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
@@ -651,6 +651,107 @@ cleanup:
     return ret;
 }
 
+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;
+    }
+
+    k5_setmsg(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;
+            k5_setmsg(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 *entry,
                         char **db_args)
@@ -662,12 +763,12 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
     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];
     char                        *filtuser=NULL;
     struct berval               **bersecretkey=NULL;
     LDAPMod                     **mods=NULL;
     krb5_boolean                create_standalone=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;
@@ -860,24 +961,6 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
      * any of the subtrees
      */
     if (xargs.dn_from_kbd == TRUE) {
-        /* make sure the DN falls in the subtree */
-        int              dnlen=0, subtreelen=0;
-        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 we haven't already done so. */
         if (subtreelist == NULL) {
             st = krb5_get_subtree_info(ldap_context, &subtreelist, &ntrees);
@@ -885,81 +968,10 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
                 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 (outofsubtree == TRUE) {
-            st = EINVAL;
-            k5_setmsg(context, st, _("DN is out of the realm subtree"));
+        st = validate_xargs(context, ldap_server_handle, &xargs,
+                            standalone_principal_dn, subtreelist, ntrees);
+        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_msgfree(result);
-            result = 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);
-                    }
-                }
-            } 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"));
-        k5_setmsg(context, st, "%s", errbuf);
-        goto cleanup;
     }
 
     if (xargs.linkdn != NULL) {
diff --git a/src/tests/t_kdb.py b/src/tests/t_kdb.py
index 185225a..001905f 100755
--- a/src/tests/t_kdb.py
+++ b/src/tests/t_kdb.py
@@ -171,6 +171,14 @@ out = realm.run([kadminl, 'ank', '-randkey', '-x', 'dn=cn=krb5', 'princ1'],
                 expected_code=1)
 if 'DN is out of the realm subtree' not in out:
     fail('Unexpected kadmin.local output for out-of-realm dn')
+# Check that the DN container check is a hierarchy test, not a simple
+# suffix match (CVE-2018-5730).  We expect this operation to fail
+# either way (because "xcn" isn't a valid DN tag) but the container
+# check should happen before the DN is parsed.
+out = realm.run([kadminl, 'ank', '-randkey', '-x', 'dn=xcn=t1,cn=krb5',
+                 'princ1'], expected_code=1)
+if 'DN is out of the realm subtree' not in out:
+    fail('Unexpected kadmin.local output for non-hierarchy dn prefix')
 realm.run([kadminl, 'ank', '-randkey', '-x', 'dn=cn=t2,cn=krb5', 'princ1'])
 out = realm.run([kadminl, 'getprinc', 'princ1'])
 if 'Principal: princ1' not in out:
@@ -208,6 +216,12 @@ out = realm.run([kadminl, 'modprinc', '-x', 'containerdn=cn=t2,cn=krb5',
                  'princ3'], expected_code=1)
 if 'containerdn option not supported' not in out:
     fail('Unexpected kadmin.local output trying to reset containerdn')
+# Verify that containerdn is checked when linkdn is also supplied
+# (CVE-2018-5730).
+out = realm.run([kadminl, 'ank', '-randkey', '-x', 'containerdn=cn=krb5',
+                 '-x', 'linkdn=cn=t2,cn=krb5', 'princ4'], expected_code=1)
+if 'DN is out of the realm subtree' not in out:
+    fail('Unexpected kadmin.local output for linkdn+containerdn test')
 
 # Create and modify a ticket policy.
 kldaputil(['create_policy', '-maxtktlife', '3hour', '-maxrenewlife', '6hour',