From 997e1bbb2ec662357089aa43763e138183860cc3 Mon Sep 17 00:00:00 2001

From: Greg Hudson <ghudson@mit.edu>

Date: Fri, 12 Jan 2018 11:43:01 -0500

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

(cherry picked from commit e1caf6fb74981da62039846931ebdffed71309d1)

[rharwood@redhat.com fuzz - didn't port tests to expected_msg]

---

 src/lib/kadm5/srv/svr_principal.c             |   7 +

 src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h   |   2 +-

 .../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 0d4f0a632..64a4a2e97 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 06b477537..0c19804ad 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 88a170495..b7c9212cb 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

-            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 c0eeb0118..319687ff3 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.

+realm.run([kadminl, 'ank', '-randkey', '-x', 'dn=xcn=t1,cn=krb5', 'princ1'],

+          expected_code=1, expected_msg='DN is out of the realm subtree')

+

 realm.run([kadminl, 'ank', '-randkey', '-x', 'dn=cn=t2,cn=krb5', 'princ1'])

 out = realm.run([kadminl, 'getprinc', 'princ1'])

 if 'Principal: princ1' not in out:

@@ -209,6 +217,12 @@ out = realm.run([kadminl, 'modprinc', '-x', 'containerdn=cn=t2,cn=krb5',

 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).

+realm.run([kadminl, 'ank', '-randkey', '-x', 'containerdn=cn=krb5',

+           '-x', 'linkdn=cn=t2,cn=krb5', 'princ4'], expected_code=1,

+          expected_msg='DN is out of the realm subtree')

+

 # Create and modify a ticket policy.

 kldaputil(['create_policy', '-maxtktlife', '3hour', '-maxrenewlife', '6hour',

            '-allow_forwardable', 'tktpol'])