7 Mar
2023
7 Mar
'23
5:08 p.m.
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=b284e82f478f40dd8a5b0…
commit b284e82f478f40dd8a5b046ea211c4938cf06cc8
Author: George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Tue Mar 7 11:42:22 2023 +0100
Commit: George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Tue Mar 7 17:50:39 2023 +0100
[NTOS:SE] Do not allocate memory pool just for the access rights
Access check is an expensive operation, that is, whenever an access to an
object is performed an access check has to be done to ensure the access
can be allowed to the calling thread who attempts to access such object.
Currently SepAnalyzeAcesFromDacl allocates a block of pool memory for
access check rights, nagging the Memory Manager like a desperate naughty
creep. So instead initialize the access rights as a simple variable in
SepAccessCheck and pass it out as an address to SepAnalyzeAcesFromDacl so
that the function will fill it up with access rights. This helps with
performance, avoiding wasting a few bits of memory just to hold these
access rights.
In addition to that, add a few asserts and fix the copyright header on
both se.h and accesschk.c, to reflect the Coding Style rules.
---
ntoskrnl/include/internal/se.h | 12 +-
ntoskrnl/include/internal/tag.h | 1 -
ntoskrnl/se/accesschk.c | 302 ++++++++++++----------------------------
ntoskrnl/se/debug.c | 11 +-
4 files changed, 103 insertions(+), 223 deletions(-)
diff --git a/ntoskrnl/include/internal/se.h b/ntoskrnl/include/internal/se.h
index 59cb4dbe631..d261b03c54a 100644
--- a/ntoskrnl/include/internal/se.h
+++ b/ntoskrnl/include/internal/se.h
@@ -1,9 +1,9 @@
/*
- * PROJECT: ReactOS Kernel
- * LICENSE: GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
- * PURPOSE: Internal header for the Security Manager
- * COPYRIGHT: Copyright Eric Kohl
- * Copyright 2022 George Bișoc <george.bisoc(a)reactos.org>
+ * PROJECT: ReactOS Kernel
+ * LICENSE: GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
+ * PURPOSE: Internal header for the Security Manager
+ * COPYRIGHT: Copyright Eric Kohl
+ * Copyright 2022 George Bișoc <george.bisoc(a)reactos.org>
*/
#pragma once
@@ -303,7 +303,7 @@ SepDumpTokenDebugInfo(
VOID
SepDumpAccessRightsStats(
- _In_opt_ PACCESS_CHECK_RIGHTS AccessRights);
+ _In_ PACCESS_CHECK_RIGHTS AccessRights);
#endif // DBG
//
diff --git a/ntoskrnl/include/internal/tag.h b/ntoskrnl/include/internal/tag.h
index f7a021152ce..35cf14d6ae6 100644
--- a/ntoskrnl/include/internal/tag.h
+++ b/ntoskrnl/include/internal/tag.h
@@ -164,7 +164,6 @@
#define TAG_LOGON_NOTIFICATION 'nLeS'
#define TAG_SID_AND_ATTRIBUTES 'aSeS'
#define TAG_SID_VALIDATE 'vSeS'
-#define TAG_ACCESS_CHECK_RIGHT 'rCeS'
#define TAG_DACL 'lcaD'
/* LPC Tags */
diff --git a/ntoskrnl/se/accesschk.c b/ntoskrnl/se/accesschk.c
index 0796dc8db78..690f7c029e7 100644
--- a/ntoskrnl/se/accesschk.c
+++ b/ntoskrnl/se/accesschk.c
@@ -1,10 +1,10 @@
/*
- * PROJECT: ReactOS Kernel
- * LICENSE: GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
- * PURPOSE: Security access check control implementation
- * COPYRIGHT: Copyright 2014 Timo Kreuzer <timo.kreuzer(a)reactos.org>
- * Copyright 2014 Eric Kohl
- * Copyright 2022 George Bișoc <george.bisoc(a)reactos.org>
+ * PROJECT: ReactOS Kernel
+ * LICENSE: GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
+ * PURPOSE: Security access check control implementation
+ * COPYRIGHT: Copyright 2014 Timo Kreuzer <timo.kreuzer(a)reactos.org>
+ * Copyright 2014 Eric Kohl
+ * Copyright 2022-2023 George Bișoc <george.bisoc(a)reactos.org>
*/
/* INCLUDES *******************************************************************/
@@ -15,69 +15,6 @@
/* PRIVATE FUNCTIONS **********************************************************/
-/**
- * @brief
- * Allocates memory for the internal access check rights
- * data structure and initializes it for use for the kernel.
- * The purpose of this piece of data is to track down the
- * remaining, granted and denied access rights whilst we
- * are doing an access check procedure.
- *
- * @return
- * Returns a pointer to allocated and initialized access
- * check rights, otherwise NULL is returned.
- */
-PACCESS_CHECK_RIGHTS
-SepInitAccessCheckRights(VOID)
-{
- PACCESS_CHECK_RIGHTS AccessRights;
-
- PAGED_CODE();
-
- /* Allocate some pool for access check rights */
- AccessRights = ExAllocatePoolWithTag(PagedPool,
- sizeof(ACCESS_CHECK_RIGHTS),
- TAG_ACCESS_CHECK_RIGHT);
-
- /* Bail out if we failed */
- if (!AccessRights)
- {
- return NULL;
- }
-
- /* Initialize the structure */
- AccessRights->RemainingAccessRights = 0;
- AccessRights->GrantedAccessRights = 0;
- AccessRights->DeniedAccessRights = 0;
-
- return AccessRights;
-}
-
-/**
- * @brief
- * Frees an allocated access check rights from
- * memory space after access check procedures
- * have finished.
- *
- * @param[in] AccessRights
- * A pointer to access check rights of which is
- * to be freed from memory.
- *
- * @return
- * Nothing.
- */
-VOID
-SepFreeAccessCheckRights(
- _In_ PACCESS_CHECK_RIGHTS AccessRights)
-{
- PAGED_CODE();
-
- if (AccessRights)
- {
- ExFreePoolWithTag(AccessRights, TAG_ACCESS_CHECK_RIGHT);
- }
-}
-
/**
* @brief
* Analyzes an access control entry that is present in a discretionary
@@ -92,6 +29,13 @@ SepFreeAccessCheckRights(
* requestor that gave us the acknowledgement that he desires MAXIMUM_ALLOWED access
* right or AccessCheckRegular if the requestor wants a subset of access rights.
*
+ * @param[in] RemainingAccess
+ * The remaining access rights that have yet to be granted to the calling thread
+ * whomst requests access to a certain object. This parameter mustn't be 0 as
+ * the remaining rights are left to be addressed. This is the case if we have
+ * to address the remaining rights on a regular subset basis (the requestor
+ * didn't ask for MAXIMUM_ALLOWED). Otherwise this parameter can be 0.
+ *
* @param[in] Dacl
* The discretionary access control list to be given to this function. This DACL
* must have at least one ACE currently present in the list.
@@ -113,12 +57,6 @@ SepFreeAccessCheckRights(
* for restricted SIDs only when doing an equaility comparison check between the
* two identifiers.
*
- * @param[in] AccessRightsAllocated
- * If this parameter is set to TRUE, the function will not allocate the access
- * check rights again. This is typical when we have to do additional analysis
- * of ACEs because a token has restricted SIDs (see IsTokenRestricted parameter)
- * of which we already initialized the access check rights pointer before.
- *
* @param[in] PrincipalSelfSid
* A pointer to a security identifier that represents a principal. A principal
* identifies a user object which is associated with its own security descriptor.
@@ -140,43 +78,30 @@ SepFreeAccessCheckRights(
* question represents the number of elements in such array. This parameter must be 0
* if no array list is provided.
*
- * @param[in] RemainingAccess
- * The remaining access rights that have yet to be granted to the calling thread
- * whomst requests access to a certain object. This parameter mustn't be 0 as
- * the remaining rights are left to be addressed. This is the case if we have
- * to address the remaining rights on a regular subset basis (the requestor
- * didn't ask for MAXIMUM_ALLOWED). Otherwise this parameter can be 0.
- *
- * @return
- * Returns a pointer to initialized access check rights after ACE analysis
- * has finished. This pointer contains the rights that have been acquired
- * in order to determine if access can be granted to the calling thread.
- * Typically this pointer contains the remaining, denied and granted rights.
- *
- * Otherwise NULL is returned and thus access check procedure can't any longer
- * continue further. We have prematurely failed this access check operation
- * at this point.
+ * @param[in,out] AccessCheckRights
+ * A pointer to a structure that contains the access check rights. This function fills
+ * up this structure with remaining, granted and denied rights to the caller for
+ * access check. Henceforth, this parameter must not be NULL!
*/
-PACCESS_CHECK_RIGHTS
+VOID
SepAnalyzeAcesFromDacl(
_In_ ACCESS_CHECK_RIGHT_TYPE ActionType,
+ _In_ ACCESS_MASK RemainingAccess,
_In_ PACL Dacl,
_In_ PACCESS_TOKEN AccessToken,
_In_ PACCESS_TOKEN PrimaryAccessToken,
_In_ BOOLEAN IsTokenRestricted,
- _In_ BOOLEAN AccessRightsAllocated,
_In_opt_ PSID PrincipalSelfSid,
_In_ PGENERIC_MAPPING GenericMapping,
_In_opt_ POBJECT_TYPE_LIST ObjectTypeList,
_In_ ULONG ObjectTypeListLength,
- _In_ ACCESS_MASK RemainingAccess)
+ _Inout_ PACCESS_CHECK_RIGHTS AccessCheckRights)
{
NTSTATUS Status;
PACE CurrentAce;
ULONG AceIndex;
PSID Sid;
ACCESS_MASK Access;
- PACCESS_CHECK_RIGHTS AccessRights;
PAGED_CODE();
@@ -191,25 +116,6 @@ SepAnalyzeAcesFromDacl(
/* TODO: To be removed once we support compound ACEs handling in Se */
DBG_UNREFERENCED_PARAMETER(PrimaryAccessToken);
- /*
- * Allocate memory for access check rights if
- * we have not done it so. Otherwise just use
- * the already allocated pointer. This is
- * typically when we have to do additional
- * ACEs analysis because the token has
- * restricted SIDs so we have allocated this
- * pointer before.
- */
- if (!AccessRightsAllocated)
- {
- AccessRights = SepInitAccessCheckRights();
- if (!AccessRights)
- {
- DPRINT1("SepAnalyzeAcesFromDacl(): Failed to initialize the access check
rights!\n");
- return NULL;
- }
- }
-
/* Determine how we should analyze the ACEs */
switch (ActionType)
{
@@ -239,6 +145,7 @@ SepAnalyzeAcesFromDacl(
{
/* Get the SID from this ACE */
Sid = SepGetSidFromAce(ACCESS_DENIED_ACE_TYPE, CurrentAce);
+ ASSERT(Sid);
if (SepSidInTokenEx(AccessToken, PrincipalSelfSid, Sid, TRUE,
IsTokenRestricted))
{
@@ -252,14 +159,15 @@ SepAnalyzeAcesFromDacl(
}
/* Deny access rights that have not been granted yet */
- AccessRights->DeniedAccessRights |= (Access &
~AccessRights->GrantedAccessRights);
- DPRINT("SepAnalyzeAcesFromDacl(): DeniedAccessRights
0x%08lx\n", AccessRights->DeniedAccessRights);
+ AccessCheckRights->DeniedAccessRights |= (Access &
~AccessCheckRights->GrantedAccessRights);
+ DPRINT("SepAnalyzeAcesFromDacl(): DeniedAccessRights
0x%08lx\n", AccessCheckRights->DeniedAccessRights);
}
}
else if (CurrentAce->Header.AceType == ACCESS_ALLOWED_ACE_TYPE)
{
/* Get the SID from this ACE */
Sid = SepGetSidFromAce(ACCESS_ALLOWED_ACE_TYPE, CurrentAce);
+ ASSERT(Sid);
if (SepSidInTokenEx(AccessToken, PrincipalSelfSid, Sid, FALSE,
IsTokenRestricted))
{
@@ -273,8 +181,8 @@ SepAnalyzeAcesFromDacl(
}
/* Grant access rights that have not been denied yet */
- AccessRights->GrantedAccessRights |= (Access &
~AccessRights->DeniedAccessRights);
- DPRINT("SepAnalyzeAcesFromDacl(): GrantedAccessRights
0x%08lx\n", AccessRights->GrantedAccessRights);
+ AccessCheckRights->GrantedAccessRights |= (Access &
~AccessCheckRights->DeniedAccessRights);
+ DPRINT("SepAnalyzeAcesFromDacl(): GrantedAccessRights
0x%08lx\n", AccessCheckRights->GrantedAccessRights);
}
}
else
@@ -296,7 +204,8 @@ SepAnalyzeAcesFromDacl(
case AccessCheckRegular:
{
/* Cache the remaining access rights to be addressed */
- AccessRights->RemainingAccessRights = RemainingAccess;
+ ASSERT(RemainingAccess != 0);
+ AccessCheckRights->RemainingAccessRights = RemainingAccess;
/* Loop over the DACL to retrieve ACEs */
for (AceIndex = 0; AceIndex < Dacl->AceCount; AceIndex++)
@@ -317,6 +226,7 @@ SepAnalyzeAcesFromDacl(
{
/* Get the SID from this ACE */
Sid = SepGetSidFromAce(ACCESS_DENIED_ACE_TYPE, CurrentAce);
+ ASSERT(Sid);
if (SepSidInTokenEx(AccessToken, PrincipalSelfSid, Sid, TRUE,
IsTokenRestricted))
{
@@ -334,10 +244,10 @@ SepAnalyzeAcesFromDacl(
* granted. Access is implicitly denied for
* the calling thread. Track this access right.
*/
- if (AccessRights->RemainingAccessRights & Access)
+ if (AccessCheckRights->RemainingAccessRights &
Access)
{
DPRINT("SepAnalyzeAcesFromDacl(): Refuted access
0x%08lx\n", Access);
- AccessRights->DeniedAccessRights |= Access;
+ AccessCheckRights->DeniedAccessRights |= Access;
break;
}
}
@@ -346,6 +256,7 @@ SepAnalyzeAcesFromDacl(
{
/* Get the SID from this ACE */
Sid = SepGetSidFromAce(ACCESS_ALLOWED_ACE_TYPE, CurrentAce);
+ ASSERT(Sid);
if (SepSidInTokenEx(AccessToken, PrincipalSelfSid, Sid, FALSE,
IsTokenRestricted))
{
@@ -358,13 +269,13 @@ SepAnalyzeAcesFromDacl(
RtlMapGenericMask(&Access, GenericMapping);
}
- /* Remove granted rights */
- DPRINT("SepAnalyzeAcesFromDacl(): RemainingAccessRights
0x%08lx Access 0x%08lx\n", AccessRights->RemainingAccessRights, Access);
- AccessRights->RemainingAccessRights &= ~Access;
- DPRINT("SepAnalyzeAcesFromDacl(): RemainingAccessRights
0x%08lx\n", AccessRights->RemainingAccessRights);
+ /* Remove the remaining rights */
+ DPRINT("SepAnalyzeAcesFromDacl(): RemainingAccessRights
0x%08lx Access 0x%08lx\n", AccessCheckRights->RemainingAccessRights, Access);
+ AccessCheckRights->RemainingAccessRights &= ~Access;
+ DPRINT("SepAnalyzeAcesFromDacl(): RemainingAccessRights
0x%08lx\n", AccessCheckRights->RemainingAccessRights);
/* Track the granted access right */
- AccessRights->GrantedAccessRights |= Access;
+ AccessCheckRights->GrantedAccessRights |= Access;
}
}
else
@@ -381,9 +292,6 @@ SepAnalyzeAcesFromDacl(
/* We shouldn't reach here */
DEFAULT_UNREACHABLE;
}
-
- /* Return the access rights that we've got */
- return AccessRights;
}
/**
@@ -486,7 +394,7 @@ SepAccessCheck(
BOOLEAN Defaulted;
NTSTATUS Status;
PACCESS_TOKEN Token = NULL;
- PACCESS_CHECK_RIGHTS AccessCheckRights = NULL;
+ ACCESS_CHECK_RIGHTS AccessCheckRights = {0};
PAGED_CODE();
@@ -604,31 +512,17 @@ SepAccessCheck(
if (DesiredAccess & MAXIMUM_ALLOWED)
{
/* Perform access checks against ACEs from this DACL */
- AccessCheckRights = SepAnalyzeAcesFromDacl(AccessCheckMaximum,
- Dacl,
- Token,
- PrimaryAccessToken,
- FALSE,
- FALSE,
- PrincipalSelfSid,
- GenericMapping,
- ObjectTypeList,
- ObjectTypeListLength,
- 0);
-
- /*
- * Getting the access check rights is very
- * important as we have to do access checks
- * depending on the kind of rights we get.
- * Fail prematurely if we can't...
- */
- if (!AccessCheckRights)
- {
- DPRINT1("SepAccessCheck(): Failed to obtain access check
rights!\n");
- Status = STATUS_INSUFFICIENT_RESOURCES;
- PreviouslyGrantedAccess = 0;
- goto ReturnCommonStatus;
- }
+ SepAnalyzeAcesFromDacl(AccessCheckMaximum,
+ 0,
+ Dacl,
+ Token,
+ PrimaryAccessToken,
+ FALSE,
+ PrincipalSelfSid,
+ GenericMapping,
+ ObjectTypeList,
+ ObjectTypeListLength,
+ &AccessCheckRights);
/*
* Perform further access checks if this token
@@ -636,21 +530,21 @@ SepAccessCheck(
*/
if (SeTokenIsRestricted(Token))
{
- AccessCheckRights = SepAnalyzeAcesFromDacl(AccessCheckMaximum,
- Dacl,
- Token,
- PrimaryAccessToken,
- TRUE,
- TRUE,
- PrincipalSelfSid,
- GenericMapping,
- ObjectTypeList,
- ObjectTypeListLength,
- 0);
+ SepAnalyzeAcesFromDacl(AccessCheckMaximum,
+ 0,
+ Dacl,
+ Token,
+ PrimaryAccessToken,
+ TRUE,
+ PrincipalSelfSid,
+ GenericMapping,
+ ObjectTypeList,
+ ObjectTypeListLength,
+ &AccessCheckRights);
}
/* Fail if some rights have not been granted */
- RemainingAccess &= ~(MAXIMUM_ALLOWED |
AccessCheckRights->GrantedAccessRights);
+ RemainingAccess &= ~(MAXIMUM_ALLOWED |
AccessCheckRights.GrantedAccessRights);
if (RemainingAccess != 0)
{
DPRINT1("SepAccessCheck(): Failed to grant access rights.
RemainingAccess = 0x%08lx DesiredAccess = 0x%08lx\n", RemainingAccess,
DesiredAccess);
@@ -660,7 +554,7 @@ SepAccessCheck(
}
/* Set granted access right and access status */
- PreviouslyGrantedAccess |= AccessCheckRights->GrantedAccessRights;
+ PreviouslyGrantedAccess |= AccessCheckRights.GrantedAccessRights;
if (PreviouslyGrantedAccess != 0)
{
Status = STATUS_SUCCESS;
@@ -676,36 +570,22 @@ SepAccessCheck(
}
/* Grant rights according to the DACL */
- AccessCheckRights = SepAnalyzeAcesFromDacl(AccessCheckRegular,
- Dacl,
- Token,
- PrimaryAccessToken,
- FALSE,
- FALSE,
- PrincipalSelfSid,
- GenericMapping,
- ObjectTypeList,
- ObjectTypeListLength,
- RemainingAccess);
-
- /*
- * Getting the access check rights is very
- * important as we have to do access checks
- * depending on the kind of rights we get.
- * Fail prematurely if we can't...
- */
- if (!AccessCheckRights)
- {
- DPRINT1("SepAccessCheck(): Failed to obtain access check rights!\n");
- Status = STATUS_INSUFFICIENT_RESOURCES;
- PreviouslyGrantedAccess = 0;
- goto ReturnCommonStatus;
- }
+ SepAnalyzeAcesFromDacl(AccessCheckRegular,
+ RemainingAccess,
+ Dacl,
+ Token,
+ PrimaryAccessToken,
+ FALSE,
+ PrincipalSelfSid,
+ GenericMapping,
+ ObjectTypeList,
+ ObjectTypeListLength,
+ &AccessCheckRights);
/* Fail if some rights have not been granted */
- if (AccessCheckRights->RemainingAccessRights != 0)
+ if (AccessCheckRights.RemainingAccessRights != 0)
{
- DPRINT1("SepAccessCheck(): Failed to grant access rights. RemainingAccess =
0x%08lx DesiredAccess = 0x%08lx\n", AccessCheckRights->RemainingAccessRights,
DesiredAccess);
+ DPRINT1("SepAccessCheck(): Failed to grant access rights. RemainingAccess =
0x%08lx DesiredAccess = 0x%08lx\n", AccessCheckRights.RemainingAccessRights,
DesiredAccess);
PreviouslyGrantedAccess = 0;
Status = STATUS_ACCESS_DENIED;
goto ReturnCommonStatus;
@@ -717,22 +597,22 @@ SepAccessCheck(
*/
if (SeTokenIsRestricted(Token))
{
- AccessCheckRights = SepAnalyzeAcesFromDacl(AccessCheckRegular,
- Dacl,
- Token,
- PrimaryAccessToken,
- TRUE,
- TRUE,
- PrincipalSelfSid,
- GenericMapping,
- ObjectTypeList,
- ObjectTypeListLength,
- RemainingAccess);
+ SepAnalyzeAcesFromDacl(AccessCheckRegular,
+ RemainingAccess,
+ Dacl,
+ Token,
+ PrimaryAccessToken,
+ TRUE,
+ PrincipalSelfSid,
+ GenericMapping,
+ ObjectTypeList,
+ ObjectTypeListLength,
+ &AccessCheckRights);
/* Fail if some rights have not been granted */
- if (AccessCheckRights->RemainingAccessRights != 0)
+ if (AccessCheckRights.RemainingAccessRights != 0)
{
- DPRINT1("SepAccessCheck(): Failed to grant access rights.
RemainingAccess = 0x%08lx DesiredAccess = 0x%08lx\n",
AccessCheckRights->RemainingAccessRights, DesiredAccess);
+ DPRINT1("SepAccessCheck(): Failed to grant access rights.
RemainingAccess = 0x%08lx DesiredAccess = 0x%08lx\n",
AccessCheckRights.RemainingAccessRights, DesiredAccess);
PreviouslyGrantedAccess = 0;
Status = STATUS_ACCESS_DENIED;
goto ReturnCommonStatus;
@@ -770,14 +650,10 @@ ReturnCommonStatus:
{
SepDumpSdDebugInfo(SecurityDescriptor);
SepDumpTokenDebugInfo(Token);
- SepDumpAccessRightsStats(AccessCheckRights);
+ SepDumpAccessRightsStats(&AccessCheckRights);
}
#endif
- /* Free the allocated access check rights */
- SepFreeAccessCheckRights(AccessCheckRights);
- AccessCheckRights = NULL;
-
return NT_SUCCESS(Status);
}
diff --git a/ntoskrnl/se/debug.c b/ntoskrnl/se/debug.c
index f21d5400938..87e4f1e57e1 100644
--- a/ntoskrnl/se/debug.c
+++ b/ntoskrnl/se/debug.c
@@ -323,10 +323,15 @@ SepDumpTokenDebugInfo(
*/
VOID
SepDumpAccessRightsStats(
- _In_opt_ PACCESS_CHECK_RIGHTS AccessRights)
+ _In_ PACCESS_CHECK_RIGHTS AccessRights)
{
- /* Don't dump anything if no access check rights list was provided */
- if (!AccessRights)
+ /*
+ * Dump the access rights only if we have remaining rights
+ * to dump in the first place. RemainingAccessRights can be 0
+ * if access check procedure has failed prematurely and this
+ * member hasn't been filled yet.
+ */
+ if (!AccessRights->RemainingAccessRights)
{
return;
}