From 5e4491e84555fbf32b50ec08e3a8027f9ab38e9c Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 15 Sep 2020 13:36:43 +0200 Subject: [PATCH] CVE-2020-25717 wb_sids2xids: inline wb_sids2xids_extract_for_domain_index() into wb_sids2xids_next_sids2unix() Instead of re-creating the dom_ids element, we just use a pre-allocated map_ids_in array. This is a bit tricky as we need to use map_ids_out as a copy of map_ids_in, because the _ids argument of dcerpc_wbint_Sids2UnixIDs_send() in [in,out], which means that _ids->ids is changed between dcerpc_wbint_Sids2UnixIDs_send() and dcerpc_wbint_Sids2UnixIDs_recv()! If the domain doesn't need any mappings, we'll move to the next domain early, for now this can't happend but it will in future. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14539 Signed-off-by: Stefan Metzmacher Reviewed-by: Gary Lockyer BUG: https://bugzilla.samba.org/show_bug.cgi?id=14556 (cherry picked from commit f6bb0ed21f82f2cf1f238f9f00cd049ecf8673af) --- source3/winbindd/wb_sids2xids.c | 115 +++++++++++++++++++++----------- 1 file changed, 75 insertions(+), 40 deletions(-) diff --git a/source3/winbindd/wb_sids2xids.c b/source3/winbindd/wb_sids2xids.c index b934425f4fd..aefb9f93ccb 100644 --- a/source3/winbindd/wb_sids2xids.c +++ b/source3/winbindd/wb_sids2xids.c @@ -39,6 +39,9 @@ struct wb_sids2xids_state { uint32_t lookup_count; struct dom_sid *lookup_sids; + struct wbint_TransIDArray map_ids_in; + struct wbint_TransIDArray map_ids_out; + /* * Domain array to use for the idmap call. The output from * lookupsids cannot be used directly since for migrated @@ -53,7 +56,6 @@ struct wb_sids2xids_state { struct lsa_RefDomainList idmap_doms; uint32_t dom_index; - struct wbint_TransIDArray *dom_ids; struct lsa_RefDomainList idmap_dom; bool tried_dclookup; @@ -107,6 +109,11 @@ struct tevent_req *wb_sids2xids_send(TALLOC_CTX *mem_ctx, return tevent_req_post(req, ev); } + state->map_ids_in.ids = talloc_zero_array(state, struct wbint_TransID, num_sids); + if (tevent_req_nomem(state->map_ids_in.ids, req)) { + return tevent_req_post(req, ev); + } + /* * Extract those sids that can not be resolved from cache * into a separate list to be handed to id mapping, keeping @@ -218,9 +225,6 @@ static bool wb_sids2xids_in_cache(struct dom_sid *sid, struct id_map *map) } static enum id_type lsa_SidType_to_id_type(const enum lsa_SidType sid_type); -static struct wbint_TransIDArray *wb_sids2xids_extract_for_domain_index( - TALLOC_CTX *mem_ctx, const struct wbint_TransIDArray *src, - uint32_t domain_index); static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq) { @@ -308,7 +312,11 @@ static void wb_sids2xids_next_sids2unix(struct tevent_req *req) req, struct wb_sids2xids_state); struct tevent_req *subreq = NULL; struct dcerpc_binding_handle *child_binding_handle = NULL; + const struct wbint_TransIDArray *src = NULL; + struct wbint_TransIDArray *dst = NULL; + uint32_t si; + next_domain: state->tried_dclookup = false; if (state->dom_index == state->idmap_doms.count) { @@ -316,10 +324,23 @@ static void wb_sids2xids_next_sids2unix(struct tevent_req *req) return; } - state->dom_ids = wb_sids2xids_extract_for_domain_index( - state, &state->ids, state->dom_index); - if (tevent_req_nomem(state->dom_ids, req)) { - return; + src = &state->ids; + dst = &state->map_ids_in; + dst->num_ids = 0; + + for (si=0; si < src->num_ids; si++) { + if (src->ids[si].domain_index != state->dom_index) { + continue; + } + + dst->ids[dst->num_ids] = src->ids[si]; + dst->ids[dst->num_ids].domain_index = 0; + dst->num_ids += 1; + } + + if (dst->num_ids == 0) { + state->dom_index += 1; + goto next_domain; } state->idmap_dom = (struct lsa_RefDomainList) { @@ -328,10 +349,23 @@ static void wb_sids2xids_next_sids2unix(struct tevent_req *req) .max_size = 1 }; + /* + * dcerpc_wbint_Sids2UnixIDs_send/recv will + * allocate a new array for the response + * and overwrite _ids->ids pointer. + * + * So we better make a temporary copy + * of state->map_ids_in (which contains the request array) + * into state->map_ids_out. + * + * That makes it possible to reuse the pre-allocated + * state->map_ids_in.ids array. + */ + state->map_ids_out = state->map_ids_in; child_binding_handle = idmap_child_handle(); subreq = dcerpc_wbint_Sids2UnixIDs_send( state, state->ev, child_binding_handle, &state->idmap_dom, - state->dom_ids); + &state->map_ids_out); if (tevent_req_nomem(subreq, req)) { return; } @@ -394,7 +428,7 @@ static void wb_sids2xids_done(struct tevent_req *subreq) return; } - src = state->dom_ids; + src = &state->map_ids_out; src_idx = 0; dst = &state->ids; @@ -405,11 +439,17 @@ static void wb_sids2xids_done(struct tevent_req *subreq) /* * All we can do here is to report "not mapped" */ + src = &state->map_ids_in; for (i=0; inum_ids; i++) { src->ids[i].xid.type = ID_TYPE_NOT_SPECIFIED; } } + if (src->num_ids != state->map_ids_in.num_ids) { + tevent_req_nterror(req, NT_STATUS_INTERNAL_ERROR); + return; + } + for (i=0; inum_ids; i++) { if (dst->ids[i].domain_index == state->dom_index) { dst->ids[i].xid = src->ids[src_idx].xid; @@ -417,7 +457,17 @@ static void wb_sids2xids_done(struct tevent_req *subreq) } } - TALLOC_FREE(state->dom_ids); + state->map_ids_in.num_ids = 0; + if (NT_STATUS_IS_OK(status)) { + /* + * If we got a valid response, we expect + * state->map_ids_out.ids to be a new allocated + * array, which we want to free early. + */ + SMB_ASSERT(state->map_ids_out.ids != state->map_ids_in.ids); + TALLOC_FREE(state->map_ids_out.ids); + } + state->map_ids_out = (struct wbint_TransIDArray) { .num_ids = 0, }; state->dom_index += 1; @@ -453,10 +503,23 @@ static void wb_sids2xids_gotdc(struct tevent_req *subreq) } } + /* + * dcerpc_wbint_Sids2UnixIDs_send/recv will + * allocate a new array for the response + * and overwrite _ids->ids pointer. + * + * So we better make a temporary copy + * of state->map_ids_in (which contains the request array) + * into state->map_ids_out. + * + * That makes it possible to reuse the pre-allocated + * state->map_ids_in.ids array. + */ + state->map_ids_out = state->map_ids_in; child_binding_handle = idmap_child_handle(); subreq = dcerpc_wbint_Sids2UnixIDs_send( state, state->ev, child_binding_handle, &state->idmap_dom, - state->dom_ids); + &state->map_ids_out); if (tevent_req_nomem(subreq, req)) { return; } @@ -504,31 +567,3 @@ NTSTATUS wb_sids2xids_recv(struct tevent_req *req, return NT_STATUS_OK; } - -static struct wbint_TransIDArray *wb_sids2xids_extract_for_domain_index( - TALLOC_CTX *mem_ctx, const struct wbint_TransIDArray *src, - uint32_t domain_index) -{ - struct wbint_TransIDArray *ret; - uint32_t i; - - ret = talloc_zero(mem_ctx, struct wbint_TransIDArray); - if (ret == NULL) { - return NULL; - } - ret->ids = talloc_array(ret, struct wbint_TransID, src->num_ids); - if (ret->ids == NULL) { - TALLOC_FREE(ret); - return NULL; - } - - for (i=0; inum_ids; i++) { - if (src->ids[i].domain_index == domain_index) { - ret->ids[ret->num_ids] = src->ids[i]; - ret->ids[ret->num_ids].domain_index = 0; - ret->num_ids += 1; - } - } - - return ret; -} -- 2.47.2