https://git.reactos.org/?p=reactos.git;a=commitdiff;h=4bf32102abfacf4917dd1…
commit 4bf32102abfacf4917dd18d00673608a8fa877c7
Author: Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Thu May 5 22:21:21 2022 +0200
Commit: Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Fri May 6 21:32:08 2022 +0200
[SHELL32_APITEST] ShellExecCmdLine: Do **NOT** use TerminateProcess to close the opened windows.
This design, introduced in 418edcd2b, is fundamentally flawed as it
can also close windows unrelated to the running test (e.g. windows
of programs that the user can start, while the test is running).
But since we cannot do much better, mitigate instead the other main
problem of this design: Just use PostMessageW(WM_CLOSE), as it used
to be, instead of TerminateProcess().
Indeed, using TerminateProcess() otherwise could kill unrelated
processes the test hasn't created. In particular it could kill the
csrss.exe system process if, during the testing procedure, a hard-error
popup shows up.
And this is precisely the case when this test runs with limited memory,
and a hard-error
"Insufficient system resources exist to complete the requested service."
arises.
---
modules/rostests/apitests/shell32/ShellExecCmdLine.cpp | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/modules/rostests/apitests/shell32/ShellExecCmdLine.cpp b/modules/rostests/apitests/shell32/ShellExecCmdLine.cpp
index e869e6deee5..218ff08ed88 100644
--- a/modules/rostests/apitests/shell32/ShellExecCmdLine.cpp
+++ b/modules/rostests/apitests/shell32/ShellExecCmdLine.cpp
@@ -609,13 +609,7 @@ static void CleanupNewlyCreatedWindows(void)
}
}
if (!bFound)
- {
- DWORD dwPID;
- GetWindowThreadProcessId(s_wi1.phwnd[i1], &dwPID);
- HANDLE hProcess = OpenProcess(PROCESS_TERMINATE, TRUE, dwPID);
- TerminateProcess(hProcess, -1);
- CloseHandle(hProcess);
- }
+ PostMessageW(s_wi1.phwnd[i1], WM_CLOSE, 0, 0);
}
free(s_wi1.phwnd);
ZeroMemory(&s_wi1, sizeof(s_wi1));
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=55c117c4c932ed17d1338…
commit 55c117c4c932ed17d1338c3c56b305915a0fa88f
Author: George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Sat Feb 5 22:38:22 2022 +0100
Commit: George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Fri May 6 10:09:53 2022 +0200
[NTOS:SE] Deny access to the caller if access is not allowed by the object
There are two fundamental problems when it comes to access checks in ReactOS. First, the internal function SepAccessCheck which is the heart and brain of the whole access checks logic of the kernel warrants access to the calling thread of a process to an object even though access could not be given.
This can potentially leave security issues as we literally leave objects to be touched indiscriminately by anyone regardless of their ACEs in the DACL of a security descriptor. Second, the current access check code doesn't take into account the fact that an access token can have restricted SIDs. In such scenario we must perform additional access checks by iterating over the restricted SIDs of the primary token by comparing the SID equality and see if the group can be granted certain r [...]
Part of SepAccessCheck's code logic will be split for a separate private kernel routine, SepAnalyzeAcesFromDacl. The reasons for this are primarily two -- such code is subject to grow eventually as we'll support different type ACEs and handle them accordingly -- and we avoid further code duplicates. On Windows Server 2003 there are 5 different type of ACEs that are supported for access checks:
- ACCESS_DENIED_ACE_TYPE (supported by ReactOS)
- ACCESS_ALLOWED_ACE_TYPE (supported by ReactOS)
- ACCESS_DENIED_OBJECT_ACE_TYPE
- ACCESS_ALLOWED_OBJECT_ACE_TYPE
- ACCESS_ALLOWED_COMPOUND_ACE_TYPE
This gives the opportunity for us to have a semi serious kernel where security of objects are are taken into account, rather than giving access to everyone.
CORE-9174
CORE-9175
CORE-9184
CORE-14520
---
ntoskrnl/se/accesschk.c | 749 ++++++++++++++++++++++++++++++++++++++----------
1 file changed, 599 insertions(+), 150 deletions(-)
diff --git a/ntoskrnl/se/accesschk.c b/ntoskrnl/se/accesschk.c
index a8322c326ac..2a92a0ef247 100644
--- a/ntoskrnl/se/accesschk.c
+++ b/ntoskrnl/se/accesschk.c
@@ -2,8 +2,9 @@
* 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 Timo Kreuzer <timo.kreuzer(a)reactos.org>
- * Copyright Eric Kohl
+ * COPYRIGHT: Copyright 2014 Timo Kreuzer <timo.kreuzer(a)reactos.org>
+ * Copyright 2014 Eric Kohl
+ * Copyright 2022 George Bișoc <george.bisoc(a)reactos.org>
*/
/* INCLUDES *******************************************************************/
@@ -12,89 +13,488 @@
#define NDEBUG
#include <debug.h>
-/* GLOBALS ********************************************************************/
+/* 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);
-/* PRIVATE FUNCTIONS **********************************************************/
+ /* 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
+ * access control list (DACL) for access right masks of each entry with
+ * the purpose to judge whether the calling thread can be warranted
+ * access check to a certain object or not.
+ *
+ * @param[in] ActionType
+ * The type of analysis to be done against an access entry. This type
+ * influences how access rights are gathered. This can either be AccessCheckMaximum
+ * which means the algorithm will perform analysis against ACEs on behalf of the
+ * 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] 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.
+ *
+ * @param[in] AccessToken
+ * A pointer to an access token, where an equality comparison check is performed if
+ * the security identifier (SID) from a ACE of a certain object is present in this
+ * token. This token represents the effective (calling thread) token of the caller.
+ *
+ * @param[in] PrimaryAccessToken
+ * A pointer to an access token, represented as an access token associated with the
+ * primary calling process. This token describes the primary security context of the
+ * main process.
+ *
+ * @param[in] IsTokenRestricted
+ * If this parameter is set to TRUE, the function considers the token pointed by
+ * AccessToken parameter argument as restricted. That is, the token has restricted
+ * SIDs therefore the function will act accordingly against that token by checking
+ * 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.
+ *
+ * @param[in] GenericMapping
+ * A pointer to a generic mapping that is associated with the object in question
+ * being checked for access. If certain set of desired access rights have
+ * a generic access right, this parameter is needed to map generic rights.
+ *
+ * @param[in] ObjectTypeList
+ * A pointer to a list array of object types. If such array is provided to the
+ * function, the algorithm will perform a different approach by doing analysis
+ * against ACEs each sub-object of an object of primary level (level 0) or sub-objects
+ * of a sub-object of an object. If this parameter is NULL, the function will normally
+ * analyze the ACEs of a DACL of the target object itself.
+ *
+ * @param[in] ObjectTypeListLength
+ * The length of the object type list array, pointed by ObjectTypeList. This length in
+ * 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.
+ */
+PACCESS_CHECK_RIGHTS
+SepAnalyzeAcesFromDacl(
+ _In_ ACCESS_CHECK_RIGHT_TYPE ActionType,
+ _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)
+{
+ NTSTATUS Status;
+ PACE CurrentAce;
+ ULONG AceIndex;
+ PSID Sid;
+ ACCESS_MASK Access;
+ PACCESS_CHECK_RIGHTS AccessRights;
+
+ PAGED_CODE();
+
+ /* These parameters are really needed */
+ ASSERT(Dacl);
+ ASSERT(AccessToken);
+
+ /* TODO: To be removed once we support object type handling in Se */
+ DBG_UNREFERENCED_PARAMETER(ObjectTypeList);
+ DBG_UNREFERENCED_PARAMETER(ObjectTypeListLength);
+
+ /* 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)
+ {
+ /*
+ * We got the acknowledgement the calling thread desires
+ * maximum rights (as according to MAXIMUM_ALLOWED access
+ * mask). Analyze the ACE of the given DACL.
+ */
+ case AccessCheckMaximum:
+ {
+ /* Loop over the DACL to retrieve ACEs */
+ for (AceIndex = 0; AceIndex < Dacl->AceCount; AceIndex++)
+ {
+ /* Obtain a ACE now */
+ Status = RtlGetAce(Dacl, AceIndex, (PVOID*)&CurrentAce);
+
+ /* Getting this ACE is important, otherwise something is seriously wrong */
+ ASSERT(NT_SUCCESS(Status));
+
+ /*
+ * Now it's time to analyze it based upon the
+ * type of this ACE we're being given.
+ */
+ if (!(CurrentAce->Header.AceFlags & INHERIT_ONLY_ACE))
+ {
+ if (CurrentAce->Header.AceType == ACCESS_DENIED_ACE_TYPE)
+ {
+ /* Get the SID from this ACE */
+ Sid = SepGetSidFromAce(ACCESS_DENIED_ACE_TYPE, CurrentAce);
+
+ if (SepSidInTokenEx(AccessToken, PrincipalSelfSid, Sid, TRUE, IsTokenRestricted))
+ {
+ /* Get this access right from the ACE */
+ Access = CurrentAce->AccessMask;
+
+ /* Map this access right if it has a generic mask right */
+ if ((Access & GENERIC_ACCESS) && GenericMapping)
+ {
+ RtlMapGenericMask(&Access, GenericMapping);
+ }
+
+ /* Deny access rights that have not been granted yet */
+ AccessRights->DeniedAccessRights |= (Access & ~AccessRights->GrantedAccessRights);
+ DPRINT("SepAnalyzeAcesFromDacl(): DeniedAccessRights 0x%08lx\n", AccessRights->DeniedAccessRights);
+ }
+ }
+ else if (CurrentAce->Header.AceType == ACCESS_ALLOWED_ACE_TYPE)
+ {
+ /* Get the SID from this ACE */
+ Sid = SepGetSidFromAce(ACCESS_ALLOWED_ACE_TYPE, CurrentAce);
+
+ if (SepSidInTokenEx(AccessToken, PrincipalSelfSid, Sid, FALSE, IsTokenRestricted))
+ {
+ /* Get this access right from the ACE */
+ Access = CurrentAce->AccessMask;
+
+ /* Map this access right if it has a generic mask right */
+ if ((Access & GENERIC_ACCESS) && GenericMapping)
+ {
+ RtlMapGenericMask(&Access, GenericMapping);
+ }
+
+ /* Grant access rights that have not been denied yet */
+ AccessRights->GrantedAccessRights |= (Access & ~AccessRights->DeniedAccessRights);
+ DPRINT("SepAnalyzeAcesFromDacl(): GrantedAccessRights 0x%08lx\n", AccessRights->GrantedAccessRights);
+ }
+ }
+ else
+ {
+ DPRINT1("SepAnalyzeAcesFromDacl(): Unsupported ACE type 0x%lx\n", CurrentAce->Header.AceType);
+ }
+ }
+ }
+
+ /* We're done here */
+ break;
+ }
+
+ /*
+ * We got the acknowledgement the calling thread desires
+ * only a subset of rights therefore we have to act a little
+ * different here.
+ */
+ case AccessCheckRegular:
+ {
+ /* Cache the remaining access rights to be addressed */
+ AccessRights->RemainingAccessRights = RemainingAccess;
+
+ /* Loop over the DACL to retrieve ACEs */
+ for (AceIndex = 0; AceIndex < Dacl->AceCount; AceIndex++)
+ {
+ /* Obtain a ACE now */
+ Status = RtlGetAce(Dacl, AceIndex, (PVOID*)&CurrentAce);
+
+ /* Getting this ACE is important, otherwise something is seriously wrong */
+ ASSERT(NT_SUCCESS(Status));
+
+ /*
+ * Now it's time to analyze it based upon the
+ * type of this ACE we're being given.
+ */
+ if (!(CurrentAce->Header.AceFlags & INHERIT_ONLY_ACE))
+ {
+ if (CurrentAce->Header.AceType == ACCESS_DENIED_ACE_TYPE)
+ {
+ /* Get the SID from this ACE */
+ Sid = SepGetSidFromAce(ACCESS_DENIED_ACE_TYPE, CurrentAce);
+
+ if (SepSidInTokenEx(AccessToken, PrincipalSelfSid, Sid, TRUE, IsTokenRestricted))
+ {
+ /* Get this access right from the ACE */
+ Access = CurrentAce->AccessMask;
+
+ /* Map this access right if it has a generic mask right */
+ if ((Access & GENERIC_ACCESS) && GenericMapping)
+ {
+ RtlMapGenericMask(&Access, GenericMapping);
+ }
+
+ /*
+ * The caller requests a right that cannot be
+ * granted. Access is implicitly denied for
+ * the calling thread. Track this access right.
+ */
+ if (AccessRights->RemainingAccessRights & Access)
+ {
+ DPRINT("SepAnalyzeAcesFromDacl(): Refuted access 0x%08lx\n", Access);
+ AccessRights->DeniedAccessRights |= Access;
+ break;
+ }
+ }
+ }
+ else if (CurrentAce->Header.AceType == ACCESS_ALLOWED_ACE_TYPE)
+ {
+ /* Get the SID from this ACE */
+ Sid = SepGetSidFromAce(ACCESS_ALLOWED_ACE_TYPE, CurrentAce);
+
+ if (SepSidInTokenEx(AccessToken, PrincipalSelfSid, Sid, FALSE, IsTokenRestricted))
+ {
+ /* Get this access right from the ACE */
+ Access = CurrentAce->AccessMask;
+
+ /* Map this access right if it has a generic mask right */
+ if ((Access & GENERIC_ACCESS) && GenericMapping)
+ {
+ 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);
+
+ /* Track the granted access right */
+ AccessRights->GrantedAccessRights |= Access;
+ }
+ }
+ else
+ {
+ DPRINT1("SepAnalyzeAcesFromDacl(): Unsupported ACE type 0x%lx\n", CurrentAce->Header.AceType);
+ }
+ }
+ }
+
+ /* We're done here */
+ break;
+ }
+
+ /* We shouldn't reach here */
+ DEFAULT_UNREACHABLE;
+ }
+
+ /* Return the access rights that we've got */
+ return AccessRights;
+}
/**
* @brief
* Private function that determines whether security access rights can be given
- * to an object depending on the security descriptor and other security context
- * entities, such as an owner.
+ * to the calling thread in order to access an object depending on the security
+ * descriptor and other security context entities, such as an owner. This
+ * function is the heart and brain of the whole access check algorithm in
+ * the kernel.
*
- * @param[in] SecurityDescriptor
- * Security descriptor of the object that is being accessed.
+ * @param[in] ClientAccessToken
+ * A pointer to a client (thread) access token that requests access rights
+ * of an object or subset of multiple objects.
*
- * @param[in] SubjectSecurityContext
- * The captured subject security context.
+ * @param[in] PrimaryAccessToken
+ * A pointer to a primary access token that describes the primary security
+ * context of the main calling process.
+ *
+ * @param[in] PrincipalSelfSid
+ * A pointer to a security identifier that represents a security principal,
+ * that is, a user object associated with its security descriptor.
*
* @param[in] DesiredAccess
- * Access right bitmask that the calling thread wants to acquire.
+ * The access rights desired by the calling thread to acquire in order to
+ * access an object.
+ *
+ * @param[in] ObjectTypeList
+ * An array list of object types to be checked against for access. The function
+ * will act accordingly in this case by checking each sub-object of an object
+ * of primary level and such. If this parameter is NULL, the function will
+ * perform a normal access check against the target object itself.
*
* @param[in] ObjectTypeListLength
- * The length of a object type list.
+ * The length of a object type list. Such length represents the number of
+ * elements in this list.
*
* @param[in] PreviouslyGrantedAccess
- * The access rights previously acquired in the past.
- *
- * @param[out] Privileges
- * The returned set of privileges.
+ * The access rights previously acquired in the past. If this parameter is 0,
+ * it is deemed that the calling thread hasn't acquired any rights. Access checks
+ * are more tighten in this case.
*
* @param[in] GenericMapping
- * The generic mapping of access rights of an object type.
+ * A pointer to a generic mapping of access rights of the target object.
*
* @param[in] AccessMode
* The processor request level mode.
*
+ * @param[in] UseResultList
+ * If set to TRUE, the function will return a list of granted access rights
+ * of each sub-object as well as status code for each. If this parameter is
+ * set to FALSE, then the function will just return only the granted access
+ * rights and status code for single object that's been target for access
+ * checks.
+ *
+ * @param[out] Privileges
+ * A pointer to a definite set of privileges that have been audited
+ * whilst doing access check procedures. Such set of privileges are
+ * optionally returned to the caller. This can be set to NULL if
+ * the caller doesn't want to obtain a set of privileges.
+ *
* @param[out] GrantedAccessList
- * A list of granted access rights.
+ * A list of granted access rights returned to the caller. This list
+ * can comprehend multiple elements which represent the sub-objects
+ * that have been checked or a single element which is the target
+ * object itself.
*
* @param[out] AccessStatusList
- * The returned status code specifying why access cannot be made
- * onto an object (if said access is denied in the first place).
- *
- * @param[in] UseResultList
- * If set to TRUE, the function will return complete lists of
- * access status codes and granted access rights.
+ * A list of access status codes returned to the caller. This list
+ * can comprehend multiple elements which represent the sub-objects
+ * that have been checked or a single element which is the target
+ * object itself.
*
* @return
* Returns TRUE if access onto the specific object is allowed, FALSE
* otherwise.
- *
- * @remarks
- * The function is currently incomplete!
*/
-BOOLEAN NTAPI
+BOOLEAN
+NTAPI
SepAccessCheck(
_In_ PSECURITY_DESCRIPTOR SecurityDescriptor,
- _In_ PSECURITY_SUBJECT_CONTEXT SubjectSecurityContext,
+ _In_opt_ PACCESS_TOKEN ClientAccessToken,
+ _In_ PACCESS_TOKEN PrimaryAccessToken,
+ _In_opt_ PSID PrincipalSelfSid,
_In_ ACCESS_MASK DesiredAccess,
- _In_ POBJECT_TYPE_LIST ObjectTypeList,
+ _In_opt_ POBJECT_TYPE_LIST ObjectTypeList,
_In_ ULONG ObjectTypeListLength,
_In_ ACCESS_MASK PreviouslyGrantedAccess,
- _Out_ PPRIVILEGE_SET* Privileges,
_In_ PGENERIC_MAPPING GenericMapping,
_In_ KPROCESSOR_MODE AccessMode,
+ _In_ BOOLEAN UseResultList,
+ _Out_opt_ PPRIVILEGE_SET* Privileges,
_Out_ PACCESS_MASK GrantedAccessList,
- _Out_ PNTSTATUS AccessStatusList,
- _In_ BOOLEAN UseResultList)
+ _Out_ PNTSTATUS AccessStatusList)
{
ACCESS_MASK RemainingAccess;
- ACCESS_MASK TempAccess;
- ACCESS_MASK TempGrantedAccess = 0;
- ACCESS_MASK TempDeniedAccess = 0;
+ PACCESS_CHECK_RIGHTS AccessCheckRights;
PACCESS_TOKEN Token;
- ULONG i, ResultListLength;
+ ULONG ResultListLength;
+ ULONG ResultListIndex;
PACL Dacl;
BOOLEAN Present;
BOOLEAN Defaulted;
- PACE CurrentAce;
- PSID Sid;
NTSTATUS Status;
+
PAGED_CODE();
- DPRINT("SepAccessCheck()\n");
+ /* A security descriptor must be expected for access checks */
+ ASSERT(SecurityDescriptor);
+
+ /* Assume no access check rights first */
+ AccessCheckRights = NULL;
/* Check for no access desired */
if (!DesiredAccess)
@@ -103,6 +503,7 @@ SepAccessCheck(
if (!PreviouslyGrantedAccess)
{
/* Then there's nothing to give */
+ DPRINT1("SepAccessCheck(): The caller has no previously granted access gained!\n");
Status = STATUS_ACCESS_DENIED;
goto ReturnCommonStatus;
}
@@ -121,16 +522,35 @@ SepAccessCheck(
/* Initialize remaining access rights */
RemainingAccess = DesiredAccess;
- Token = SubjectSecurityContext->ClientToken ?
- SubjectSecurityContext->ClientToken : SubjectSecurityContext->PrimaryToken;
-
- /* Check for ACCESS_SYSTEM_SECURITY and WRITE_OWNER access */
+ /*
+ * Obtain the token provided by the caller. Client (or also
+ * called impersonation or thread) token takes precedence over
+ * the primary token which is the token associated with the security
+ * context of the main calling process. This is because it is the
+ * client itself that requests access of an object or subset of
+ * multiple objects. Otherwise obtain the security context of the
+ * main process (the actual primary token).
+ */
+ Token = ClientAccessToken ? ClientAccessToken : PrimaryAccessToken;
+
+ /*
+ * We should at least expect a primary token
+ * to be present if client token is not
+ * available.
+ */
+ ASSERT(Token);
+
+ /*
+ * Check for ACCESS_SYSTEM_SECURITY and WRITE_OWNER access.
+ * Write down a set of privileges that have been checked
+ * if the caller wants it.
+ */
Status = SePrivilegePolicyCheck(&RemainingAccess,
&PreviouslyGrantedAccess,
NULL,
Token,
- NULL,
- UserMode);
+ Privileges,
+ AccessMode);
if (!NT_SUCCESS(Status))
{
goto ReturnCommonStatus;
@@ -153,7 +573,7 @@ SepAccessCheck(
goto ReturnCommonStatus;
}
- /* RULE 1: Grant desired access if the object is unprotected */
+ /* Grant desired access if the object is unprotected */
if (Present == FALSE || Dacl == NULL)
{
PreviouslyGrantedAccess |= RemainingAccess;
@@ -176,6 +596,7 @@ SepAccessCheck(
}
else
{
+ DPRINT1("SepAccessCheck(): The DACL has no ACEs and the caller has no previously granted access!\n");
PreviouslyGrantedAccess = 0;
Status = STATUS_ACCESS_DENIED;
}
@@ -185,125 +606,140 @@ SepAccessCheck(
/* Determine the MAXIMUM_ALLOWED access rights according to the DACL */
if (DesiredAccess & MAXIMUM_ALLOWED)
{
- CurrentAce = (PACE)(Dacl + 1);
- for (i = 0; i < Dacl->AceCount; i++)
+ /* 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)
{
- if (!(CurrentAce->Header.AceFlags & INHERIT_ONLY_ACE))
- {
- Sid = (PSID)(CurrentAce + 1);
- if (CurrentAce->Header.AceType == ACCESS_DENIED_ACE_TYPE)
- {
- if (SepSidInToken(Token, Sid))
- {
- /* Map access rights from the ACE */
- TempAccess = CurrentAce->AccessMask;
- RtlMapGenericMask(&TempAccess, GenericMapping);
-
- /* Deny access rights that have not been granted yet */
- TempDeniedAccess |= (TempAccess & ~TempGrantedAccess);
- }
- }
- else if (CurrentAce->Header.AceType == ACCESS_ALLOWED_ACE_TYPE)
- {
- if (SepSidInToken(Token, Sid))
- {
- /* Map access rights from the ACE */
- TempAccess = CurrentAce->AccessMask;
- RtlMapGenericMask(&TempAccess, GenericMapping);
-
- /* Grant access rights that have not been denied yet */
- TempGrantedAccess |= (TempAccess & ~TempDeniedAccess);
- }
- }
- else
- {
- DPRINT1("Unsupported ACE type 0x%lx\n", CurrentAce->Header.AceType);
- }
- }
+ DPRINT1("SepAccessCheck(): Failed to obtain access check rights!\n");
+ Status = STATUS_INSUFFICIENT_RESOURCES;
+ PreviouslyGrantedAccess = 0;
+ goto ReturnCommonStatus;
+ }
- /* Get the next ACE */
- CurrentAce = (PACE)((ULONG_PTR)CurrentAce + CurrentAce->Header.AceSize);
+ /*
+ * Perform further access checks if this token
+ * has restricted SIDs.
+ */
+ if (SeTokenIsRestricted(Token))
+ {
+ AccessCheckRights = SepAnalyzeAcesFromDacl(AccessCheckMaximum,
+ Dacl,
+ Token,
+ PrimaryAccessToken,
+ TRUE,
+ TRUE,
+ PrincipalSelfSid,
+ GenericMapping,
+ ObjectTypeList,
+ ObjectTypeListLength,
+ 0);
}
/* Fail if some rights have not been granted */
- RemainingAccess &= ~(MAXIMUM_ALLOWED | TempGrantedAccess);
+ RemainingAccess &= ~(MAXIMUM_ALLOWED | AccessCheckRights->GrantedAccessRights);
if (RemainingAccess != 0)
{
+ DPRINT1("SepAccessCheck(): Failed to grant access rights. RemainingAccess = 0x%08lx DesiredAccess = 0x%08lx\n", RemainingAccess, DesiredAccess);
PreviouslyGrantedAccess = 0;
Status = STATUS_ACCESS_DENIED;
goto ReturnCommonStatus;
}
/* Set granted access right and access status */
- PreviouslyGrantedAccess |= TempGrantedAccess;
+ PreviouslyGrantedAccess |= AccessCheckRights->GrantedAccessRights;
if (PreviouslyGrantedAccess != 0)
{
Status = STATUS_SUCCESS;
}
else
{
+ DPRINT1("SepAccessCheck(): Failed to grant access rights. PreviouslyGrantedAccess == 0 DesiredAccess = %08lx\n", DesiredAccess);
Status = STATUS_ACCESS_DENIED;
}
+
+ /* We have successfully granted all the rights */
goto ReturnCommonStatus;
}
- /* RULE 4: Grant rights according to the DACL */
- CurrentAce = (PACE)(Dacl + 1);
- for (i = 0; i < Dacl->AceCount; i++)
+ /* 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)
{
- if (!(CurrentAce->Header.AceFlags & INHERIT_ONLY_ACE))
- {
- Sid = (PSID)(CurrentAce + 1);
- if (CurrentAce->Header.AceType == ACCESS_DENIED_ACE_TYPE)
- {
- if (SepSidInToken(Token, Sid))
- {
- /* Map access rights from the ACE */
- TempAccess = CurrentAce->AccessMask;
- RtlMapGenericMask(&TempAccess, GenericMapping);
-
- /* Leave if a remaining right must be denied */
- if (RemainingAccess & TempAccess)
- break;
- }
- }
- else if (CurrentAce->Header.AceType == ACCESS_ALLOWED_ACE_TYPE)
- {
- if (SepSidInToken(Token, Sid))
- {
- /* Map access rights from the ACE */
- TempAccess = CurrentAce->AccessMask;
- DPRINT("TempAccess 0x%08lx\n", TempAccess);
- RtlMapGenericMask(&TempAccess, GenericMapping);
-
- /* Remove granted rights */
- DPRINT("RemainingAccess 0x%08lx TempAccess 0x%08lx\n", RemainingAccess, TempAccess);
- RemainingAccess &= ~TempAccess;
- DPRINT("RemainingAccess 0x%08lx\n", RemainingAccess);
- }
- }
- else
- {
- DPRINT1("Unsupported ACE type 0x%lx\n", CurrentAce->Header.AceType);
- }
- }
-
- /* Get the next ACE */
- CurrentAce = (PACE)((ULONG_PTR)CurrentAce + CurrentAce->Header.AceSize);
+ DPRINT1("SepAccessCheck(): Failed to obtain access check rights!\n");
+ Status = STATUS_INSUFFICIENT_RESOURCES;
+ PreviouslyGrantedAccess = 0;
+ goto ReturnCommonStatus;
}
- DPRINT("DesiredAccess %08lx\nPreviouslyGrantedAccess %08lx\nRemainingAccess %08lx\n",
- DesiredAccess, PreviouslyGrantedAccess, RemainingAccess);
-
/* Fail if some rights have not been granted */
- if (RemainingAccess != 0)
+ if (AccessCheckRights->RemainingAccessRights != 0)
{
- DPRINT("HACK: RemainingAccess = 0x%08lx DesiredAccess = 0x%08lx\n", RemainingAccess, DesiredAccess);
-#if 0
- /* HACK HACK HACK */
+ 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;
-#endif
+ }
+
+ /*
+ * Perform further access checks if this token
+ * has restricted SIDs.
+ */
+ if (SeTokenIsRestricted(Token))
+ {
+ AccessCheckRights = SepAnalyzeAcesFromDacl(AccessCheckRegular,
+ Dacl,
+ Token,
+ PrimaryAccessToken,
+ TRUE,
+ TRUE,
+ PrincipalSelfSid,
+ GenericMapping,
+ ObjectTypeList,
+ ObjectTypeListLength,
+ RemainingAccess);
+
+ /* Fail if some rights have not been granted */
+ if (AccessCheckRights->RemainingAccessRights != 0)
+ {
+ 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;
+ }
}
/* Set granted access rights */
@@ -312,22 +748,29 @@ SepAccessCheck(
/* Fail if no rights have been granted */
if (PreviouslyGrantedAccess == 0)
{
- DPRINT1("PreviouslyGrantedAccess == 0 DesiredAccess = %08lx\n", DesiredAccess);
+ DPRINT1("SepAccessCheck(): Failed to grant access rights. PreviouslyGrantedAccess == 0 DesiredAccess = %08lx\n", DesiredAccess);
Status = STATUS_ACCESS_DENIED;
goto ReturnCommonStatus;
}
+ /*
+ * If we're here then we granted all the desired
+ * access rights the caller wanted.
+ */
Status = STATUS_SUCCESS;
- goto ReturnCommonStatus;
ReturnCommonStatus:
ResultListLength = UseResultList ? ObjectTypeListLength : 1;
- for (i = 0; i < ResultListLength; i++)
+ for (ResultListIndex = 0; ResultListIndex < ResultListLength; ResultListIndex++)
{
- GrantedAccessList[i] = PreviouslyGrantedAccess;
- AccessStatusList[i] = Status;
+ GrantedAccessList[ResultListIndex] = PreviouslyGrantedAccess;
+ AccessStatusList[ResultListIndex] = Status;
}
+ /* Free the allocated access check rights */
+ SepFreeAccessCheckRights(AccessCheckRights);
+ AccessCheckRights = NULL;
+
return NT_SUCCESS(Status);
}
@@ -425,8 +868,9 @@ SepGetPrivilegeSetLength(
* The captured subject security context.
*
* @param[in] SubjectContextLocked
- * If set to TRUE, a lock must be acquired for the security subject
- * context.
+ * If set to TRUE, the caller acknowledges that the subject context
+ * has already been locked by the caller himself. If set to FALSE,
+ * the function locks the subject context.
*
* @param[in] DesiredAccess
* Access right bitmask that the calling thread wants to acquire.
@@ -552,17 +996,19 @@ SeAccessCheck(
{
/* Call the internal function */
ret = SepAccessCheck(SecurityDescriptor,
- SubjectSecurityContext,
+ SubjectSecurityContext->ClientToken,
+ SubjectSecurityContext->PrimaryToken,
+ NULL,
DesiredAccess,
NULL,
0,
PreviouslyGrantedAccess,
- Privileges,
GenericMapping,
AccessMode,
+ FALSE,
+ Privileges,
GrantedAccess,
- AccessStatus,
- FALSE);
+ AccessStatus);
}
/* Release the lock if needed */
@@ -726,6 +1172,7 @@ NtAccessCheck(
ULONG CapturedPrivilegeSetLength, RequiredPrivilegeSetLength;
PTOKEN Token;
NTSTATUS Status;
+
PAGED_CODE();
/* Check if this is kernel mode */
@@ -922,17 +1369,19 @@ NtAccessCheck(
{
/* Now perform the access check */
SepAccessCheck(CapturedSecurityDescriptor,
- &SubjectSecurityContext,
+ Token,
+ &SubjectSecurityContext.PrimaryToken,
+ NULL,
DesiredAccess,
NULL,
0,
PreviouslyGrantedAccess,
- &PrivilegeSet, //FIXME
GenericMapping,
PreviousMode,
+ FALSE,
+ NULL,
GrantedAccess,
- AccessStatus,
- FALSE);
+ AccessStatus);
}
/* Release subject context and unlock the token */
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=f48191b4b5b2f5d5498ff…
commit f48191b4b5b2f5d5498ff77a651d062a59dc546b
Author: George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Sat Feb 5 22:21:14 2022 +0100
Commit: George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Fri May 6 10:09:53 2022 +0200
[NTOS:SE] Enable support for principal and restricted SIDs
SepSidInTokenEx function already provides the necessary mechanism to handle scenario where a token has restricted SIDs or a principal SID is given to the call. There's no reason to have these redundant ASSERTs anymore.
In addition to that make sure if the SID is not a restricted and if that SID is the first element on the array and it's enabled, this is the primary user.
---
ntoskrnl/se/access.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/ntoskrnl/se/access.c b/ntoskrnl/se/access.c
index 7e6eb23d136..98b00a70d1e 100644
--- a/ntoskrnl/se/access.c
+++ b/ntoskrnl/se/access.c
@@ -37,7 +37,7 @@ ERESOURCE SepSubjectContextLock;
*
* @param[in] Restricted
* If set to TRUE, the caller expects that a SID in a token is
- * restricted.
+ * restricted (by the general definition, a token is restricted).
*
* @return
* Returns TRUE if the specified SID in the call is present in the token,
@@ -52,7 +52,7 @@ SepSidInTokenEx(
_In_ BOOLEAN Deny,
_In_ BOOLEAN Restricted)
{
- ULONG i;
+ ULONG SidIndex;
PTOKEN Token = (PTOKEN)_Token;
PISID TokenSid, Sid = (PISID)_Sid;
PSID_AND_ATTRIBUTES SidAndAttributes;
@@ -60,10 +60,6 @@ SepSidInTokenEx(
USHORT SidMetadata;
PAGED_CODE();
- /* Not yet supported */
- ASSERT(PrincipalSelfSid == NULL);
- ASSERT(Restricted == FALSE);
-
/* Check if a principal SID was given, and this is our current SID already */
if ((PrincipalSelfSid) && (RtlEqualSid(SePrincipalSelfSid, Sid)))
{
@@ -91,7 +87,7 @@ SepSidInTokenEx(
SidMetadata = *(PUSHORT)&Sid->Revision;
/* Loop every SID */
- for (i = 0; i < SidCount; i++)
+ for (SidIndex = 0; SidIndex < SidCount; SidIndex++)
{
TokenSid = (PISID)SidAndAttributes->Sid;
#if SE_SID_DEBUG
@@ -106,8 +102,15 @@ SepSidInTokenEx(
/* Check if the SID data matches */
if (RtlEqualMemory(Sid, TokenSid, SidLength))
{
- /* Check if the group is enabled, or used for deny only */
- if ((!(i) && !(SidAndAttributes->Attributes & SE_GROUP_USE_FOR_DENY_ONLY)) ||
+ /*
+ * Check if the group is enabled, or used for deny only.
+ * Otherwise we have to check if this is the first user.
+ * We understand that by looking if this SID is not
+ * restricted, this is the first element we are iterating
+ * and that it doesn't have SE_GROUP_USE_FOR_DENY_ONLY
+ * attribute.
+ */
+ if ((!Restricted && (SidIndex == 0) && !(SidAndAttributes->Attributes & SE_GROUP_USE_FOR_DENY_ONLY)) ||
(SidAndAttributes->Attributes & SE_GROUP_ENABLED) ||
((Deny) && (SidAndAttributes->Attributes & SE_GROUP_USE_FOR_DENY_ONLY)))
{
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=bac67a65f26df384f5962…
commit bac67a65f26df384f5962e85f001f5984caa2b66
Author: George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Sat Feb 5 22:01:39 2022 +0100
Commit: George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Fri May 6 10:09:53 2022 +0200
[NTOS:SE] Implement SepGetSidFromAce
This function will be used to retrieve a security identifier from a valid access control entry in the kernel. Mostly and exclusively used within access checks related code and such.
---
ntoskrnl/se/sid.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 71 insertions(+)
diff --git a/ntoskrnl/se/sid.c b/ntoskrnl/se/sid.c
index 509b3777488..13aeba2662b 100644
--- a/ntoskrnl/se/sid.c
+++ b/ntoskrnl/se/sid.c
@@ -412,6 +412,77 @@ SepReleaseSid(
}
}
+/**
+ * @brief
+ * Captures a security identifier from a
+ * given access control entry. This identifier
+ * is valid for the whole of its lifetime.
+ *
+ * @param[in] AceType
+ * The type of an access control entry. This
+ * type that is given by the calling thread
+ * must coincide with the actual ACE that is
+ * given in the second parameter otherwise this
+ * can potentially lead to UNDEFINED behavior!
+ *
+ * @param[in] Ace
+ * A pointer to an access control entry, which
+ * can be obtained from a DACL.
+ *
+ * @return
+ * Returns a pointer to a security identifier (SID),
+ * otherwise NULL is returned if an unsupported ACE
+ * type was given to the function.
+ */
+PSID
+NTAPI
+SepGetSidFromAce(
+ _In_ UCHAR AceType,
+ _In_ PACE Ace)
+{
+ PSID Sid;
+ PAGED_CODE();
+
+ /* Sanity check */
+ ASSERT(Ace);
+
+ /* Initialize the SID */
+ Sid = NULL;
+
+ /* Obtain the SID based upon ACE type */
+ switch (AceType)
+ {
+ case ACCESS_DENIED_ACE_TYPE:
+ {
+ Sid = (PSID)&((PACCESS_DENIED_ACE)Ace)->SidStart;
+ break;
+ }
+
+ case ACCESS_ALLOWED_ACE_TYPE:
+ {
+ Sid = (PSID)&((PACCESS_ALLOWED_ACE)Ace)->SidStart;
+ break;
+ }
+
+ case ACCESS_DENIED_OBJECT_ACE_TYPE:
+ {
+ Sid = (PSID)&((PACCESS_DENIED_OBJECT_ACE)Ace)->SidStart;
+ break;
+ }
+
+ case ACCESS_ALLOWED_OBJECT_ACE_TYPE:
+ {
+ Sid = (PSID)&((PACCESS_ALLOWED_OBJECT_ACE)Ace)->SidStart;
+ break;
+ }
+
+ default:
+ break;
+ }
+
+ return Sid;
+}
+
/**
* @brief
* Captures a SID with attributes.
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=f559f63063a64aba3f489…
commit f559f63063a64aba3f489aef210a9848a0850d48
Author: George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Sun Feb 13 19:12:19 2022 +0100
Commit: George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Fri May 6 10:09:52 2022 +0200
[SERVICES] Assign a World identity authority for Everyone SID, not Null authority
The current code allocates memory and initializes the Everyone "World" security identifier but with a Null authority identifier. This is utterly wrong on so many levels, more so partly because a Null authority identifier is 0 so after the Everyone SID is initialized, it is actually initialized as S-1-0-0 instead of S-1-1-0.
---
base/system/services/security.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/base/system/services/security.c b/base/system/services/security.c
index b2639e95a20..2e8f284a714 100644
--- a/base/system/services/security.c
+++ b/base/system/services/security.c
@@ -55,6 +55,7 @@ DWORD
ScmCreateSids(VOID)
{
SID_IDENTIFIER_AUTHORITY NullAuthority = {SECURITY_NULL_SID_AUTHORITY};
+ SID_IDENTIFIER_AUTHORITY WorldAuthority = {SECURITY_WORLD_SID_AUTHORITY};
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
PULONG pSubAuthority;
ULONG ulLength1 = RtlLengthRequiredSid(1);
@@ -78,7 +79,7 @@ ScmCreateSids(VOID)
return ERROR_OUTOFMEMORY;
}
- RtlInitializeSid(pWorldSid, &NullAuthority, 1);
+ RtlInitializeSid(pWorldSid, &WorldAuthority, 1);
pSubAuthority = RtlSubAuthoritySid(pWorldSid, 0);
*pSubAuthority = SECURITY_WORLD_RID;