[reactos] 04/07: [NTOS:SE] Fix locking in SepDuplicateToken() and SepPerformTokenFiltering() (#4523)
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=3370652dfc2f1d0c37e067...
commit 3370652dfc2f1d0c37e067b31ee37af67a6320c8 Author: Hermès Bélusca-Maïto hermes.belusca-maito@reactos.org AuthorDate: Sun May 22 18:11:18 2022 +0200 Commit: Hermès Bélusca-Maïto hermes.belusca-maito@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;
-
Hermès Bélusca-Maïto