[ros-diffs] [reactos] 02/07: [NTOS:SE] SepCaptureAcl(): Add missing validation of the captured ACL (#4523)

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

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=389a2da7ff94cfcefff32…

commit 389a2da7ff94cfcefff3220797f61311519d6fa0
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Thu May 19 23:43:18 2022 +0200
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Mon May 23 19:30:34 2022 +0200

    [NTOS:SE] SepCaptureAcl(): Add missing validation of the captured ACL (#4523)
    
    - The ACL is however not validated when the function is called within
      kernel mode and no capture is actually being done.
    
    - Simplify aspects of the function (returning early when possible).
---
 ntoskrnl/se/acl.c   | 85 ++++++++++++++++++++++++++++-------------------------
 ntoskrnl/se/token.c |  2 +-
 2 files changed, 46 insertions(+), 41 deletions(-)

diff --git a/ntoskrnl/se/acl.c b/ntoskrnl/se/acl.c
index e160b34c429..d7c53d4ac17 100644
--- a/ntoskrnl/se/acl.c
+++ b/ntoskrnl/se/acl.c
@@ -329,15 +329,15 @@ SepCreateImpersonationTokenDacl(
  *
  * @param[in] AccessMode
  * Processor level access mode. The processor mode determines how
- * are the input arguments probed.
+ * the input arguments are probed.
  *
  * @param[in] PoolType
  * Pool type for new captured ACL for creation. The pool type determines
- * how the ACL data should reside in the pool memory.
+ * in which memory pool the ACL data should reside.
  *
  * @param[in] CaptureIfKernel
- * If set to TRUE and the processor access mode being KernelMode, we're
- * capturing an ACL directly in the kernel. Otherwise we're capturing
+ * If set to TRUE and the processor access mode being KernelMode, we are
+ * capturing an ACL directly in the kernel. Otherwise we are capturing
  * within a kernel mode driver.
  *
  * @param[out] CapturedAcl
@@ -357,11 +357,19 @@ SepCaptureAcl(
     _Out_ PACL *CapturedAcl)
 {
     PACL NewAcl;
-    ULONG AclSize = 0;
-    NTSTATUS Status = STATUS_SUCCESS;
+    ULONG AclSize;
 
     PAGED_CODE();
 
+    /* If in kernel mode and we do not capture, just
+     * return the given ACL and don't validate it. */
+    if ((AccessMode == KernelMode) && !CaptureIfKernel)
+    {
+        *CapturedAcl = InputAcl;
+        return STATUS_SUCCESS;
+    }
+
+    /* Otherwise, capture and validate the ACL, depending on the access mode */
     if (AccessMode != KernelMode)
     {
         _SEH2_TRY
@@ -381,59 +389,56 @@ SepCaptureAcl(
         }
         _SEH2_END;
 
+        /* Validate the minimal size an ACL can have */
+        if (AclSize < sizeof(ACL))
+            return STATUS_INVALID_ACL;
+
         NewAcl = ExAllocatePoolWithTag(PoolType,
                                        AclSize,
                                        TAG_ACL);
-        if (NewAcl != NULL)
-        {
-            _SEH2_TRY
-            {
-                RtlCopyMemory(NewAcl,
-                              InputAcl,
-                              AclSize);
+        if (!NewAcl)
+            return STATUS_INSUFFICIENT_RESOURCES;
 
-                *CapturedAcl = NewAcl;
-            }
-            _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
-            {
-                /* Free the ACL and return the exception code */
-                ExFreePoolWithTag(NewAcl, TAG_ACL);
-                _SEH2_YIELD(return _SEH2_GetExceptionCode());
-            }
-            _SEH2_END;
+        _SEH2_TRY
+        {
+            RtlCopyMemory(NewAcl, InputAcl, AclSize);
         }
-        else
+        _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
         {
-            Status = STATUS_INSUFFICIENT_RESOURCES;
+            /* Free the ACL and return the exception code */
+            ExFreePoolWithTag(NewAcl, TAG_ACL);
+            _SEH2_YIELD(return _SEH2_GetExceptionCode());
         }
-    }
-    else if (!CaptureIfKernel)
-    {
-        *CapturedAcl = InputAcl;
+        _SEH2_END;
     }
     else
     {
         AclSize = InputAcl->AclSize;
 
+        /* Validate the minimal size an ACL can have */
+        if (AclSize < sizeof(ACL))
+            return STATUS_INVALID_ACL;
+
         NewAcl = ExAllocatePoolWithTag(PoolType,
                                        AclSize,
                                        TAG_ACL);
+        if (!NewAcl)
+            return STATUS_INSUFFICIENT_RESOURCES;
 
-        if (NewAcl != NULL)
-        {
-            RtlCopyMemory(NewAcl,
-                          InputAcl,
-                          AclSize);
+        RtlCopyMemory(NewAcl, InputAcl, AclSize);
+    }
 
-            *CapturedAcl = NewAcl;
-        }
-        else
-        {
-            Status = STATUS_INSUFFICIENT_RESOURCES;
-        }
+    /* Validate the captured ACL */
+    if (!RtlValidAcl(NewAcl))
+    {
+        /* Free the ACL and fail */
+        ExFreePoolWithTag(NewAcl, TAG_ACL);
+        return STATUS_INVALID_ACL;
     }
 
-    return Status;
+    /* It's valid, return it */
+    *CapturedAcl = NewAcl;
+    return STATUS_SUCCESS;
 }
 
 /**
diff --git a/ntoskrnl/se/token.c b/ntoskrnl/se/token.c
index 1415e36009d..f215f4c1fe2 100644
--- a/ntoskrnl/se/token.c
+++ b/ntoskrnl/se/token.c
@@ -4518,7 +4518,7 @@ NtSetInformationToken(
                     {
                         PACL CapturedAcl;
 
-                        /* Capture and copy the dacl */
+                        /* Capture, validate, and copy the DACL */
                         Status = SepCaptureAcl(InputAcl,
                                                PreviousMode,
                                                PagedPool,