CVE-2020-25718 s4-rpc_server: Change sid list functions to operate on a array of...

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>

diff --git a/source4/rpc_server/common/sid_helper.c b/source4/rpc_server/common/sid_helper.c
index 698249391ef566c512e534c111424a5c9a9ca94a..65d7e7c7271e7bc8685cfb0686cd270f29bfa363 100644 (file)
--- 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 e458b2a99311922cdd2a964243f8b74360aa97f6..c7d2addd104d21a00012bd26cf4d62d6ac86df58 100644 (file)
--- 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 9972138dbdef60b24b4e8a1a446bc9ae746c8f53..c8dd0ceeb775b1fcb1fc5865a27c835b8cd3e17a 100644 (file)
--- 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;
        }