From a389f8aa0ce16db83922eb04af26a546fdcec265 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?George=20Bi=C8=99oc?= <george.bisoc@reactos.org>
Date: Fri, 2 Jun 2023 18:41:12 +0200
Subject: [PATCH] [NTOS:SE] Make an access token effective after the end of
 token duplication

Removing any disabled privileges or groups in the middle of token dynamic
part allocation can pose problems. During the operation of making an access
token as effective, we are toying with the privileges and groups arrays
of the token.

After that we are allocating the dynamic part and set EndMem (the end tail
of the memory part) to that dynamic part, previously it was set to the
variable part. As a matter of fact we are making the token effective in
the middle where EndMem still points to VariablePart, thus DynamicPart
will end up with memory pool blocks butchered in the pool list.

Another problem, albeit not related to the DynamicPart corruption, is that
the code starts iterating over the UserAndGroups array from 0, which is
the actual user. One cannot simply remove the user from the array, so we
have to start looping right from the groups.

Move the token effective code part at the end of the SepDuplicateToken
function, which fixes the random pool corruptions caused by the butchered
DynamicPart.

CORE-18986
---
 ntoskrnl/se/tokenlif.c | 86 ++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 41 deletions(-)

diff --git a/ntoskrnl/se/tokenlif.c b/ntoskrnl/se/tokenlif.c
index 44f269ed40b..85a8ee01f78 100644
--- a/ntoskrnl/se/tokenlif.c
+++ b/ntoskrnl/se/tokenlif.c
@@ -665,6 +665,44 @@ SepDuplicateToken(
         }
     }
 
+    /* Now allocate the token's dynamic information area and set the data */
+    AccessToken->DynamicPart = ExAllocatePoolWithTag(PagedPool,
+                                                     DynamicPartSize,
+                                                     TAG_TOKEN_DYNAMIC);
+    if (AccessToken->DynamicPart == NULL)
+    {
+        Status = STATUS_INSUFFICIENT_RESOURCES;
+        goto Quit;
+    }
+
+    /* Unused memory in the dynamic area */
+    AccessToken->DynamicAvailable = 0;
+
+    /*
+     * Assign the primary group to the token
+     * and put it in the dynamic part as well.
+     */
+    EndMem = (PVOID)AccessToken->DynamicPart;
+    AccessToken->PrimaryGroup = EndMem;
+    RtlCopySid(RtlLengthSid(AccessToken->UserAndGroups[PrimaryGroupIndex].Sid),
+                            EndMem,
+                            AccessToken->UserAndGroups[PrimaryGroupIndex].Sid);
+    AccessToken->DefaultOwnerIndex = Token->DefaultOwnerIndex;
+    EndMem = (PVOID)((ULONG_PTR)EndMem + RtlLengthSid(AccessToken->UserAndGroups[PrimaryGroupIndex].Sid));
+
+    /*
+     * The existing token has a default DACL only
+     * if it has an allocated dynamic part.
+     */
+    if (Token->DynamicPart && Token->DefaultDacl)
+    {
+        AccessToken->DefaultDacl = EndMem;
+
+        RtlCopyMemory(EndMem,
+                      Token->DefaultDacl,
+                      Token->DefaultDacl->AclSize);
+    }
+
     /*
      * Filter the token by removing the disabled privileges
      * and groups if the caller wants to duplicate an access
@@ -672,11 +710,15 @@ SepDuplicateToken(
      */
     if (EffectiveOnly)
     {
-        /* Begin querying the groups and search for disabled ones */
-        for (GroupsIndex = 0; GroupsIndex < AccessToken->UserAndGroupCount; GroupsIndex++)
+        /*
+         * Begin querying the groups and search for disabled ones. Do not touch the
+         * user which is at the first position because it cannot be disabled, no
+         * matter what attributes it has.
+         */
+        for (GroupsIndex = 1; GroupsIndex < AccessToken->UserAndGroupCount; GroupsIndex++)
         {
             /*
-             * A group or user is considered disabled if its attributes is either
+             * A group is considered disabled if its attributes is either
              * 0 or SE_GROUP_ENABLED is not included in the attributes flags list.
              * That is because a certain user and/or group can have several attributes
              * that bear no influence on whether a user/group is enabled or not
@@ -738,44 +780,6 @@ SepDuplicateToken(
         }
     }
 
-    /* Now allocate the token's dynamic information area and set the data */
-    AccessToken->DynamicPart = ExAllocatePoolWithTag(PagedPool,
-                                                     DynamicPartSize,
-                                                     TAG_TOKEN_DYNAMIC);
-    if (AccessToken->DynamicPart == NULL)
-    {
-        Status = STATUS_INSUFFICIENT_RESOURCES;
-        goto Quit;
-    }
-
-    /* Unused memory in the dynamic area */
-    AccessToken->DynamicAvailable = 0;
-
-    /*
-     * Assign the primary group to the token
-     * and put it in the dynamic part as well.
-     */
-    EndMem = (PVOID)AccessToken->DynamicPart;
-    AccessToken->PrimaryGroup = EndMem;
-    RtlCopySid(RtlLengthSid(AccessToken->UserAndGroups[PrimaryGroupIndex].Sid),
-                            EndMem,
-                            AccessToken->UserAndGroups[PrimaryGroupIndex].Sid);
-    AccessToken->DefaultOwnerIndex = Token->DefaultOwnerIndex;
-    EndMem = (PVOID)((ULONG_PTR)EndMem + RtlLengthSid(AccessToken->UserAndGroups[PrimaryGroupIndex].Sid));
-
-    /*
-     * The existing token has a default DACL only
-     * if it has an allocated dynamic part.
-     */
-    if (Token->DynamicPart && Token->DefaultDacl)
-    {
-        AccessToken->DefaultDacl = EndMem;
-
-        RtlCopyMemory(EndMem,
-                      Token->DefaultDacl,
-                      Token->DefaultDacl->AclSize);
-    }
-
     /* Return the token to the caller */
     *NewAccessToken = AccessToken;
     Status = STATUS_SUCCESS;