From 985549be991c73c7455ed3b1393bd464ef4d197a Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy Date: Tue, 24 Jan 2017 11:02:30 +0200 Subject: [PATCH 1/1] ipa-kdb: support KDB DAL version 6.1 DAL version 6.0 removed support for a callback to free principal. This broke KDB drivers which had complex e_data structure within the principal structure. As result, FreeIPA KDB driver was leaking memory with DAL version 6.0 (krb5 1.15). DAL version 6.1 added a special callback for freeing e_data structure. See details at krb5/krb5#596 Restructure KDB driver code to provide this callback in case we are built against DAL version that supports it. For DAL version prior to 6.0 use this callback in the free_principal callback to tidy the code. Use explicit KDB version dependency in Fedora 26+ via BuildRequires. With new DAL version, freeipa package will fail to build and we'll have to add a support for new DAL version explicitly. https://fedorahosted.org/freeipa/ticket/6619 --- daemons/configure.ac | 21 ++++++++++++++++++ daemons/ipa-kdb/ipa_kdb.c | 42 ++++++++++++++++++++++++++++++++++-- daemons/ipa-kdb/ipa_kdb.h | 2 ++ daemons/ipa-kdb/ipa_kdb_principals.c | 42 ++++++++++++++++++++---------------- freeipa.spec.in | 9 ++++++++ 5 files changed, 96 insertions(+), 20 deletions(-) diff --git a/daemons/configure.ac b/daemons/configure.ac index 5c5a1046397aa97ba18cafc1b81dc2a6fb2dfd34..77a3be0397fac0364d364f2e4ac3f917cca7fff3 100644 --- a/daemons/configure.ac +++ b/daemons/configure.ac @@ -66,6 +66,27 @@ AC_SUBST(KRB5_LIBS) AC_SUBST(KRAD_LIBS) AC_SUBST(krb5rundir) +AC_CHECK_HEADER(kdb.h, [], [AC_MSG_ERROR([kdb.h not found])]) +AC_CHECK_MEMBER( + [kdb_vftabl.free_principal], + [AC_DEFINE([HAVE_KDB_FREEPRINCIPAL], [1], + [KDB driver API has free_principal callback])], + [AC_MSG_NOTICE([KDB driver API has no free_principal callback])], + [[#include ]]) +AC_CHECK_MEMBER( + [kdb_vftabl.free_principal_e_data], + [AC_DEFINE([HAVE_KDB_FREEPRINCIPAL_EDATA], [1], + [KDB driver API has free_principal_e_data callback])], + [AC_MSG_NOTICE([KDB driver API has no free_principal_e_data callback])], + [[#include ]]) + +if test "x$ac_cv_member_kdb_vftabl_free_principal" = "xno" \ + -a "x$ac_cv_member_kdb_vftable_free_principal_e_data" = "xno" ; then + AC_MSG_WARN([KDB driver API does not allow to free Kerberos principal data.]) + AC_MSG_WARN([KDB driver will leak memory on Kerberos principal use]) + AC_MSG_WARN([See https://github.com/krb5/krb5/pull/596 for details]) +fi + dnl --------------------------------------------------------------------------- dnl - Check for Mozilla LDAP and OpenLDAP SDK dnl --------------------------------------------------------------------------- diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c index e96353fe2602652dbf12f68745080b64665ffed2..e74ab56270500c2d3753d76754b8a5f1c28200a0 100644 --- a/daemons/ipa-kdb/ipa_kdb.c +++ b/daemons/ipa-kdb/ipa_kdb.c @@ -625,6 +625,9 @@ static void ipadb_free(krb5_context context, void *ptr) /* KDB Virtual Table */ +/* We explicitly want to keep different ABI tables below separate. */ +/* Do not merge them together. Older ABI does not need to be updated */ + #if KRB5_KDB_DAL_MAJOR_VERSION == 5 kdb_vftabl kdb_function_table = { .maj_ver = KRB5_KDB_DAL_MAJOR_VERSION, @@ -657,8 +660,9 @@ kdb_vftabl kdb_function_table = { .audit_as_req = ipadb_audit_as_req, .check_allowed_to_delegate = ipadb_check_allowed_to_delegate }; +#endif -#elif KRB5_KDB_DAL_MAJOR_VERSION == 6 +#if (KRB5_KDB_DAL_MAJOR_VERSION == 6) && !defined(HAVE_KDB_FREEPRINCIPAL_EDATA) kdb_vftabl kdb_function_table = { .maj_ver = KRB5_KDB_DAL_MAJOR_VERSION, .min_ver = 0, @@ -686,8 +690,42 @@ kdb_vftabl kdb_function_table = { .audit_as_req = ipadb_audit_as_req, .check_allowed_to_delegate = ipadb_check_allowed_to_delegate }; +#endif -#else +#if (KRB5_KDB_DAL_MAJOR_VERSION == 6) && defined(HAVE_KDB_FREEPRINCIPAL_EDATA) +kdb_vftabl kdb_function_table = { + .maj_ver = KRB5_KDB_DAL_MAJOR_VERSION, + .min_ver = 1, + .init_library = ipadb_init_library, + .fini_library = ipadb_fini_library, + .init_module = ipadb_init_module, + .fini_module = ipadb_fini_module, + .create = ipadb_create, + .get_age = ipadb_get_age, + .get_principal = ipadb_get_principal, + .put_principal = ipadb_put_principal, + .delete_principal = ipadb_delete_principal, + .iterate = ipadb_iterate, + .create_policy = ipadb_create_pwd_policy, + .get_policy = ipadb_get_pwd_policy, + .put_policy = ipadb_put_pwd_policy, + .iter_policy = ipadb_iterate_pwd_policy, + .delete_policy = ipadb_delete_pwd_policy, + .fetch_master_key = ipadb_fetch_master_key, + .store_master_key_list = ipadb_store_master_key_list, + .change_pwd = ipadb_change_pwd, + .sign_authdata = ipadb_sign_authdata, + .check_transited_realms = ipadb_check_transited_realms, + .check_policy_as = ipadb_check_policy_as, + .audit_as_req = ipadb_audit_as_req, + .check_allowed_to_delegate = ipadb_check_allowed_to_delegate, + /* The order is important, DAL version 6.1 added + * the free_principal_e_data callback */ + .free_principal_e_data = ipadb_free_principal_e_data, +}; +#endif + +#if (KRB5_KDB_DAL_MAJOR_VERSION != 5) && (KRB5_KDB_DAL_MAJOR_VERSION != 6) #error unsupported DAL major version #endif diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h index 1fdb409df92f1f8d9a82af3423e6e73313c62ab7..d5a343345562062b309d14c2e493f8d3028a6780 100644 --- a/daemons/ipa-kdb/ipa_kdb.h +++ b/daemons/ipa-kdb/ipa_kdb.h @@ -180,6 +180,8 @@ krb5_error_code ipadb_get_principal(krb5_context kcontext, unsigned int flags, krb5_db_entry **entry); void ipadb_free_principal(krb5_context kcontext, krb5_db_entry *entry); +/* Helper function for DAL API 6.1 or later */ +void ipadb_free_principal_e_data(krb5_context kcontext, krb5_octet *e_data); krb5_error_code ipadb_put_principal(krb5_context kcontext, krb5_db_entry *entry, char **db_args); diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c index 5b80909475565d6bb4fa8cba67629094daf51eb3..3bd8fb8c70c61b056a714bc0a8149bd8524beb1d 100644 --- a/daemons/ipa-kdb/ipa_kdb_principals.c +++ b/daemons/ipa-kdb/ipa_kdb_principals.c @@ -1274,11 +1274,32 @@ done: return kerr; } +void ipadb_free_principal_e_data(krb5_context kcontext, krb5_octet *e_data) +{ + struct ipadb_e_data *ied; + int i; + + ied = (struct ipadb_e_data *)e_data; + if (ied->magic == IPA_E_DATA_MAGIC) { + ldap_memfree(ied->entry_dn); + free(ied->passwd); + free(ied->pw_policy_dn); + for (i = 0; ied->pw_history && ied->pw_history[i]; i++) { + free(ied->pw_history[i]); + } + free(ied->pw_history); + for (i = 0; ied->authz_data && ied->authz_data[i]; i++) { + free(ied->authz_data[i]); + } + free(ied->authz_data); + free(ied->pol); + free(ied); + } +} + void ipadb_free_principal(krb5_context kcontext, krb5_db_entry *entry) { - struct ipadb_e_data *ied; krb5_tl_data *prev, *next; - int i; if (entry) { krb5_free_principal(kcontext, entry->princ); @@ -1292,22 +1313,7 @@ void ipadb_free_principal(krb5_context kcontext, krb5_db_entry *entry) ipa_krb5_free_key_data(entry->key_data, entry->n_key_data); if (entry->e_data) { - ied = (struct ipadb_e_data *)entry->e_data; - if (ied->magic == IPA_E_DATA_MAGIC) { - ldap_memfree(ied->entry_dn); - free(ied->passwd); - free(ied->pw_policy_dn); - for (i = 0; ied->pw_history && ied->pw_history[i]; i++) { - free(ied->pw_history[i]); - } - free(ied->pw_history); - for (i = 0; ied->authz_data && ied->authz_data[i]; i++) { - free(ied->authz_data[i]); - } - free(ied->authz_data); - free(ied->pol); - free(ied); - } + ipadb_free_principal_e_data(kcontext, entry->e_data); } free(entry); diff --git a/freeipa.spec.in b/freeipa.spec.in index 1dd8d0c60cacfc79554bb3c61fa8297e89b7b192..52ad0d4c1f1cec95821e17401e709f05ea9d97f6 100644 --- a/freeipa.spec.in +++ b/freeipa.spec.in @@ -57,7 +57,16 @@ BuildRequires: nspr-devel BuildRequires: nss-devel BuildRequires: openssl-devel BuildRequires: openldap-devel +# For KDB DAL version, make explicit dependency so that increase of version +# will cause the build to fail due to unsatisfied dependencies. +# DAL version change may cause code crash or memory leaks, it is better to fail early. +%if 0%{?fedora} > 25 +BuildRequires: krb5-devel >= 1.15-5 +BuildRequires: krb5-kdb-version = 6.1 +%else +# 1.12+: libkrad (http://krbdev.mit.edu/rt/Ticket/Display.html?id=7678) BuildRequires: krb5-devel >= 1.13 +%endif BuildRequires: krb5-workstation BuildRequires: libuuid-devel BuildRequires: libcurl-devel >= 7.21.7-2 -- 2.9.3