Skip Menu |
 

History Show all quoted text — Show full headers
# Fri Dec 18 19:00:57 2015 wfiveash (Will Fiveash) - Ticket created
Subject: potential memleak of pol_entry->name in populate_policy()
Download (untitled) / with headers
text/plain 482B
In populate_policy() src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c:

pol_entry->name = strdup(pol_name);

and later:

if (st)
goto cleanup;
/*
* We don't store the policy refcnt, because principals might be
maintained
* outside of kadmin. Instead, we will check for principal
references when
* policies are deleted.
*/
pol_entry->policy_refcnt = 0;

cleanup:
return st;

So if st is non-zero then pol_entry->name will be leaked.
# Fri Dec 18 19:18:44 2015 ghudson@mit.edu (Greg Hudson) - Status changed from 'new' to 'open'
# Fri Dec 18 19:18:44 2015 ghudson@mit.edu (Greg Hudson) - Correspondence added
Download (untitled) / with headers
text/plain 556B
It's a little more complicated than that. pol_entry->name isn't strictly
leaked since it's still accessible to the caller--although by current
practices, a function like that shouldn't leave behind a partly-populated
structure for the caller to clean up on error.

Of the two callers of that function,
krb5_ldap_get_password_policy_from_dn() cleans up the policy structure on
error, so can't leak memory if I'm reading it correctly.
krb5_ldap_iterate_password_policy() does not appear to clean up the
policy structure on error, so can leak memory.
# Wed Dec 23 13:57:40 2015 wfiveash (Will Fiveash) - Correspondence added
Download (untitled) / with headers
text/plain 1.1KiB
[ghudson - Fri Dec 18 19:18:44 2015]:

Show quoted text
> It's a little more complicated than that. pol_entry->name isn't strictly
> leaked since it's still accessible to the caller--although by current
> practices, a function like that shouldn't leave behind a partly-populated
> structure for the caller to clean up on error.
>
> Of the two callers of that function,
> krb5_ldap_get_password_policy_from_dn() cleans up the policy structure on
> error, so can't leak memory if I'm reading it correctly.
> krb5_ldap_iterate_password_policy() does not appear to clean up the
> policy structure on error, so can leak memory.

Looking at this more I agree. I think the patch should be:

diff -ur krb5-1.13.3/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c
krb5-1.13.3-ldap-fix/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c
--- krb5-1.13.3/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c
+++ krb5-1.13.3-ldap-fix/src/plugins/kdb/ldap/libkdb_ldap/ldap_pwd_policy.c
@@ -461,7 +461,8 @@
}

cleanup:
- free(entry);
+ if (st && entry)
+ krb5_ldap_free_password_policy(context, entry);
free(policy);
ldap_msgfree(result);
krb5_ldap_put_handle_to_pool(ldap_context, ldap_server_handle);