From b6ef581001dff1c87786226fce3ecce7267f9894 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Feb 02 2012 19:23:16 +0000 Subject: Fixes a serious memory hierarchy bug causing unpredictable behavior in the LDAP provider. --- diff --git a/0002-DP-Fix-bugs-in-sss_dp_get_account_int.patch b/0002-DP-Fix-bugs-in-sss_dp_get_account_int.patch new file mode 100644 index 0000000..cd58c17 --- /dev/null +++ b/0002-DP-Fix-bugs-in-sss_dp_get_account_int.patch @@ -0,0 +1,265 @@ +From 707b20e80a5c5b86944dc55bbc652b392a4c6454 Mon Sep 17 00:00:00 2001 +From: Stephen Gallagher +Date: Sat, 21 Jan 2012 12:11:23 -0500 +Subject: [PATCH 5/5] DP: Fix bugs in sss_dp_get_account_int + +The conversion to the tevent_req style introduced numerous bugs +related to memory management of the various client requests. In +some circumstances, this could cause memory corruption and +segmentation faults in the NSS responder. This patch makes the +following changes: + +1) Rename the internal lookup from subreq to sidereq, to indicate +that it is not a sub-request of the current lookup (and therefore +is not cancelled if the current request is). + +2) Change the handling of the callback loops since they call +tevent_req_[done|error], which results in them being freed (and +therefore removed from the cb_list. This was the source of the +memory corruption that would occasionally result in dereferencing +an unreadable request. + +3) Remove the unnecessary sss_dp_get_account_int_recv() function +and change sss_dp_get_account_done() so that it only frees the +sidereq. All of the waiting processes have already been signaled +with the final results from sss_dp_get_account_int_done() +--- + src/responder/common/responder_dp.c | 110 +++++++++++----------------- + src/responder/nss/nsssrv_cmd.c | 1 + + src/responder/pam/pamsrv_cmd.c | 1 + + src/responder/sudo/sudosrv_get_sudorules.c | 1 + + 4 files changed, 47 insertions(+), 66 deletions(-) + +diff --git a/src/responder/common/responder_dp.c b/src/responder/common/responder_dp.c +index f51e2496a165cc2b776af776f2e9d1ea75b8e62c..9219037cc6055899e675eef846af54238d5c61e1 100644 +--- a/src/responder/common/responder_dp.c ++++ b/src/responder/common/responder_dp.c +@@ -92,11 +92,28 @@ static int sss_dp_req_destructor(void *ptr) + /* If there are callbacks that haven't been invoked, return + * an error now. + */ +- DLIST_FOR_EACH(cb, sdp_req->cb_list) { ++ while((cb = sdp_req->cb_list) != NULL) { + state = tevent_req_data(cb->req, struct dp_get_account_state); + state->err_maj = DP_ERR_FATAL; + state->err_min = EIO; ++ ++ /* tevent_req_done/error will free cb */ + tevent_req_error(cb->req, EIO); ++ ++ /* Freeing the cb removes it from the cb_list. ++ * Therefore, the cb_list should now be pointing ++ * at a new callback. If it's not, it means the ++ * callback handler didn't free cb and may leak ++ * memory. Be paranoid and protect against this ++ * situation. ++ */ ++ if (cb == sdp_req->cb_list) { ++ DEBUG(SSSDBG_FATAL_FAILURE, ++ ("BUG: a callback did not free its request. " ++ "May leak memory\n")); ++ /* Skip to the next since a memory leak is non-fatal */ ++ sdp_req->cb_list = sdp_req->cb_list->next; ++ } + } + + /* Destroy the hash entry */ +@@ -225,14 +242,6 @@ sss_dp_get_account_int_send(struct resp_ctx *rctx, + static void + sss_dp_get_account_done(struct tevent_req *subreq); + +-static errno_t +-sss_dp_get_account_int_recv(TALLOC_CTX *mem_ctx, +- struct tevent_req *req, +- dbus_uint16_t *err_maj, +- dbus_uint32_t *err_min, +- char **err_msg); +- +- + /* Send a request to the data provider + * Once this function is called, the communication + * with the data provider will always run to +@@ -252,7 +261,7 @@ sss_dp_get_account_send(TALLOC_CTX *mem_ctx, + errno_t ret; + int hret; + struct tevent_req *req; +- struct tevent_req *subreq; ++ struct tevent_req *sidereq; + struct dp_get_account_state *state; + struct sss_dp_req *sdp_req; + struct sss_dp_callback *cb; +@@ -343,19 +352,19 @@ sss_dp_get_account_send(TALLOC_CTX *mem_ctx, + */ + + value.type = HASH_VALUE_PTR; +- subreq = sss_dp_get_account_int_send(rctx, state->key, dom, ++ sidereq = sss_dp_get_account_int_send(rctx, state->key, dom, + be_type, filter); +- if (!subreq) { ++ if (!sidereq) { + ret = ENOMEM; + goto error; + } +- tevent_req_set_callback(subreq, sss_dp_get_account_done, NULL); ++ tevent_req_set_callback(sidereq, sss_dp_get_account_done, NULL); + + /* We should now be able to find the sdp_req in the hash table */ + hret = hash_lookup(rctx->dp_request_table, state->key, &value); + if (hret != HASH_SUCCESS) { + /* Something must have gone wrong with creating the request */ +- talloc_zfree(subreq); ++ talloc_zfree(sidereq); + ret = EIO; + goto error; + } +@@ -402,23 +411,10 @@ error: + } + + static void +-sss_dp_get_account_done(struct tevent_req *subreq) ++sss_dp_get_account_done(struct tevent_req *sidereq) + { +- errno_t ret; +- struct tevent_req *req = tevent_req_callback_data(subreq, +- struct tevent_req); +- struct dp_get_account_state *state = +- tevent_req_data(req, struct dp_get_account_state); +- +- ret = sss_dp_get_account_int_recv(state, req, +- &state->err_maj, +- &state->err_min, +- &state->err_msg); +- if (ret != EOK) { +- tevent_req_done(req); +- } else { +- tevent_req_error(req, ret); +- } ++ /* Nothing to do here. The callbacks have already been invoked */ ++ talloc_zfree(sidereq); + } + + errno_t +@@ -599,7 +595,7 @@ static void sss_dp_get_account_int_done(DBusPendingCall *pending, void *ptr) + int ret; + struct tevent_req *req; + struct sss_dp_req *sdp_req; +- struct sss_dp_callback *cb, *prevcb = NULL; ++ struct sss_dp_callback *cb; + struct dp_get_account_int_state *state; + struct dp_get_account_state *cb_state; + +@@ -630,58 +626,40 @@ static void sss_dp_get_account_int_done(DBusPendingCall *pending, void *ptr) + } + + /* Check whether we need to issue any callbacks */ +- DLIST_FOR_EACH(cb, sdp_req->cb_list) { ++ while ((cb = sdp_req->cb_list) != NULL) { + cb_state = tevent_req_data(cb->req, struct dp_get_account_state); + cb_state->err_maj = sdp_req->err_maj; + cb_state->err_min = sdp_req->err_min; + cb_state->err_msg = talloc_strdup(cb_state, sdp_req->err_msg); + /* Don't bother checking for NULL. If it fails due to ENOMEM, +- * we can't really handle it annyway. ++ * we can't really handle it anyway. + */ + ++ /* tevent_req_done/error will free cb */ + if (ret == EOK) { + tevent_req_done(cb->req); + } else { + tevent_req_error(cb->req, ret); + } + +- /* Freeing the request removes it from the list */ +- if (prevcb) talloc_free(prevcb); +- prevcb = cb; ++ /* Freeing the cb removes it from the cb_list. ++ * Therefore, the cb_list should now be pointing ++ * at a new callback. If it's not, it means the ++ * callback handler didn't free cb and may leak ++ * memory. Be paranoid and protect against this ++ * situation. ++ */ ++ if (cb == sdp_req->cb_list) { ++ DEBUG(SSSDBG_FATAL_FAILURE, ++ ("BUG: a callback did not free its request. " ++ "May leak memory\n")); ++ /* Skip to the next since a memory leak is non-fatal */ ++ sdp_req->cb_list = sdp_req->cb_list->next; ++ } + } +- talloc_free(prevcb); + + /* We're done with this request. Free the sdp_req + * This will clean up the hash table entry as well + */ + talloc_zfree(sdp_req); + } +- +-static errno_t +-sss_dp_get_account_int_recv(TALLOC_CTX *mem_ctx, +- struct tevent_req *req, +- dbus_uint16_t *err_maj, +- dbus_uint32_t *err_min, +- char **err_msg) +-{ +- struct dp_get_account_int_state *state = +- tevent_req_data(req, struct dp_get_account_int_state); +- +- enum tevent_req_state TRROEstate; +- uint64_t TRROEerr; +- +- *err_maj = state->sdp_req->err_maj; +- *err_min = state->sdp_req->err_min; +- *err_msg = talloc_steal(mem_ctx, state->sdp_req->err_msg); +- +- if (tevent_req_is_error(req, &TRROEstate, &TRROEerr)) { +- if (TRROEstate == TEVENT_REQ_USER_ERROR) { +- *err_maj = DP_ERR_FATAL; +- *err_min = TRROEerr; +- } else { +- return EIO; +- } +- } +- +- return EOK; +-} +diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c +index 3bc30ab8641b1787ded15165890e61836e46e802..fc2dca8d7a9e9dc1e5d68c98f95a5d3d67231f4a 100644 +--- a/src/responder/nss/nsssrv_cmd.c ++++ b/src/responder/nss/nsssrv_cmd.c +@@ -700,6 +700,7 @@ static void nsssrv_dp_send_acct_req_done(struct tevent_req *req) + ret = sss_dp_get_account_recv(cb_ctx->mem_ctx, req, + &err_maj, &err_min, + &err_msg); ++ talloc_zfree(req); + if (ret != EOK) { + NSS_CMD_FATAL_ERROR(cb_ctx->cctx); + } +diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c +index 3b2d509e237b12516e1234a34a8542ae09752c43..2e544cd5aa5a566e5557f2d9280b57b24f39befd 100644 +--- a/src/responder/pam/pamsrv_cmd.c ++++ b/src/responder/pam/pamsrv_cmd.c +@@ -994,6 +994,7 @@ static void pam_dp_send_acct_req_done(struct tevent_req *req) + ret = sss_dp_get_account_recv(cb_ctx->mem_ctx, req, + &err_maj, &err_min, + &err_msg); ++ talloc_zfree(req); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + ("Fatal error, killing connection!\n")); +diff --git a/src/responder/sudo/sudosrv_get_sudorules.c b/src/responder/sudo/sudosrv_get_sudorules.c +index 5d54f95ab78bc43338dd55205e85dbba7bd5f437..1723fd42c8222e72e46498a3b83e427099243369 100644 +--- a/src/responder/sudo/sudosrv_get_sudorules.c ++++ b/src/responder/sudo/sudosrv_get_sudorules.c +@@ -181,6 +181,7 @@ static void sudosrv_dp_send_acct_req_done(struct tevent_req *req) + ret = sss_dp_get_account_recv(cb_ctx->mem_ctx, req, + &err_maj, &err_min, + &err_msg); ++ talloc_zfree(req); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + ("Fatal error, killing connection!\n")); +-- +1.7.4.1 + diff --git a/sssd.spec b/sssd.spec index 22185b5..b34b9f9 100644 --- a/sssd.spec +++ b/sssd.spec @@ -19,7 +19,7 @@ Name: sssd Version: 1.7.0 -Release: 3%{?dist} +Release: 4%{?dist} Group: Applications/System Summary: System Security Services Daemon License: GPLv3+ @@ -30,6 +30,7 @@ BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) ### Patches ### Patch0001: 0001-LDAP-Do-not-fail-if-RootDSE-check-cannot-determine-s.patch +Patch0002: 0002-DP-Fix-bugs-in-sss_dp_get_account_int.patch ### Dependencies ### @@ -379,6 +380,10 @@ fi %postun -n libipa_hbac -p /sbin/ldconfig %changelog +* Wed Feb 01 2012 Stephen Gallagher - 1.7.0-4 +- Fixes a serious memory hierarchy bug causing unpredictable behavior in the + LDAP provider. + * Wed Feb 01 2012 Stephen Gallagher - 1.7.0-3 - Resolves: rhbz#773706 - SSSD fails during autodetection of search bases for new LDAP features