[tfaber] 65244: [NTOS:SE] Various minor behavior corrections & simplifications for SeAssignSecurityEx: - Fail on missing subject context or invalid group - Initialize NewDescriptor to NULL on failu...

Author: tfaber
Date: Tue Nov  4 12:35:22 2014
New Revision: 65244

URL: http://svn.reactos.org/svn/reactos?rev=65244&view=rev
Log:
[NTOS:SE]
Various minor behavior corrections & simplifications for SeAssignSecurityEx:
- Fail on missing subject context or invalid group
- Initialize NewDescriptor to NULL on failure
- Never set SE_*_DEFAULTED flags
- Assume a primary token exists
- Remove unnecessary variable initialization
- Remove unnecessary length alignment
CORE-8745

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

Modified: trunk/reactos/ntoskrnl/se/sd.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/se/sd.c?rev=65244…
==============================================================================
--- trunk/reactos/ntoskrnl/se/sd.c	[iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/se/sd.c	[iso-8859-1] Tue Nov  4 12:35:22 2014
@@ -1173,12 +1173,12 @@
     PISECURITY_DESCRIPTOR ExplicitDescriptor = _ExplicitDescriptor;
     PISECURITY_DESCRIPTOR_RELATIVE Descriptor;
     PTOKEN Token;
-    ULONG OwnerLength = 0;
-    ULONG GroupLength = 0;
-    ULONG DaclLength = 0;
-    ULONG SaclLength = 0;
-    ULONG Length = 0;
-    ULONG Control = 0;
+    ULONG OwnerLength;
+    ULONG GroupLength;
+    ULONG DaclLength;
+    ULONG SaclLength;
+    ULONG Length;
+    SECURITY_DESCRIPTOR_CONTROL Control = 0;
     ULONG Current;
     PSID Owner = NULL;
     PSID Group = NULL;
@@ -1192,6 +1192,13 @@
 
     PAGED_CODE();
 
+    *NewDescriptor = NULL;
+
+    if (!ARGUMENT_PRESENT(SubjectContext))
+    {
+        return STATUS_NO_TOKEN;
+    }
+
     /* Lock subject context */
     SeLockSubjectContext(SubjectContext);
 
@@ -1210,48 +1217,33 @@
         DPRINT("Use explicit owner sid!\n");
         Owner = SepGetOwnerFromDescriptor(ExplicitDescriptor);
     }
-
     if (!Owner)
     {
-        if (Token != NULL)
-        {
-            DPRINT("Use token owner sid!\n");
-            Owner = Token->UserAndGroups[Token->DefaultOwnerIndex].Sid;
-        }
-        else
-        {
-            DPRINT("Use default owner sid!\n");
-            Owner = SeLocalSystemSid;
-        }
-
-        Control |= SE_OWNER_DEFAULTED;
-    }
-
-    OwnerLength = ROUND_UP(RtlLengthSid(Owner), 4);
+        DPRINT("Use token owner sid!\n");
+        Owner = Token->UserAndGroups[Token->DefaultOwnerIndex].Sid;
+    }
+
+    OwnerLength = RtlLengthSid(Owner);
+    NT_ASSERT(OwnerLength % sizeof(ULONG) == 0);
 
     /* Inherit the Group SID */
     if (ExplicitDescriptor != NULL)
     {
         Group = SepGetGroupFromDescriptor(ExplicitDescriptor);
     }
-
     if (!Group)
     {
-        if (Token != NULL)
-        {
-            DPRINT("Use token group sid!\n");
-            Group = Token->PrimaryGroup;
-        }
-        else
-        {
-            DPRINT("Use default group sid!\n");
-            Group = SeLocalSystemSid;
-        }
-
-        Control |= SE_GROUP_DEFAULTED;
-    }
-
-    GroupLength = ROUND_UP(RtlLengthSid(Group), 4);
+        DPRINT("Use token group sid!\n");
+        Group = Token->PrimaryGroup;
+    }
+    if (!Group)
+    {
+        SeUnlockSubjectContext(SubjectContext);
+        return STATUS_INVALID_PRIMARY_GROUP;
+    }
+
+    GroupLength = RtlLengthSid(Group);
+    NT_ASSERT(GroupLength % sizeof(ULONG) == 0);
 
     /* Inherit the DACL */
     if (ExplicitDescriptor != NULL &&
@@ -1268,23 +1260,17 @@
         DPRINT("Use parent DACL!\n");
         /* FIXME: Inherit */
         Dacl = SepGetDaclFromDescriptor(ParentDescriptor);
-        Control |= (SE_DACL_PRESENT | SE_DACL_DEFAULTED);
-    }
-    else if (Token != NULL && Token->DefaultDacl != NULL)
+        Control |= SE_DACL_PRESENT;
+    }
+    else if (Token->DefaultDacl)
     {
         DPRINT("Use token default DACL!\n");
-        /* FIXME: Inherit */
         Dacl = Token->DefaultDacl;
-        Control |= (SE_DACL_PRESENT | SE_DACL_DEFAULTED);
-    }
-    else
-    {
-        DPRINT("Use NULL DACL!\n");
-        Dacl = NULL;
-        Control |= (SE_DACL_PRESENT | SE_DACL_DEFAULTED);
-    }
-
-    DaclLength = (Dacl != NULL) ? ROUND_UP(Dacl->AclSize, 4) : 0;
+        Control |= SE_DACL_PRESENT;
+    }
+
+    DaclLength = (Dacl != NULL) ? Dacl->AclSize : 0;
+    NT_ASSERT(DaclLength % sizeof(ULONG) == 0);
 
     /* Inherit the SACL */
     if (ExplicitDescriptor != NULL &&
@@ -1301,10 +1287,11 @@
         DPRINT("Use parent SACL!\n");
         /* FIXME: Inherit */
         Sacl = SepGetSaclFromDescriptor(ParentDescriptor);
-        Control |= (SE_SACL_PRESENT | SE_SACL_DEFAULTED);
-    }
-
-    SaclLength = (Sacl != NULL) ? ROUND_UP(Sacl->AclSize, 4) : 0;
+        Control |= SE_SACL_PRESENT;
+    }
+
+    SaclLength = (Sacl != NULL) ? Sacl->AclSize : 0;
+    NT_ASSERT(SaclLength % sizeof(ULONG) == 0);
 
     /* Allocate and initialize the new security descriptor */
     Length = sizeof(SECURITY_DESCRIPTOR_RELATIVE) +
@@ -1328,7 +1315,7 @@
     RtlZeroMemory(Descriptor, Length);
     RtlCreateSecurityDescriptor(Descriptor, SECURITY_DESCRIPTOR_REVISION);
 
-    Descriptor->Control = (USHORT)Control | SE_SELF_RELATIVE;
+    Descriptor->Control = Control | SE_SELF_RELATIVE;
 
     Current = sizeof(SECURITY_DESCRIPTOR_RELATIVE);