From 47d49aa7b795a31c559ce75e49940f5b5aac8d0f 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 --- 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 43ba955ac..6e1e3e351 100644 --- a/daemons/ipa-kdb/ipa_kdb.c +++ b/daemons/ipa-kdb/ipa_kdb.c @@ -57,6 +57,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 31f617129..81a8fd483 100644 --- a/daemons/ipa-kdb/ipa_kdb_mspac.c +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c @@ -418,7 +418,6 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, krb5_timestamp authtime, struct netr_SamInfo3 *info3) { - LDAP *lcontext = ipactx->lcontext; LDAPDerefRes *deref_results = NULL; struct dom_sid sid; gid_t prigid = -1; @@ -435,7 +434,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, bool is_idobject = false; krb5_principal princ; - 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++) { @@ -472,13 +471,14 @@ 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; } } else if (is_service) { - ret = ipadb_ldap_attr_to_str(lcontext, lentry, "krbCanonicalName", &strres); + ret = ipadb_ldap_attr_to_str(ipactx->lcontext, lentry, + "krbCanonicalName", &strres); if (ret) { /* krbCanonicalName is mandatory for services */ return ret; @@ -498,7 +498,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; @@ -511,7 +511,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; @@ -544,7 +545,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, info3->base.kickoff_time = INT64_MAX; #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: @@ -562,7 +563,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, info3->base.allow_password_change = info3->base.last_password_change; info3->base.force_password_change = INT64_MAX; - 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); @@ -575,7 +576,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: @@ -589,7 +590,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: @@ -603,7 +604,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: @@ -617,7 +618,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: @@ -648,7 +649,7 @@ static krb5_error_code ipadb_fill_info3(struct ipadb_context *ipactx, info3->base.rid = 515; } } 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 */ @@ -665,7 +666,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; @@ -2511,7 +2512,7 @@ static void ipadb_free_sid_blacklists(char ***sid_blocklist_incoming, char ***si 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 }; @@ -2545,6 +2546,7 @@ krb5_error_code 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 d1fa51578..cf1b4f53e 100644 --- a/daemons/ipa-kdb/ipa_kdb_principals.c +++ b/daemons/ipa-kdb/ipa_kdb_principals.c @@ -333,6 +333,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); @@ -607,8 +612,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); @@ -620,11 +633,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 */