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

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