From: Nick Porter Date: Wed, 13 Oct 2021 19:24:41 +0000 (+0100) Subject: v4: Protect LDAP connections from being freed when there are outstanding queries... X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c895a0e28fbfda7cda5aa955d717cb95326fcb00;p=thirdparty%2Ffreeradius-server.git v4: Protect LDAP connections from being freed when there are outstanding queries... (#4267) * Use correct DN string in debug message * Use query type to identify LDAP searches * Protect fr_ldap_connection_t from being freed if there are pending queries * Add watcher checking for outstanding queries when an LDAP connection closes * Alter _ldap_query_free() to free the associated connection if it is closed ... ... and has no other queries associated with it * Queries are removed from the pending tree by the query destructor * Improve handling of un-expected results retrieved from libldap * Don't free LDAP results just because an LDAP error was returned ... ... if the caller wants the results, it may want to query them regardless of success or failure. * Add debug message for auth bind reject * Free the bind_auth_ctx when we've finished with it * Improved version of _ldap_bind_auth_io_read() - handle fr_ldap_result() returning no result - handle LDAP_PROC_REJECT - use a normal while loop --- diff --git a/src/lib/ldap/base.c b/src/lib/ldap/base.c index a280848ec17..aec9c1cfa4e 100644 --- a/src/lib/ldap/base.c +++ b/src/lib/ldap/base.c @@ -450,7 +450,7 @@ fr_ldap_rcode_t fr_ldap_result(LDAPMessage **result, LDAPControl ***ctrls, if (status != LDAP_PROC_SUCCESS) break; } - if (*result_p && ((status < 0) || !result)) { + if (*result_p && (!result)) { ldap_msgfree(*result_p); *result_p = NULL; } @@ -1036,11 +1036,18 @@ int fr_ldap_global_config(int debug_level, char const *tls_random_file) /** Free any libldap structures when an fr_ldap_query_t is freed * + * It is also possible that the connection used for this query is now closed, + * in that instance we free it here. */ static int _ldap_query_free(fr_ldap_query_t *query) { int i; + /* + * Remove the query from the tree of outstanding queries + */ + if (query->ldap_conn) fr_rb_remove(query->ldap_conn->queries, query); + /* * Free any results which were retrieved */ @@ -1071,6 +1078,15 @@ static int _ldap_query_free(fr_ldap_query_t *query) fr_dlist_talloc_free(&query->referrals); + /* + * If the connection this query was using has no pending queries and + * is no-longer associated with a fr_connection_t then free it + */ + if ((query->ldap_conn) && (query->ldap_conn->conn == NULL) && + (fr_rb_num_elements(query->ldap_conn->queries) == 0)) { + talloc_free(query->ldap_conn); + } + return 0; } diff --git a/src/lib/ldap/bind.c b/src/lib/ldap/bind.c index 58ea321f07e..7fa7f9d8d3f 100644 --- a/src/lib/ldap/bind.c +++ b/src/lib/ldap/bind.c @@ -286,6 +286,7 @@ static unlang_action_t ldap_async_auth_bind_results(rlm_rcode_t *p_result, UNUSE RETURN_MODULE_DISALLOW; case LDAP_PROC_REJECT: + RDEBUG2("Bind as user \"%s\" rejected", bind_ctx->bind_dn); RETURN_MODULE_REJECT; case LDAP_PROC_BAD_DN: @@ -298,6 +299,8 @@ static unlang_action_t ldap_async_auth_bind_results(rlm_rcode_t *p_result, UNUSE RETURN_MODULE_FAIL; } + + talloc_free(bind_auth_ctx); } /** Signal an outstanding LDAP bind request to cancel @@ -311,6 +314,8 @@ static void ldap_async_auth_bind_cancel(UNUSED request_t *request, fr_state_sign ldap_abandon_ext(bind_auth_ctx->bind_ctx->c->handle, bind_auth_ctx->msgid, NULL, NULL); fr_rb_remove(bind_auth_ctx->thread->binds, bind_auth_ctx); + + talloc_free(bind_auth_ctx); } /** Initiate an async LDAP bind for authentication diff --git a/src/lib/ldap/connection.c b/src/lib/ldap/connection.c index b53382e4597..cb88bd3ac07 100644 --- a/src/lib/ldap/connection.c +++ b/src/lib/ldap/connection.c @@ -239,6 +239,11 @@ static void _ldap_connection_close(UNUSED fr_event_list_t *el, void *h, UNUSED v */ static int _ldap_connection_free(fr_ldap_connection_t *c) { + /* + * If there are any pending queries, don't free + */ + if ((c->queries) && (fr_rb_num_elements(c->queries) > 0)) return -1; + talloc_free_children(c); /* Force inverted free order */ fr_ldap_control_clear(c); @@ -290,6 +295,23 @@ fr_ldap_connection_t *fr_ldap_connection_alloc(TALLOC_CTX *ctx) return c; } +/** Watcher for LDAP connections being closed + * + * If there are any outstanding queries on the connection then + * re-parent the connection to the NULL ctx so that it remains + * until all the queries have been dealt with. + */ +static void _ldap_connection_close_watch(fr_connection_t *conn, UNUSED fr_connection_state_t prev, + UNUSED fr_connection_state_t state, void *uctx) +{ + fr_ldap_connection_t *ldap_conn = talloc_get_type_abort(uctx, fr_ldap_connection_t); + + if (fr_rb_num_elements(ldap_conn->queries) == 0) return; + + talloc_reparent(conn, NULL, ldap_conn); + ldap_conn->conn = NULL; +} + /** (Re-)Initialises the libldap side of the connection handle * * The first ldap state transition is either: @@ -343,6 +365,8 @@ static fr_connection_state_t _ldap_connection_init(void **h, fr_connection_t *co c = fr_ldap_connection_alloc(conn); c->conn = conn; + fr_connection_add_watch_pre(conn, FR_CONNECTION_STATE_CLOSED, _ldap_connection_close_watch, false, c); + /* * Configure/allocate the libldap handle */ @@ -485,7 +509,6 @@ static void ldap_request_cancel_mux(fr_trunk_connection_t *tconn, fr_connection_ while ((fr_trunk_connection_pop_cancellation(&treq, tconn)) == 0) { query = treq->preq; ldap_abandon_ext(ldap_conn->handle, query->msgid, NULL, NULL); - fr_rb_remove(ldap_conn->queries, query); fr_trunk_request_signal_cancel_complete(treq); @@ -791,6 +814,18 @@ static void ldap_trunk_request_demux(UNUSED fr_trunk_connection_t *tconn, fr_con if (!query) { WARN("Ignoring msgid %i - doesn't match any outstanding queries (it may have been cancelled)", find.msgid); + ldap_msgfree(result); + continue; + } + + /* + * This really shouldn't happen - as we only retrieve complete sets of results - + * but as the query data structure will last until its results are fully handled + * better to have this safety check here. + */ + if (query->ret != LDAP_RESULT_PENDING) { + WARN("Received results for msgid %i which has already been handled - ignoring", find.msgid); + ldap_msgfree(result); continue; } @@ -807,7 +842,8 @@ static void ldap_trunk_request_demux(UNUSED fr_trunk_connection_t *tconn, fr_con switch (rcode) { case LDAP_PROC_SUCCESS: - query->ret = ((!query->mods) && (ldap_count_entries(ldap_conn->handle, result) == 0)) ? + query->ret = ((query->type == LDAP_REQUEST_SEARCH) && + (ldap_count_entries(ldap_conn->handle, result) == 0)) ? LDAP_RESULT_NO_RESULT : LDAP_RESULT_SUCCESS; break; @@ -852,7 +888,7 @@ static void ldap_trunk_request_demux(UNUSED fr_trunk_connection_t *tconn, fr_con break; case LDAP_PROC_BAD_DN: - ROPTIONAL(RDEBUG2, DEBUG2, "DN %s does not exist", query->ldap_url->lud_dn); + ROPTIONAL(RDEBUG2, DEBUG2, "DN %s does not exist", query->dn); query->ret = LDAP_RESULT_BAD_DN; break; @@ -886,9 +922,8 @@ static void ldap_trunk_request_demux(UNUSED fr_trunk_connection_t *tconn, fr_con if (query->parser) query->parser(query, result); /* - * Remove the query from the outstanding list and tidy up + * Mark the trunk request as complete and set the request as runnable */ - fr_rb_remove(ldap_conn->queries, query); fr_trunk_request_signal_complete(query->treq); if (query->request) unlang_interpret_mark_runnable(query->request); diff --git a/src/modules/rlm_ldap/rlm_ldap.c b/src/modules/rlm_ldap/rlm_ldap.c index 0871940ce6a..f8c7d641d12 100644 --- a/src/modules/rlm_ldap/rlm_ldap.c +++ b/src/modules/rlm_ldap/rlm_ldap.c @@ -616,45 +616,61 @@ static void _ldap_bind_auth_io_read(UNUSED fr_event_list_t *el, UNUSED int fd, U int ret; -next_result: - /* - * Fetch the next LDAP result which has been fully received - */ - ret = fr_ldap_result(&result, NULL, ldap_conn, LDAP_RES_ANY, LDAP_MSG_ALL, NULL, fr_time_delta_wrap(10)); + do { + /* + * Fetch the next LDAP result which has been fully received + */ + ret = fr_ldap_result(&result, NULL, ldap_conn, LDAP_RES_ANY, LDAP_MSG_ALL, NULL, fr_time_delta_wrap(10)); - /* - * Timeout in this case really means no results to read - we've - * handled everything that was available - */ - if (ret == LDAP_PROC_TIMEOUT) return; + /* + * Timeout in this case really means no results to read - we've + * handled everything that was available + */ + if (ret == LDAP_PROC_TIMEOUT) return; - find.msgid = ldap_msgid(result); - bind_auth_ctx = fr_rb_find(thread->binds, &find); + /* + * If there is no result don't try to process one + */ + if (!result) return; - if (!bind_auth_ctx) { - WARN("Ignoring bind result msgid %i - doesn't match any outstanidng binds", find.msgid); - goto next_result; - } + find.msgid = ldap_msgid(result); + bind_auth_ctx = fr_rb_find(thread->binds, &find); - /* - * Remove from the list of pending bind requests - */ - fr_rb_remove(bind_auth_ctx->thread->binds, bind_auth_ctx); + if (!bind_auth_ctx) { + WARN("Ignoring bind result msgid %i - doesn't match any outstanidng binds", find.msgid); + ldap_msgfree(result); + continue; + } + + /* + * Remove from the list of pending bind requests + */ + fr_rb_remove(bind_auth_ctx->thread->binds, bind_auth_ctx); - bind_auth_ctx->ret = ret; + bind_auth_ctx->ret = ret; - switch (ret) { - case LDAP_PROC_SUCCESS: - case LDAP_PROC_NOT_PERMITTED: - break; + switch (ret) { + /* + * Accept or reject will be SUCCESS, NOT_PERMITTED or REJECT + */ + case LDAP_PROC_SUCCESS: + case LDAP_PROC_NOT_PERMITTED: + case LDAP_PROC_REJECT: + break; - default: - fr_ldap_state_error(bind_auth_ctx->bind_ctx->c); /* Restart the connection state machine */ - break; - } - unlang_interpret_mark_runnable(bind_auth_ctx->request); + default: + ERROR("LDAP connection returned an error - restarting the connection"); + fr_ldap_state_error(bind_auth_ctx->bind_ctx->c); /* Restart the connection state machine */ + break; + } + unlang_interpret_mark_runnable(bind_auth_ctx->request); + + /* + * Clear up the libldap results + */ + ldap_msgfree(result); - goto next_result; + } while (1); } /** Watch callback to add fd read callback to LDAP connection