From fc65e4b49c9900aeca06276f05f31e20d28a6c20 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet@samba.org>
Date: Fri, 1 Oct 2021 10:47:29 +1300
Subject: [PATCH 232/284] CVE-2020-25718 s4-rpc_server: Change sid list
 
Conflict: NA 
Reference: https://git.samba.org/samba.git/?p=samba.git;a=patch;h=fc65e4b49c9900aeca06276f05f31e20d28a6c20

 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 <abartlet@samba.org>
Reviewed-by: Joseph Sutton <josephsutton@catalyst.net.nz>
---
 source4/rpc_server/common/sid_helper.c        | 44 +++++++++----------
 source4/rpc_server/drsuapi/getncchanges.c     | 33 +++++++++-----
 source4/rpc_server/netlogon/dcerpc_netlogon.c | 33 +++++++++-----
 3 files changed, 67 insertions(+), 43 deletions(-)

diff --git a/source4/rpc_server/common/sid_helper.c b/source4/rpc_server/common/sid_helper.c
index 698249391ef5..65d7e7c7271e 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; i<el->num_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; i<el->num_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 e458b2a99311..c7d2addd104d 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 9972138dbdef..c8dd0ceeb775 100644
--- a/source4/rpc_server/netlogon/dcerpc_netlogon.c
+++ b/source4/rpc_server/netlogon/dcerpc_netlogon.c
@@ -2850,10 +2850,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, "<SID=%s>",
 				 dom_sid_string(mem_ctx, user_sid));
@@ -2868,17 +2868,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;
 	}
@@ -2889,19 +2894,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;
 	}
 
-- 
2.25.1
