From: Andrew Bartlett Date: Thu, 30 Sep 2021 21:47:29 +0000 (+1300) Subject: CVE-2020-25718 s4-rpc_server: Change sid list functions to operate on a array of... X-Git-Tag: ldb-2.5.0~152 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=4796b0a5c1d3948642d17eef9f72d364f0e29de3;p=thirdparty%2Fsamba.git CVE-2020-25718 s4-rpc_server: Change sid list functions to operate on a array of struct dom_sid This is instead of an array of struct dom_sid *. The reason is that auth_user_info_dc has an array of struct dom_sid (the user token) and for checking if an RODC should be allowed to print a particular ticket, we want to reuse that a rather then reconstruct it via tokenGroups. This also avoids a lot of memory allocation. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14558 Signed-off-by: Andrew Bartlett Reviewed-by: Joseph Sutton --- diff --git a/source4/rpc_server/common/sid_helper.c b/source4/rpc_server/common/sid_helper.c index 698249391ef..65d7e7c7271 100644 --- a/source4/rpc_server/common/sid_helper.c +++ b/source4/rpc_server/common/sid_helper.c @@ -29,13 +29,16 @@ /* see if any SIDs in list1 are in list2 */ -bool sid_list_match(const struct dom_sid **list1, const struct dom_sid **list2) +bool sid_list_match(uint32_t num_sids1, + const struct dom_sid *list1, + uint32_t num_sids2, + const struct dom_sid *list2) { unsigned int i, j; /* do we ever have enough SIDs here to worry about O(n^2) ? */ - for (i=0; list1[i]; i++) { - for (j=0; list2[j]; j++) { - if (dom_sid_equal(list1[i], list2[j])) { + for (i=0; i < num_sids1; i++) { + for (j=0; j < num_sids2; j++) { + if (dom_sid_equal(&list1[i], &list2[j])) { return true; } } @@ -51,9 +54,10 @@ WERROR samdb_result_sid_array_ndr(struct ldb_context *sam_ctx, struct ldb_message *msg, TALLOC_CTX *mem_ctx, const char *attr, - const struct dom_sid ***sids, - const struct dom_sid **additional_sids, - unsigned int num_additional) + uint32_t *num_sids, + struct dom_sid **sids, + const struct dom_sid *additional_sids, + unsigned int num_additional) { struct ldb_message_element *el; unsigned int i, j; @@ -65,30 +69,25 @@ WERROR samdb_result_sid_array_ndr(struct ldb_context *sam_ctx, } /* Make array long enough for NULL and additional SID */ - (*sids) = talloc_array(mem_ctx, const struct dom_sid *, - el->num_values + num_additional + 1); + (*sids) = talloc_array(mem_ctx, struct dom_sid, + el->num_values + num_additional); W_ERROR_HAVE_NO_MEMORY(*sids); for (i=0; inum_values; i++) { enum ndr_err_code ndr_err; - struct dom_sid *sid; - sid = talloc(*sids, struct dom_sid); - W_ERROR_HAVE_NO_MEMORY(sid); - - ndr_err = ndr_pull_struct_blob(&el->values[i], sid, sid, + ndr_err = ndr_pull_struct_blob_all_noalloc(&el->values[i], &(*sids)[i], (ndr_pull_flags_fn_t)ndr_pull_dom_sid); if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { return WERR_INTERNAL_DB_CORRUPTION; } - (*sids)[i] = sid; } for (j = 0; j < num_additional; j++) { (*sids)[i++] = additional_sids[j]; } - (*sids)[i] = NULL; + *num_sids = i; return WERR_OK; } @@ -101,7 +100,8 @@ WERROR samdb_result_sid_array_dn(struct ldb_context *sam_ctx, struct ldb_message *msg, TALLOC_CTX *mem_ctx, const char *attr, - const struct dom_sid ***sids) + uint32_t *num_sids, + struct dom_sid **sids) { struct ldb_message_element *el; unsigned int i; @@ -112,23 +112,21 @@ WERROR samdb_result_sid_array_dn(struct ldb_context *sam_ctx, return WERR_OK; } - (*sids) = talloc_array(mem_ctx, const struct dom_sid *, el->num_values + 1); + (*sids) = talloc_array(mem_ctx, struct dom_sid, el->num_values + 1); W_ERROR_HAVE_NO_MEMORY(*sids); for (i=0; inum_values; i++) { struct ldb_dn *dn = ldb_dn_from_ldb_val(mem_ctx, sam_ctx, &el->values[i]); NTSTATUS status; - struct dom_sid *sid; + struct dom_sid sid = { 0, }; - sid = talloc(*sids, struct dom_sid); - W_ERROR_HAVE_NO_MEMORY(sid); - status = dsdb_get_extended_dn_sid(dn, sid, "SID"); + status = dsdb_get_extended_dn_sid(dn, &sid, "SID"); if (!NT_STATUS_IS_OK(status)) { return WERR_INTERNAL_DB_CORRUPTION; } (*sids)[i] = sid; } - (*sids)[i] = NULL; + *num_sids = i; return WERR_OK; } diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c index e458b2a9931..c7d2addd104 100644 --- a/source4/rpc_server/drsuapi/getncchanges.c +++ b/source4/rpc_server/drsuapi/getncchanges.c @@ -1171,10 +1171,10 @@ static WERROR getncchanges_repl_secret(struct drsuapi_bind_state *b_state, const char *rodc_attrs[] = { "msDS-KrbTgtLink", "msDS-NeverRevealGroup", "msDS-RevealOnDemandGroup", "objectGUID", NULL }; const char *obj_attrs[] = { "tokenGroups", "objectSid", "UserAccountControl", "msDS-KrbTgtLinkBL", NULL }; struct ldb_result *rodc_res = NULL, *obj_res = NULL; - const struct dom_sid **never_reveal_sids, **reveal_sids, **token_sids; + uint32_t num_never_reveal_sids, num_reveal_sids, num_token_sids; + struct dom_sid *never_reveal_sids, *reveal_sids, *token_sids; const struct dom_sid *object_sid = NULL; WERROR werr; - const struct dom_sid *additional_sids[] = { NULL, NULL }; DEBUG(3,(__location__ ": DRSUAPI_EXOP_REPL_SECRET extended op on %s\n", drs_ObjectIdentifier_to_string(mem_ctx, ncRoot))); @@ -1259,12 +1259,13 @@ static WERROR getncchanges_repl_secret(struct drsuapi_bind_state *b_state, /* if the object SID is equal to the user_sid, allow */ object_sid = samdb_result_dom_sid(mem_ctx, obj_res->msgs[0], "objectSid"); + if (object_sid == NULL) { + goto failed; + } if (dom_sid_equal(user_sid, object_sid)) { goto allowed; } - additional_sids[0] = object_sid; - /* * Must be an RODC account at this point, verify machine DN matches the * SID account @@ -1294,13 +1295,17 @@ static WERROR getncchanges_repl_secret(struct drsuapi_bind_state *b_state, } werr = samdb_result_sid_array_dn(b_state->sam_ctx_system, rodc_res->msgs[0], - mem_ctx, "msDS-NeverRevealGroup", &never_reveal_sids); + mem_ctx, "msDS-NeverRevealGroup", + &num_never_reveal_sids, + &never_reveal_sids); if (!W_ERROR_IS_OK(werr)) { goto denied; } werr = samdb_result_sid_array_dn(b_state->sam_ctx_system, rodc_res->msgs[0], - mem_ctx, "msDS-RevealOnDemandGroup", &reveal_sids); + mem_ctx, "msDS-RevealOnDemandGroup", + &num_reveal_sids, + &reveal_sids); if (!W_ERROR_IS_OK(werr)) { goto denied; } @@ -1311,19 +1316,27 @@ static WERROR getncchanges_repl_secret(struct drsuapi_bind_state *b_state, * TODO determine if sIDHistory is required for this check */ werr = samdb_result_sid_array_ndr(b_state->sam_ctx_system, obj_res->msgs[0], - mem_ctx, "tokenGroups", &token_sids, - additional_sids, 1); + mem_ctx, "tokenGroups", + &num_token_sids, + &token_sids, + object_sid, 1); if (!W_ERROR_IS_OK(werr) || token_sids==NULL) { goto denied; } if (never_reveal_sids && - sid_list_match(token_sids, never_reveal_sids)) { + sid_list_match(num_token_sids, + token_sids, + num_never_reveal_sids, + never_reveal_sids)) { goto denied; } if (reveal_sids && - sid_list_match(token_sids, reveal_sids)) { + sid_list_match(num_token_sids, + token_sids, + num_reveal_sids, + reveal_sids)) { goto allowed; } diff --git a/source4/rpc_server/netlogon/dcerpc_netlogon.c b/source4/rpc_server/netlogon/dcerpc_netlogon.c index 1f60f9cc79c..b135d114716 100644 --- a/source4/rpc_server/netlogon/dcerpc_netlogon.c +++ b/source4/rpc_server/netlogon/dcerpc_netlogon.c @@ -2856,10 +2856,10 @@ static bool sam_rodc_access_check(struct ldb_context *sam_ctx, struct ldb_dn *rodc_dn; int ret; struct ldb_result *rodc_res = NULL, *obj_res = NULL; - const struct dom_sid *additional_sids[] = { NULL, NULL }; WERROR werr; struct dom_sid *object_sid; - const struct dom_sid **never_reveal_sids, **reveal_sids, **token_sids; + uint32_t num_never_reveal_sids, num_reveal_sids, num_token_sids; + struct dom_sid *never_reveal_sids, *reveal_sids, *token_sids; rodc_dn = ldb_dn_new_fmt(mem_ctx, sam_ctx, "", dom_sid_string(mem_ctx, user_sid)); @@ -2874,17 +2874,22 @@ static bool sam_rodc_access_check(struct ldb_context *sam_ctx, if (ret != LDB_SUCCESS || obj_res->count != 1) goto denied; object_sid = samdb_result_dom_sid(mem_ctx, obj_res->msgs[0], "objectSid"); - - additional_sids[0] = object_sid; + if (object_sid == NULL) { + goto denied; + } werr = samdb_result_sid_array_dn(sam_ctx, rodc_res->msgs[0], - mem_ctx, "msDS-NeverRevealGroup", &never_reveal_sids); + mem_ctx, "msDS-NeverRevealGroup", + &num_never_reveal_sids, + &never_reveal_sids); if (!W_ERROR_IS_OK(werr)) { goto denied; } werr = samdb_result_sid_array_dn(sam_ctx, rodc_res->msgs[0], - mem_ctx, "msDS-RevealOnDemandGroup", &reveal_sids); + mem_ctx, "msDS-RevealOnDemandGroup", + &num_reveal_sids, + &reveal_sids); if (!W_ERROR_IS_OK(werr)) { goto denied; } @@ -2895,19 +2900,27 @@ static bool sam_rodc_access_check(struct ldb_context *sam_ctx, * TODO determine if sIDHistory is required for this check */ werr = samdb_result_sid_array_ndr(sam_ctx, obj_res->msgs[0], - mem_ctx, "tokenGroups", &token_sids, - additional_sids, 1); + mem_ctx, "tokenGroups", + &num_token_sids, + &token_sids, + object_sid, 1); if (!W_ERROR_IS_OK(werr) || token_sids==NULL) { goto denied; } if (never_reveal_sids && - sid_list_match(token_sids, never_reveal_sids)) { + sid_list_match(num_token_sids, + token_sids, + num_never_reveal_sids, + never_reveal_sids)) { goto denied; } if (reveal_sids && - sid_list_match(token_sids, reveal_sids)) { + sid_list_match(num_token_sids, + token_sids, + num_reveal_sids, + reveal_sids)) { goto allowed; }