Skip to content

Commit

Permalink
Fix several memory leaks in LDAP KDB modules
Browse files Browse the repository at this point in the history 
Fix memory leaks discovered by running valgrind over kdbtest, and some
related leaks.  Many of them result from not calling ldap_msgfree
after an unsuccessful search (as the OpenLDAP documentation requires)
or after an exception following a search, so many of the fixes move or
add ldap_msgfree calls to cleanup labels.

ldap_osa_free_princ_ent was not used, and could not be used because it
frees the container while krb5_lookup_tl_kadm_data uses a
caller-allocated container.  Change it to leave the container alone,
but to correctly destroy xdrs.  Use it in krb5_ldap_put_principal
where princ_ent was leaked.

In krb5_ldap_put_principal, subtreelist is declared twice in interior
scopes and not properly freed; move it to function scope and free it
up in the cleanup label.  Also in krb5_ldap_put_principal, avoiding
decoding multiple KBR5_TL_KADM_DATA values (which we don't expect to
see) as later decodes would cause earlier decodes to leak.

In krb5_encode_krbsecretkey, fix a leak of the krb5_data container and
also add an error check when calling asn1_encode_sequence_of_keys;
otherwise we would dereference a null pointer if we run out of memory
encoding keys (very unlikely).

ticket: 7941 (new)
target_version: 1.12.2
tags: pullup
  • Loading branch information
@greghudson
greghudson committed Jul 12, 2014
1 parent aea099a commit bfd2a69
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 38 deletions.
2 changes: 1 addition & 1 deletion src/plugins/kdb/ldap/libkdb_ldap/ldap_misc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1017,7 +1017,7 @@ checkattributevalue(LDAP *ld, char *dn, char *attribute, char **attrvalues,
LDAP_NO_LIMIT,
&result)) != LDAP_SUCCESS) {
st = set_ldap_error(0, st, OP_SEARCH);
return st;
goto cleanup;
}

/*
Expand Down
3 changes: 3 additions & 0 deletions src/plugins/kdb/ldap/libkdb_ldap/ldap_principal.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ krb5_ldap_iterate(krb5_context context, char *match_expr,
}
} /* end of for (ent= ... */
ldap_msgfree(result);
result = NULL;
} /* end of for (tree= ... */

cleanup:
Expand All @@ -215,7 +216,9 @@ krb5_ldap_iterate(krb5_context context, char *match_expr,
for (;ntree; --ntree)
if (subtree[ntree-1])
free (subtree[ntree-1]);
free(subtree);

ldap_msgfree(result);
krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);
return st;
}
Expand Down
57 changes: 30 additions & 27 deletions src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,18 +441,18 @@ krb5_encode_krbsecretkey(krb5_key_data *key_data_in, 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;

Expand Down Expand Up @@ -502,9 +502,11 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
char **db_args)
{
int 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 *filtuser=NULL;
Expand All @@ -518,7 +520,7 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
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;
Expand Down Expand Up @@ -569,9 +571,9 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
goto cleanup;

if (entry->mask & KADM5_LOAD) {
unsigned int tree = 0, ntrees = 0;
unsigned int tree = 0;
int numlentries = 0;
char **subtreelist = NULL, *filter = NULL;
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
Expand All @@ -593,7 +595,6 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
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 {
Expand All @@ -603,7 +604,6 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
if (st == LDAP_SUCCESS) {
numlentries = ldap_count_entries(ld, result);
if (numlentries > 1) {
ldap_msgfree(result);
free(filter);
st = EINVAL;
k5_setmsg(context, st,
Expand All @@ -620,21 +620,20 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
if ((principal_dn = ldap_get_dn(ld, ent)) == NULL) {
ldap_get_option (ld, LDAP_OPT_RESULT_CODE, &st);
st = set_ldap_error (context, st, 0);
ldap_msgfree(result);
free(filter);
goto cleanup;
}
}
}
}
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.
Expand Down Expand Up @@ -709,9 +708,7 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
*/
if (xargs.dn_from_kbd == TRUE) {
/* make sure the DN falls in the subtree */
unsigned int tre=0, ntrees=0;
int dnlen=0, subtreelen=0;
char **subtreelist=NULL;
char *dn=NULL;
krb5_boolean outofsubtree=TRUE;

Expand All @@ -728,9 +725,12 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
dn = standalone_principal_dn;
}

/* get the current subtree list */
if ((st = krb5_get_subtree_info(ldap_context, &subtreelist, &ntrees)) != 0)
goto cleanup;
/* 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) {
Expand All @@ -746,10 +746,6 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
}
}

for (tre=0; tre < ntrees; ++tre) {
free(subtreelist[tre]);
}

if (outofsubtree == TRUE) {
st = EINVAL;
k5_setmsg(context, st, _("DN is out of the realm subtree"));
Expand All @@ -776,6 +772,8 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
*/
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);
Expand All @@ -789,7 +787,6 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
ldap_value_free(values);
}
}
ldap_msgfree(result);
} else {
st = set_ldap_error(context, st, OP_SEARCH);
goto cleanup;
Expand Down Expand Up @@ -997,10 +994,10 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
memset(&princ_ent, 0, sizeof(princ_ent));
for (tl_data=entry->tl_data; tl_data; tl_data=tl_data->tl_data_next) {
if (tl_data->tl_data_type == KRB5_TL_KADM_DATA) {
/* FIX ME: I guess the princ_ent should be freed after this call */
if ((st = krb5_lookup_tl_kadm_data(tl_data, &princ_ent)) != 0) {
goto cleanup;
}
break;
}
}

Expand Down Expand Up @@ -1282,6 +1279,10 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
if (polname != NULL)
free(polname);

for (tre = 0; tre < ntrees; tre++)
free(subtreelist[tre]);
free(subtreelist);

if (subtree)
free (subtree);

Expand All @@ -1298,6 +1299,8 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
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);
return(st);
}
Expand Down
9 changes: 6 additions & 3 deletions src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,15 +452,17 @@ krb5_ldap_iterate_password_policy(krb5_context context, char *match_expr,
goto cleanup;

(*func)(func_arg, entry);
/* XXX this will free policy so don't free it */
krb5_ldap_free_password_policy(context, entry);
entry = NULL;

free(policy);
policy = NULL;
}
ldap_msgfree(result);

cleanup:
free(entry);

free(policy);
ldap_msgfree(result);
krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);
return st;
}
Expand All @@ -472,6 +474,7 @@ krb5_ldap_free_password_policy (context, entry)
{
if (entry) {
free(entry->name);
free(entry->allowed_keysalts);
free(entry);
}
return;
Expand Down
11 changes: 8 additions & 3 deletions src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ krb5_ldap_list_realm(krb5_context context, char ***realms)
ldap_value_free(values);
}
} /* for (ent= ... */
ldap_msgfree(result);

cleanup:

Expand All @@ -187,6 +186,7 @@ krb5_ldap_list_realm(krb5_context context, char ***realms)

/* If there are no elements, still return a NULL terminated array */

ldap_msgfree(result);
krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);
return st;
}
Expand Down Expand Up @@ -283,7 +283,6 @@ krb5_ldap_delete_realm (krb5_context context, char *lrealm)
ldap_value_free(values);
}
}
ldap_msgfree(result);
}

/* Delete all password policies */
Expand Down Expand Up @@ -318,6 +317,12 @@ krb5_ldap_delete_realm (krb5_context context, char *lrealm)
free (subtrees);
}

if (result_arr != NULL) {
for (l = 0; l < ntree; l++)
ldap_msgfree(result_arr[l]);
free(result_arr);
}

if (policy != NULL) {
for (i = 0; policy[i] != NULL; i++)
free (policy[i]);
Expand Down Expand Up @@ -838,7 +843,6 @@ krb5_ldap_read_realm_params(krb5_context context, char *lrealm,
}

}
ldap_msgfree(result);

rlparams->mask = *mask;
*rlparamp = rlparams;
Expand All @@ -851,6 +855,7 @@ krb5_ldap_read_realm_params(krb5_context context, char *lrealm,
krb5_ldap_free_realm_params(rlparams);
*rlparamp=NULL;
}
ldap_msgfree(result);
krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);
return st;
}
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/kdb/ldap/libkdb_ldap/ldap_tkt_policy.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,6 @@ krb5_ldap_read_policy(krb5_context context, char *policyname,
if (krb5_ldap_get_value(ld, ent, "krbticketflags", (int *) &(lpolicy->tktflags)) == 0)
*omask |= LDAP_POLICY_TKTFLAGS;
}
ldap_msgfree(result);

lpolicy->mask = *omask;
store_tl_data(lpolicy->tl_data, KDB_TL_MASK, omask);
Expand All @@ -260,6 +259,7 @@ krb5_ldap_read_policy(krb5_context context, char *policyname,
krb5_ldap_free_policy(context, lpolicy);
*policy = NULL;
}
ldap_msgfree(result);
krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);
return st;
}
Expand Down Expand Up @@ -467,7 +467,6 @@ krb5_ldap_list(krb5_context context, char ***list, char *objectclass,
}
ldap_memfree(dn);
}
ldap_msgfree(result);

cleanup:
if (filter)
Expand All @@ -482,6 +481,7 @@ krb5_ldap_list(krb5_context context, char ***list, char *objectclass,
*list = NULL;
}
}
ldap_msgfree(result);
krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);
return st;
}
3 changes: 1 addition & 2 deletions src/plugins/kdb/ldap/libkdb_ldap/princ_xdr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,8 @@ ldap_osa_free_princ_ent(osa_princ_ent_t val)
XDR xdrs;

xdrmem_create(&xdrs, NULL, 0, XDR_FREE);

ldap_xdr_osa_princ_ent_rec(&xdrs, val);
free(val);
xdr_destroy(&xdrs);
}

krb5_error_code
Expand Down

0 comments on commit bfd2a69

Please sign in to comment.