From 022c2a55ec9578867b331e419fad547092d94192 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy Date: Fri, 19 Feb 2021 15:37:47 +0200 Subject: [PATCH] ipa-kdb: do not use OpenLDAP functions with NULL LDAP context Calling to ipadb_get_connection() will remove LDAP context if any error happens. This means upper layers must always verify that LDAP context exists after such calls. ipadb_get_user_auth() may re-read global configuration and that may fail and cause IPA context to have NULL LDAP context. Fixes: https://pagure.io/freeipa/issue/8681 Signed-off-by: Alexander Bokovoy Reviewed-By: Robbie Harwood Reviewed-By: Rob Crittenden Reviewed-By: Florence Blanc-Renaud --- daemons/ipa-kdb/ipa_kdb.c | 1 + daemons/ipa-kdb/ipa_kdb_mspac.c | 32 +++++++++++++++------------- daemons/ipa-kdb/ipa_kdb_principals.c | 26 ++++++++++++++++------ 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c index 0dcc74263263423da6b1f4d8441ee149bce24c58..eb18a037e41bd778b3f522644acf8d793ddf70e7 100644 --- a/daemons/ipa-kdb/ipa_kdb.c +++ b/daemons/ipa-kdb/ipa_kdb.c @@ -56,6 +56,7 @@ static void ipadb_context_free(krb5_context kcontext, /* ldap free lcontext */ if ((*ctx)->lcontext) { ldap_unbind_ext_s((*ctx)->lcontext, NULL, NULL); + (*ctx)->lcontext = NULL; } free((*ctx)->supp_encs); free((*ctx)->def_encs); diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c index b6e7516859ce59232364f6ae93dee3063914ecf4..b231844250b8c65337bf4e7523d6a800dfe0767d 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c @@ -386,7 +386,6 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, TALLOC_CTX *memctx, struct netr_SamInfo3 *info3) { - LDAP *lcontext = ipactx->lcontext; LDAPDerefRes *deref_results = NULL; struct dom_sid sid; gid_t prigid = -1; @@ -403,7 +402,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, krb5_principal princ; krb5_data *data; - ret = ipadb_ldap_attr_to_strlist(lcontext, lentry, "objectClass", + ret = ipadb_ldap_attr_to_strlist(ipactx->lcontext, lentry, "objectClass", &objectclasses); if (ret == 0 && objectclasses != NULL) { for (c = 0; objectclasses[c] != NULL; c++) { @@ -427,7 +426,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, } if (is_host) { - ret = ipadb_ldap_attr_to_str(lcontext, lentry, "fqdn", &strres); + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, "fqdn", &strres); if (ret) { /* fqdn is mandatory for hosts */ return ret; @@ -440,7 +439,8 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, return ENOENT; } } else if (is_service) { - ret = ipadb_ldap_attr_to_str(lcontext, lentry, "krbPrincipalName", &strres); + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, + "krbCanonicalName", &strres); if (ret) { /* krbPrincipalName is mandatory for services */ return ret; @@ -489,7 +489,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, return ENOENT; } } else { - ret = ipadb_ldap_attr_to_str(lcontext, lentry, "uid", &strres); + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, "uid", &strres); if (ret) { /* uid is mandatory */ return ret; @@ -502,7 +502,8 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, if (is_host || is_service) { prigid = 515; /* Well known RID for domain computers group */ } else { - ret = ipadb_ldap_attr_to_int(lcontext, lentry, "gidNumber", &intres); + ret = ipadb_ldap_attr_to_int(ipactx->lcontext, lentry, + "gidNumber", &intres); if (ret) { /* gidNumber is mandatory */ return ret; @@ -533,7 +534,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, info3->base.kickoff_time = -1; #endif - ret = ipadb_ldap_attr_to_time_t(lcontext, lentry, + ret = ipadb_ldap_attr_to_time_t(ipactx->lcontext, lentry, "krbLastPwdChange", &timeres); switch (ret) { case 0: @@ -550,7 +551,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, info3->base.allow_password_change = 0; info3->base.force_password_change = -1; - ret = ipadb_ldap_attr_to_str(lcontext, lentry, "cn", &strres); + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, "cn", &strres); switch (ret) { case 0: info3->base.full_name.string = talloc_strdup(memctx, strres); @@ -563,7 +564,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, return ret; } - ret = ipadb_ldap_attr_to_str(lcontext, lentry, + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, "ipaNTLogonScript", &strres); switch (ret) { case 0: @@ -577,7 +578,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, return ret; } - ret = ipadb_ldap_attr_to_str(lcontext, lentry, + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, "ipaNTProfilePath", &strres); switch (ret) { case 0: @@ -591,7 +592,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, return ret; } - ret = ipadb_ldap_attr_to_str(lcontext, lentry, + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, "ipaNTHomeDirectory", &strres); switch (ret) { case 0: @@ -605,7 +606,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, return ret; } - ret = ipadb_ldap_attr_to_str(lcontext, lentry, + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, "ipaNTHomeDirectoryDrive", &strres); switch (ret) { case 0: @@ -626,7 +627,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, /* Well know RID of domain controllers group */ info3->base.rid = 516; } else { - ret = ipadb_ldap_attr_to_str(lcontext, lentry, + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, "ipaNTSecurityIdentifier", &strres); if (ret) { /* SID is mandatory */ @@ -643,7 +644,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, } } - ret = ipadb_ldap_deref_results(lcontext, lentry, &deref_results); + ret = ipadb_ldap_deref_results(ipactx->lcontext, lentry, &deref_results); switch (ret) { LDAPDerefRes *dres; LDAPDerefVal *dval; @@ -2433,7 +2434,7 @@ static krb5_error_code ipadb_mspac_get_trusted_domains(struct ipadb_context *ipactx) { struct ipadb_adtrusts *t; - LDAP *lc = ipactx->lcontext; + LDAP *lc = NULL; char *attrs[] = { "cn", "ipaNTTrustPartner", "ipaNTFlatName", "ipaNTTrustedDomainSID", "ipaNTSIDBlacklistIncoming", "ipaNTSIDBlacklistOutgoing", "ipaNTAdditionalSuffixes", NULL }; @@ -2467,6 +2468,7 @@ ipadb_mspac_get_trusted_domains(struct ipadb_context *ipactx) goto done; } + lc = ipactx->lcontext; for (le = ldap_first_entry(lc, res); le; le = ldap_next_entry(lc, le)) { dnstr = ldap_get_dn(lc, le); diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c index 3b78970b118cb6d8bd97da82a79226d20ea784e1..e1e86a6102886051cdafa3d885bf75c2718b8575 100644 --- a/daemons/ipa-kdb/ipa_kdb_principals.c +++ b/daemons/ipa-kdb/ipa_kdb_principals.c @@ -341,6 +341,11 @@ static enum ipadb_user_auth ipadb_get_user_auth(struct ipadb_context *ipactx, if (gcfg != NULL) gua = gcfg->user_auth; + /* lcontext == NULL means ipadb_get_global_config() failed to load + * global config and cleared the ipactx */ + if (ipactx->lcontext == NULL) + return IPADB_USER_AUTH_NONE; + /* Get the user's user_auth settings if not disabled. */ if ((gua & IPADB_USER_AUTH_DISABLED) == 0) ipadb_parse_user_auth(ipactx->lcontext, lentry, &ua); @@ -555,8 +560,16 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext, free(entry); return KRB5_KDB_DBNOTINITED; } - lcontext = ipactx->lcontext; - if (!lcontext) { + + entry->magic = KRB5_KDB_MAGIC_NUMBER; + entry->len = KRB5_KDB_V1_BASE_LENGTH; + + /* Get User Auth configuration. */ + ua = ipadb_get_user_auth(ipactx, lentry); + + /* ipadb_get_user_auth() calls into ipadb_get_global_config() + * and that might fail, causing lcontext to become NULL */ + if (!ipactx->lcontext) { krb5_klog_syslog(LOG_INFO, "No LDAP connection in ipadb_parse_ldap_entry(); retrying...\n"); ret = ipadb_get_connection(ipactx); @@ -568,11 +581,10 @@ static krb5_error_code ipadb_parse_ldap_entry(krb5_context kcontext, } } - entry->magic = KRB5_KDB_MAGIC_NUMBER; - entry->len = KRB5_KDB_V1_BASE_LENGTH; - - /* Get User Auth configuration. */ - ua = ipadb_get_user_auth(ipactx, lentry); + /* If any code below would result in invalidating ipactx->lcontext, + * lcontext must be updated with the new ipactx->lcontext value. + * We rely on the fact that none of LDAP-parsing helpers does it. */ + lcontext = ipactx->lcontext; /* ignore mask for now */ -- 2.26.3