[ros-diffs] [reactos] 04/07: [NTOS:SE] Fix locking in SepDuplicateToken() and SepPerformTokenFiltering() (#4523)

[Hermès Bélusca-Maïto]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=3370652dfc2f1d0c37e06…

commit 3370652dfc2f1d0c37e067b31ee37af67a6320c8
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Sun May 22 18:11:18 2022 +0200
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Mon May 23 19:30:35 2022 +0200

    [NTOS:SE] Fix locking in SepDuplicateToken() and SepPerformTokenFiltering() (#4523)
    
    Shared locking must be used on the source token as it is accessed for
    reading only. This fixes in particular the kmtest SeTokenFiltering that
    would hang otherwise on a (wrong) exclusive locking.
    
    - SepPerformTokenFiltering(): Always shared-lock the source token.
      Its callers (NtFilterToken and SeFilterToken) just need to sanitize and
      put the parameters in correct form before calling this helper function.
    
    - Sync comments in NtFilterToken() with SeFilterToken().
---
 ntoskrnl/se/token.c | 66 +++++++++++++++++++++++------------------------------
 1 file changed, 28 insertions(+), 38 deletions(-)

diff --git a/ntoskrnl/se/token.c b/ntoskrnl/se/token.c
index 96e972f2c5b..aad1321abd6 100644
--- a/ntoskrnl/se/token.c
+++ b/ntoskrnl/se/token.c
@@ -1059,7 +1059,7 @@ SepDuplicateToken(
     AccessToken->OriginatingLogonSession = Token->OriginatingLogonSession;
 
     /* Lock the source token and copy the mutable fields */
-    SepAcquireTokenLockExclusive(Token);
+    SepAcquireTokenLockShared(Token);
 
     AccessToken->SessionId = Token->SessionId;
     RtlCopyLuid(&AccessToken->ModifiedId, &Token->ModifiedId);
@@ -1286,23 +1286,20 @@ SepDuplicateToken(
                       Token->DefaultDacl->AclSize);
     }
 
-    /* Unlock the source token */
-    SepReleaseTokenLock(Token);
-
-    /* Return the token */
+    /* Return the token to the caller */
     *NewAccessToken = AccessToken;
     Status = STATUS_SUCCESS;
 
 Quit:
     if (!NT_SUCCESS(Status))
     {
-        /* Unlock the source token */
-        SepReleaseTokenLock(Token);
-
         /* Dereference the token, the delete procedure will clean it up */
         ObDereferenceObject(AccessToken);
     }
 
+    /* Unlock the source token */
+    SepReleaseTokenLock(Token);
+
     return Status;
 }
 
@@ -2086,8 +2083,8 @@ SepPerformTokenFiltering(
     _In_ KPROCESSOR_MODE PreviousMode,
     _Out_ PTOKEN *FilteredToken)
 {
-    PTOKEN AccessToken;
     NTSTATUS Status;
+    PTOKEN AccessToken;
     PVOID EndMem;
     ULONG RestrictedSidsLength;
     ULONG PrivilegesLength;
@@ -2100,10 +2097,12 @@ SepPerformTokenFiltering(
     BOOLEAN WantPrivilegesDisabled;
     BOOLEAN FoundPrivilege;
     BOOLEAN FoundGroup;
+
     PAGED_CODE();
 
-    /* Ensure that the token we get is not garbage */
+    /* Ensure that the source token is valid, and lock it */
     ASSERT(Token);
+    SepAcquireTokenLockShared(Token);
 
     /* Assume the caller doesn't want privileges disabled */
     WantPrivilegesDisabled = FALSE;
@@ -2159,6 +2158,9 @@ SepPerformTokenFiltering(
     if (!NT_SUCCESS(Status))
     {
         DPRINT1("SepPerformTokenFiltering(): Failed to create the filtered token
object (Status 0x%lx)\n", Status);
+
+        /* Unlock the source token and bail out */
+        SepReleaseTokenLock(Token);
         return Status;
     }
 
@@ -2168,10 +2170,7 @@ SepPerformTokenFiltering(
     /* Set up a lock for the new token */
     Status = SepCreateTokenLock(AccessToken);
     if (!NT_SUCCESS(Status))
-    {
-        ObDereferenceObject(AccessToken);
-        return Status;
-    }
+        goto Quit;
 
     /* Allocate new IDs for the token */
     ExAllocateLocallyUniqueId(&AccessToken->TokenId);
@@ -2576,7 +2575,7 @@ SepPerformTokenFiltering(
         AccessToken->TokenFlags |= TOKEN_IS_RESTRICTED;
     }
 
-    /* We've finally filtered the token, give it to the caller */
+    /* We've finally filtered the token, return it to the caller */
     *FilteredToken = AccessToken;
     Status = STATUS_SUCCESS;
     DPRINT("SepPerformTokenFiltering(): The token has been filtered!\n");
@@ -2584,10 +2583,13 @@ SepPerformTokenFiltering(
 Quit:
     if (!NT_SUCCESS(Status))
     {
-        /* Dereference the token */
+        /* Dereference the created token */
         ObDereferenceObject(AccessToken);
     }
 
+    /* Unlock the source token */
+    SepReleaseTokenLock(Token);
+
     return Status;
 }
 
@@ -2903,12 +2905,6 @@ SepCreateSystemAnonymousLogonTokenNoEveryone(VOID)
  * operations and that the access token has been filtered. STATUS_INVALID_PARAMETER
  * is returned if one or more of the parameter are not valid. A failure NTSTATUS code
  * is returned otherwise.
- *
- * @remarks
- * WARNING -- The caller IS RESPONSIBLE for locking the existing access token
- *            before attempting to do any kind of filtering operation into
- *            the token. The lock MUST BE RELEASED after this kernel routine
- *            has finished doing its work.
  */
 NTSTATUS
 NTAPI
@@ -2925,6 +2921,7 @@ SeFilterToken(
     ULONG PrivilegesCount = 0;
     ULONG SidsCount = 0;
     ULONG RestrictedSidsCount = 0;
+
     PAGED_CODE();
 
     /* Begin copying the counters */
@@ -2969,11 +2966,11 @@ SeFilterToken(
                             NULL);
     if (!NT_SUCCESS(Status))
     {
-        DPRINT1("SeFilterToken(): Failed to insert the token (Status 0x%lx)\n",
Status);
+        DPRINT1("SeFilterToken(): Failed to insert the filtered token (Status
0x%lx)\n", Status);
         return Status;
     }
 
-    /* Give it to the caller */
+    /* Return it to the caller */
     *FilteredToken = AccessToken;
     return Status;
 }
@@ -6661,7 +6658,7 @@ NtFilterToken(
     }
     _SEH2_END;
 
-    /* Reference the token and do the job */
+    /* Reference the token */
     Status = ObReferenceObjectByHandle(ExistingTokenHandle,
                                        TOKEN_DUPLICATE,
                                        SeTokenObjectType,
@@ -6674,9 +6671,6 @@ NtFilterToken(
         return Status;
     }
 
-    /* Lock the token */
-    SepAcquireTokenLockExclusive(Token);
-
     /* Capture the group SIDs */
     if (SidsToDisable != NULL)
     {
@@ -6734,7 +6728,7 @@ NtFilterToken(
         }
     }
 
-    /* Call the internal API so that it can filter the token for us */
+    /* Call the internal API */
     Status = SepPerformTokenFiltering(Token,
                                       CapturedPrivileges,
                                       CapturedSids,
@@ -6751,7 +6745,7 @@ NtFilterToken(
         goto Quit;
     }
 
-    /* We got our filtered token, insert it to the handle */
+    /* Insert the filtered token and retrieve a handle to it */
     Status = ObInsertObject(FilteredToken,
                             NULL,
                             HandleInfo.GrantedAccess,
@@ -6760,11 +6754,11 @@ NtFilterToken(
                             &FilteredTokenHandle);
     if (!NT_SUCCESS(Status))
     {
-        DPRINT1("NtFilterToken(): Failed to insert the filtered token object into
the handle (Status 0x%lx)\n", Status);
+        DPRINT1("NtFilterToken(): Failed to insert the filtered token (Status
0x%lx)\n", Status);
         goto Quit;
     }
 
-    /* And give it to the caller once we're done */
+    /* And return it to the caller once we're done */
     _SEH2_TRY
     {
         *NewTokenHandle = FilteredTokenHandle;
@@ -6777,17 +6771,15 @@ NtFilterToken(
     _SEH2_END;
 
 Quit:
-    /* Unlock and dereference the token */
-    SepReleaseTokenLock(Token);
+    /* Dereference the token */
     ObDereferenceObject(Token);
 
-    /* Release all the stuff we've captured */
+    /* Release all the captured data */
     if (CapturedSids != NULL)
     {
         SeReleaseSidAndAttributesArray(CapturedSids,
                                        PreviousMode,
                                        TRUE);
-        CapturedSids = NULL;
     }
 
     if (CapturedPrivileges != NULL)
@@ -6795,7 +6787,6 @@ Quit:
         SeReleaseLuidAndAttributesArray(CapturedPrivileges,
                                         PreviousMode,
                                         TRUE);
-        CapturedPrivileges = NULL;
     }
 
     if (CapturedRestrictedSids != NULL)
@@ -6803,7 +6794,6 @@ Quit:
         SeReleaseSidAndAttributesArray(CapturedRestrictedSids,
                                        PreviousMode,
                                        TRUE);
-        CapturedRestrictedSids = NULL;
     }
 
     return Status;