[ros-diffs] [reactos] 01/01: [NTOS:SE] Minor refactor on NtOpenThreadTokenEx

[George Bișoc]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=5912c1165073b8d1015ad…

commit 5912c1165073b8d1015ad1e0f0d229a7c50a181d
Author:     George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Fri Jun 18 18:38:12 2021 +0200
Commit:     George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Fri Jun 18 18:38:12 2021 +0200

    [NTOS:SE] Minor refactor on NtOpenThreadTokenEx
    
    - Remove a redundant call of ObReferenceObjectByHandle. Not only it didn't make
much sense (we reference the object from thread handle and the new thread object
referencing the same handle!), specifying a request access of THREAD_ALL_ACCESS for the
thread object is kind of suspicious and all of these access rights are unwanted.
    - Add some failure checks involving the CopyOnOpen code paths
    - Add some DPRINT1 debug prints (concerning the CopyOnOpen code paths as usual)
---
 ntoskrnl/se/token.c | 75 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 28 deletions(-)

diff --git a/ntoskrnl/se/token.c b/ntoskrnl/se/token.c
index 74ec4142917..507a8d73bad 100644
--- a/ntoskrnl/se/token.c
+++ b/ntoskrnl/se/token.c
@@ -4233,7 +4233,7 @@ NtOpenThreadTokenEx(IN HANDLE ThreadHandle,
                     IN ULONG HandleAttributes,
                     OUT PHANDLE TokenHandle)
 {
-    PETHREAD Thread, NewThread;
+    PETHREAD Thread;
     HANDLE hToken;
     PTOKEN Token, NewToken = NULL, PrimaryToken;
     BOOLEAN CopyOnOpen, EffectiveOnly;
@@ -4307,40 +4307,53 @@ NtOpenThreadTokenEx(IN HANDLE ThreadHandle,
 
     if (CopyOnOpen)
     {
-        Status = ObReferenceObjectByHandle(ThreadHandle, THREAD_ALL_ACCESS,
-                                           PsThreadType, KernelMode,
-                                           (PVOID*)&NewThread, NULL);
-        if (NT_SUCCESS(Status))
-        {
-            PrimaryToken = PsReferencePrimaryToken(NewThread->ThreadsProcess);
+        PrimaryToken = PsReferencePrimaryToken(Thread->ThreadsProcess);
 
-            Status = SepCreateImpersonationTokenDacl(Token, PrimaryToken, &Dacl);
+        Status = SepCreateImpersonationTokenDacl(Token, PrimaryToken, &Dacl);
 
-            ObFastDereferenceObject(&NewThread->ThreadsProcess->Token,
PrimaryToken);
+        ObFastDereferenceObject(&Thread->ThreadsProcess->Token, PrimaryToken);
 
-            if (NT_SUCCESS(Status))
+        if (NT_SUCCESS(Status))
+        {
+            if (Dacl)
             {
-                if (Dacl)
+                Status = RtlCreateSecurityDescriptor(&SecurityDescriptor,
+                                                     SECURITY_DESCRIPTOR_REVISION);
+                if (!NT_SUCCESS(Status))
                 {
-                    RtlCreateSecurityDescriptor(&SecurityDescriptor,
-                                                SECURITY_DESCRIPTOR_REVISION);
-                    RtlSetDaclSecurityDescriptor(&SecurityDescriptor, TRUE, Dacl,
-                                                 FALSE);
+                    DPRINT1("NtOpenThreadTokenEx(): Failed to create a security
descriptor (Status 0x%lx)\n", Status);
                 }
 
-                InitializeObjectAttributes(&ObjectAttributes, NULL,
HandleAttributes,
-                                           NULL, Dacl ? &SecurityDescriptor : NULL);
-
-                Status = SepDuplicateToken(Token, &ObjectAttributes, EffectiveOnly,
-                                           TokenImpersonation, ImpersonationLevel,
-                                           KernelMode, &NewToken);
-                if (NT_SUCCESS(Status))
+                Status = RtlSetDaclSecurityDescriptor(&SecurityDescriptor, TRUE,
Dacl,
+                                                      FALSE);
+                if (!NT_SUCCESS(Status))
                 {
-                    ObReferenceObject(NewToken);
-                    Status = ObInsertObject(NewToken, NULL, DesiredAccess, 0, NULL,
-                                            &hToken);
+                    DPRINT1("NtOpenThreadTokenEx(): Failed to set a DACL to the
security descriptor (Status 0x%lx)\n", Status);
                 }
             }
+
+            InitializeObjectAttributes(&ObjectAttributes, NULL, HandleAttributes,
+                                       NULL, Dacl ? &SecurityDescriptor : NULL);
+
+            Status = SepDuplicateToken(Token, &ObjectAttributes, EffectiveOnly,
+                                       TokenImpersonation, ImpersonationLevel,
+                                       KernelMode, &NewToken);
+            if (!NT_SUCCESS(Status))
+            {
+                DPRINT1("NtOpenThreadTokenEx(): Failed to duplicate the token
(Status 0x%lx)\n");
+            }
+
+            ObReferenceObject(NewToken);
+            Status = ObInsertObject(NewToken, NULL, DesiredAccess, 0, NULL,
+                                    &hToken);
+            if (!NT_SUCCESS(Status))
+            {
+                DPRINT1("NtOpenThreadTokenEx(): Failed to insert the token object
(Status 0x%lx)\n", Status);
+            }
+        }
+        else
+        {
+            DPRINT1("NtOpenThreadTokenEx(): Failed to impersonate token from DACL
(Status 0x%lx)\n", Status);
         }
     }
     else
@@ -4348,6 +4361,10 @@ NtOpenThreadTokenEx(IN HANDLE ThreadHandle,
         Status = ObOpenObjectByPointer(Token, HandleAttributes,
                                        NULL, DesiredAccess, SeTokenObjectType,
                                        PreviousMode, &hToken);
+        if (!NT_SUCCESS(Status))
+        {
+            DPRINT1("NtOpenThreadTokenEx(): Failed to open the object (Status
0x%lx)\n", Status);
+        }
     }
 
     if (Dacl) ExFreePoolWithTag(Dacl, TAG_ACL);
@@ -4361,13 +4378,15 @@ NtOpenThreadTokenEx(IN HANDLE ThreadHandle,
 
     if (NT_SUCCESS(Status) && CopyOnOpen)
     {
-        PsImpersonateClient(Thread, NewToken, FALSE, EffectiveOnly, ImpersonationLevel);
+        Status = PsImpersonateClient(Thread, NewToken, FALSE, EffectiveOnly,
ImpersonationLevel);
+        if (!NT_SUCCESS(Status))
+        {
+            DPRINT1("NtOpenThreadTokenEx(): Failed to impersonate the client (Status
0x%lx)\n");
+        }
     }
 
     if (NewToken) ObDereferenceObject(NewToken);
 
-    if (CopyOnOpen && NewThread) ObDereferenceObject(NewThread);
-
     ObDereferenceObject(Thread);
 
     if (NT_SUCCESS(Status))