Skip to content

Commit

Permalink
Fix flaws in LDAP DN checking
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@greghudson
greghudson committed Feb 13, 2018
1 parent b1367ab commit e1caf6f
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 95 deletions.
7 changes: 7 additions & 0 deletions src/lib/kadm5/srv/svr_principal.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/kdb/ldap/libkdb_ldap/kdb_ldap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
200 changes: 106 additions & 94 deletions src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,107 @@ update_ldap_mod_auth_ind(krb5_context context, krb5_db_entry *entry,
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)
Expand All @@ -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;
Expand Down Expand Up @@ -860,106 +961,17 @@ 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);
if (st)
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) {
Expand Down
11 changes: 11 additions & 0 deletions src/tests/t_kdb.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,12 @@ def ldap_add(dn, objectclass, attrs=[]):
# in the test LDAP server.
realm.run([kadminl, 'ank', '-randkey', '-x', 'dn=cn=krb5', 'princ1'],
expected_code=1, expected_msg='DN is out of the realm subtree')
# 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'])
realm.run([kadminl, 'getprinc', 'princ1'], expected_msg='Principal: princ1')
realm.run([kadminl, 'ank', '-randkey', '-x', 'dn=cn=t2,cn=krb5', 'again'],
Expand All @@ -226,6 +232,11 @@ def ldap_add(dn, objectclass, attrs=[]):
'princ3'])
realm.run([kadminl, 'modprinc', '-x', 'containerdn=cn=t2,cn=krb5', 'princ3'],
expected_code=1, expected_msg='containerdn option not supported')
# 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',
Expand Down

0 comments on commit e1caf6f

Please sign in to comment.