[ros-diffs] [reactos] 02/03: [NTOS:PS] Do not reference the copied token twice and properly assign the impersonation level in case the server can't impersonate

[George Bișoc]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=8e2fe925f25cd14688ed8…

commit 8e2fe925f25cd14688ed82f4b84e2d1a8fb172cb
Author:     George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Tue Jun 6 17:24:15 2023 +0200
Commit:     George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Fri Jun 9 11:53:55 2023 +0200

    [NTOS:PS] Do not reference the copied token twice and properly assign the
impersonation level in case the server can't impersonate
    
    As it currently stands the PsImpersonateClient routine does the following approach.
    If impersonation couldn't be granted to a client the routine will make a copy
    of the client's access token. As it makes a copy of the said token
PsImpersonateClient
    will reference the copied token after impersonation info have been filled out.
    In the same code path we are assigning the desired level for impersonation to thread
    impersonation info.
    
    This is wrong for two reasons:
    
    - On a copy situation the SeCopyClientToken routine holds a reference as the object
    has been created. Referencing it at the bottom of the PsImpersonateClient routine
    will make it that the token is referenced twice and whenever a server stops
    impersonation the token still has an extra reference count which keeps the token
    still alive in object database and memory space.
    
    - If client impersonation is not possible the thread impersonation info should
    have been assigned SecurityIdentification level to further indicate that the
    actual impersonation of the thread is not currently in force but instead we
    are assigning the impersonation level that is supplied by the caller. For instance
    if the requested level is SecurityDelegation but impersonation is not possible
    the level will be assigned that of SecurityDelegation yet the token has an
    impersonation level of SecurityIdentification. This could lead to erratic behaviors
    as well as potential impersonation escalation.
    
    Fix the aforementioned issues by avoiding a double reference and properly assign
    the impersonation level to SecurityIdentification if the server is not able to
    impersonate the target client.
---
 ntoskrnl/ps/security.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/ntoskrnl/ps/security.c b/ntoskrnl/ps/security.c
index 23145529743..4d47764fc07 100644
--- a/ntoskrnl/ps/security.c
+++ b/ntoskrnl/ps/security.c
@@ -615,6 +615,7 @@ PsImpersonateClient(IN PETHREAD Thread,
 {
     PPS_IMPERSONATION_INFORMATION Impersonation, OldData;
     PTOKEN OldToken = NULL, ProcessToken = NULL;
+    BOOLEAN CopiedToken = FALSE;
     PACCESS_TOKEN NewToken, ImpersonationToken;
     PEJOB Job;
     NTSTATUS Status;
@@ -706,7 +707,13 @@ PsImpersonateClient(IN PETHREAD Thread,
                 return Status;
             }
 
-            /* Since we cannot impersonate, assign the newly copied token */
+            /*
+             * Since we cannot impersonate, assign the newly copied token.
+             * SeCopyClientToken already holds a reference to the copied token,
+             * let the code path below know that it must not reference it twice.
+             */
+            CopiedToken = TRUE;
+            ImpersonationLevel = SecurityIdentification;
             ImpersonationToken = NewToken;
         }
 
@@ -721,6 +728,11 @@ PsImpersonateClient(IN PETHREAD Thread,
             if ((Job->SecurityLimitFlags & JOB_OBJECT_SECURITY_NO_ADMIN)
&&
                 SeTokenIsAdmin(ImpersonationToken))
             {
+                if (CopiedToken)
+                {
+                    ObDereferenceObject(ImpersonationToken);
+                }
+
                 return STATUS_ACCESS_DENIED;
             }
 
@@ -728,6 +740,11 @@ PsImpersonateClient(IN PETHREAD Thread,
             if ((Job->SecurityLimitFlags & JOB_OBJECT_SECURITY_RESTRICTED_TOKEN)
&&
                 SeTokenIsRestricted(ImpersonationToken))
             {
+                if (CopiedToken)
+                {
+                    ObDereferenceObject(ImpersonationToken);
+                }
+
                 return STATUS_ACCESS_DENIED;
             }
 
@@ -758,7 +775,12 @@ PsImpersonateClient(IN PETHREAD Thread,
         Impersonation->CopyOnOpen = CopyOnOpen;
         Impersonation->EffectiveOnly = EffectiveOnly;
         Impersonation->Token = ImpersonationToken;
-        ObReferenceObject(ImpersonationToken);
+
+        /* Do not reference the token again if we copied it */
+        if (!CopiedToken)
+        {
+            ObReferenceObject(ImpersonationToken);
+        }
 
         /* Unlock the thread */
         PspUnlockThreadSecurityExclusive(Thread);