krb5 commit: Fix several memory leaks in LDAP KDB modules
Greg Hudson
ghudson at MIT.EDU
Sat Jul 12 12:45:08 EDT 2014
https://github.com/krb5/krb5/commit/bfd2a69193ddc1b324d48a7da6455cfbda54fc09
commit bfd2a69193ddc1b324d48a7da6455cfbda54fc09
Author: Greg Hudson <ghudson at mit.edu>
Date: Tue Jun 10 23:53:31 2014 -0400
Fix several memory leaks in LDAP KDB modules
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
src/plugins/kdb/ldap/libkdb_ldap/ldap_misc.c | 2 +-
src/plugins/kdb/ldap/libkdb_ldap/ldap_principal.c | 3 +
src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c | 57 ++++++++++---------
src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c | 9 ++-
src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c | 11 +++-
src/plugins/kdb/ldap/libkdb_ldap/ldap_tkt_policy.c | 4 +-
src/plugins/kdb/ldap/libkdb_ldap/princ_xdr.c | 3 +-
7 files changed, 51 insertions(+), 38 deletions(-)
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_misc.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_misc.c
index 23be4a9..779121f 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_misc.c
+++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_misc.c
@@ -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;
}
/*
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal.c
index 81d5cba..21695a9 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal.c
+++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal.c
@@ -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:
@@ -215,7 +216,9 @@ cleanup:
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;
}
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
index 21ede0b..06881f0 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
+++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
@@ -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;
@@ -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;
@@ -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;
@@ -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
@@ -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 {
@@ -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,
@@ -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.
@@ -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;
@@ -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) {
@@ -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"));
@@ -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);
@@ -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;
@@ -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;
}
}
@@ -1282,6 +1279,10 @@ cleanup:
if (polname != NULL)
free(polname);
+ for (tre = 0; tre < ntrees; tre++)
+ free(subtreelist[tre]);
+ free(subtreelist);
+
if (subtree)
free (subtree);
@@ -1298,6 +1299,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);
return(st);
}
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c
index 4d7d673..522773e 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c
+++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c
@@ -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;
}
@@ -472,6 +474,7 @@ krb5_ldap_free_password_policy (context, entry)
{
if (entry) {
free(entry->name);
+ free(entry->allowed_keysalts);
free(entry);
}
return;
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c
index 086c458..7858a55 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c
+++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_realm.c
@@ -170,7 +170,6 @@ krb5_ldap_list_realm(krb5_context context, char ***realms)
ldap_value_free(values);
}
} /* for (ent= ... */
- ldap_msgfree(result);
cleanup:
@@ -187,6 +186,7 @@ cleanup:
/* 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;
}
@@ -283,7 +283,6 @@ krb5_ldap_delete_realm (krb5_context context, char *lrealm)
ldap_value_free(values);
}
}
- ldap_msgfree(result);
}
/* Delete all password policies */
@@ -318,6 +317,12 @@ cleanup:
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]);
@@ -838,7 +843,6 @@ krb5_ldap_read_realm_params(krb5_context context, char *lrealm,
}
}
- ldap_msgfree(result);
rlparams->mask = *mask;
*rlparamp = rlparams;
@@ -851,6 +855,7 @@ cleanup:
krb5_ldap_free_realm_params(rlparams);
*rlparamp=NULL;
}
+ ldap_msgfree(result);
krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);
return st;
}
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_tkt_policy.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_tkt_policy.c
index 5fe3164..7e93685 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_tkt_policy.c
+++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_tkt_policy.c
@@ -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);
@@ -260,6 +259,7 @@ cleanup:
krb5_ldap_free_policy(context, lpolicy);
*policy = NULL;
}
+ ldap_msgfree(result);
krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);
return st;
}
@@ -467,7 +467,6 @@ krb5_ldap_list(krb5_context context, char ***list, char *objectclass,
}
ldap_memfree(dn);
}
- ldap_msgfree(result);
cleanup:
if (filter)
@@ -482,6 +481,7 @@ cleanup:
*list = NULL;
}
}
+ ldap_msgfree(result);
krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);
return st;
}
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/princ_xdr.c b/src/plugins/kdb/ldap/libkdb_ldap/princ_xdr.c
index 5ae8672..5eca41e 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/princ_xdr.c
+++ b/src/plugins/kdb/ldap/libkdb_ldap/princ_xdr.c
@@ -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
More information about the cvs-krb5
mailing list