[ekohl] 46626: [NTOSKRNL] - Move subject context locking to SeAccessCheck because NtAccessCheck already locks it. - Do not use the captured security descriptor in NtAccessCheck yet, because SeCaptureSecurityDescriptor seems to create broken SDs.

Author: ekohl
Date: Wed Mar 31 21:53:19 2010
New Revision: 46626

URL: http://svn.reactos.org/svn/reactos?rev=46626&view=rev
Log:
[NTOSKRNL]
- Move subject context locking to SeAccessCheck because NtAccessCheck already locks it.
- Do not use the captured security descriptor in NtAccessCheck yet, because
SeCaptureSecurityDescriptor seems to create broken SDs.

Modified:
    trunk/reactos/ntoskrnl/se/semgr.c

Modified: trunk/reactos/ntoskrnl/se/semgr.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/se/semgr.c?rev=46…
==============================================================================
--- trunk/reactos/ntoskrnl/se/semgr.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/se/semgr.c [iso-8859-1] Wed Mar 31 21:53:19 2010
@@ -355,7 +355,6 @@
 BOOLEAN NTAPI
 SepAccessCheck(IN PSECURITY_DESCRIPTOR SecurityDescriptor,
                IN PSECURITY_SUBJECT_CONTEXT SubjectSecurityContext,
-               IN BOOLEAN SubjectContextLocked,
                IN ACCESS_MASK DesiredAccess,
                IN ACCESS_MASK PreviouslyGrantedAccess,
                OUT PPRIVILEGE_SET* Privileges,
@@ -394,9 +393,6 @@
         return TRUE;
     }
 
-    /* Acquire the lock if needed */
-    if (!SubjectContextLocked) SeLockSubjectContext(SubjectSecurityContext);
-
     /* Map given accesses */
     RtlMapGenericMask(&DesiredAccess, GenericMapping);
     if (PreviouslyGrantedAccess)
@@ -418,11 +414,6 @@
                                           &Defaulted);
     if (!NT_SUCCESS(Status))
     {
-        if (SubjectContextLocked == FALSE)
-        {
-            SeUnlockSubjectContext(SubjectSecurityContext);
-        }
-
         *AccessStatus = Status;
         return FALSE;
     }
@@ -430,11 +421,6 @@
     /* RULE 1: Grant desired access if the object is unprotected */
     if (Present == FALSE || Dacl == NULL)
     {
-        if (SubjectContextLocked == FALSE)
-        {
-            SeUnlockSubjectContext(SubjectSecurityContext);
-        }
-
         if (DesiredAccess & MAXIMUM_ALLOWED)
         {
             *GrantedAccess = GenericMapping->GenericAll;
@@ -465,11 +451,6 @@
         if ((DesiredAccess & ~VALID_INHERIT_FLAGS) == 
             (CurrentAccess & ~VALID_INHERIT_FLAGS))
         {
-            if (SubjectContextLocked == FALSE)
-            {
-                SeUnlockSubjectContext(SubjectSecurityContext);
-            }
-
             *GrantedAccess = CurrentAccess;
             *AccessStatus = STATUS_SUCCESS;
             return TRUE;
@@ -483,11 +464,6 @@
     if (!NT_SUCCESS(Status))
     {
         DPRINT1("RtlGetOwnerSecurityDescriptor() failed (Status %lx)\n",
Status);
-        if (SubjectContextLocked == FALSE)
-        {
-            SeUnlockSubjectContext(SubjectSecurityContext);
-        }
-
         *AccessStatus = Status;
         return FALSE;
     }
@@ -498,11 +474,6 @@
         if ((DesiredAccess & ~VALID_INHERIT_FLAGS) == 
             (CurrentAccess & ~VALID_INHERIT_FLAGS))
         {
-            if (SubjectContextLocked == FALSE)
-            {
-                SeUnlockSubjectContext(SubjectSecurityContext);
-            }
-
             *GrantedAccess = CurrentAccess;
             *AccessStatus = STATUS_SUCCESS;
             return TRUE;
@@ -512,11 +483,6 @@
     /* Fail if DACL is absent */
     if (Present == FALSE)
     {
-        if (SubjectContextLocked == FALSE)
-        {
-            SeUnlockSubjectContext(SubjectSecurityContext);
-        }
-
         *GrantedAccess = 0;
         *AccessStatus = STATUS_ACCESS_DENIED;
         return FALSE;
@@ -531,11 +497,6 @@
         {
             if (SepSidInToken(Token, Sid))
             {
-                if (SubjectContextLocked == FALSE)
-                {
-                    SeUnlockSubjectContext(SubjectSecurityContext);
-                }
-
                 *GrantedAccess = 0;
                 *AccessStatus = STATUS_ACCESS_DENIED;
                 return FALSE;
@@ -556,11 +517,6 @@
             DPRINT1("Unknown Ace type 0x%lx\n",
CurrentAce->Header.AceType);
         }
         CurrentAce = (PACE)((ULONG_PTR)CurrentAce + CurrentAce->Header.AceSize);
-    }
-
-    if (SubjectContextLocked == FALSE)
-    {
-        SeUnlockSubjectContext(SubjectSecurityContext);
     }
 
     DPRINT("CurrentAccess %08lx\n DesiredAccess %08lx\n",
@@ -639,6 +595,8 @@
               OUT PACCESS_MASK GrantedAccess,
               OUT PNTSTATUS AccessStatus)
 {
+    BOOLEAN ret;
+
     PAGED_CODE();
 
     /* Check if this is kernel mode */
@@ -679,17 +637,26 @@
         return FALSE;
     }
 
+    /* Acquire the lock if needed */
+    if (!SubjectContextLocked)
+        SeLockSubjectContext(SubjectSecurityContext);
+
     /* Call the internal function */
-    return SepAccessCheck(SecurityDescriptor,
-                          SubjectSecurityContext,
-                          SubjectContextLocked,
-                          DesiredAccess,
-                          PreviouslyGrantedAccess,
-                          Privileges,
-                          GenericMapping,
-                          AccessMode,
-                          GrantedAccess,
-                          AccessStatus);
+    ret = SepAccessCheck(SecurityDescriptor,
+                         SubjectSecurityContext,
+                         DesiredAccess,
+                         PreviouslyGrantedAccess,
+                         Privileges,
+                         GenericMapping,
+                         AccessMode,
+                         GrantedAccess,
+                         AccessStatus);
+
+    /* Release the lock if needed */
+    if (!SubjectContextLocked)
+        SeUnlockSubjectContext(SubjectSecurityContext);
+
+    return ret;
 }
 
 /* SYSTEM CALLS ***************************************************************/
@@ -808,8 +775,8 @@
     }
 
     /* Check security descriptor for valid owner and group */
-    if (SepGetSDOwner(SecurityDescriptor)== NULL ||
-        SepGetSDGroup(SecurityDescriptor) == NULL)
+    if (SepGetSDOwner(SecurityDescriptor) == NULL ||  // FIXME: use
CapturedSecurityDescriptor
+        SepGetSDGroup(SecurityDescriptor) == NULL)    // FIXME: use
CapturedSecurityDescriptor
     {
         DPRINT("Security Descriptor does not have a valid group or owner\n");
         SeReleaseSecurityDescriptor(CapturedSecurityDescriptor,
@@ -827,9 +794,8 @@
     SeLockSubjectContext(&SubjectSecurityContext);
 
     /* Now perform the access check */
-    SepAccessCheck(CapturedSecurityDescriptor,
+    SepAccessCheck(SecurityDescriptor, // FIXME: use CapturedSecurityDescriptor
                    &SubjectSecurityContext,
-                   TRUE,
                    DesiredAccess,
                    0,
                    &PrivilegeSet, //FIXME