[NTOS:SE] Access Check Overhaul -- Deny access to the caller if not permitted #4340

GeoB99 · 2022-02-05T22:20:40Z

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 rights based on the ACE policy that represents the same SID.

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

Once the foundation of access checks is properly solid by getting rid of the ugly hack present in the code, this allows for more opportunity to continue further on implementing object type access checks, compound ACE handling, and whatnot. For now we'll live by the basics.

TODO

Make ReactOS bootable again
Investigate most of the "failed to grant access rights" occurrences on ReactOS

JIRA Issue: CORE-9184

GeoB99 · 2022-02-05T22:22:40Z

And yes in case it's not quite obvious for someone, the fact that ROS is now unbootable and lots of failed attempts to warrant rights occur is just a clear message to me that security rights in general have been neglected in ROS for years. Therefore...

Implement a base security infrastructure with code that sets up a security descriptor for the service that we're going to connect through it. Such service is based upon a desktop and a window station. === DOCUMENTATION REMARKS === The authenticated user, represented by an access token that describes its security context, is the main holder and has ultimate power against the default created desktop and window station objects in USER. The authenticated user in question is the actual logged in user, this is the case when the server is impersonating a client. Administrators on the other hand have some share of power against default desktop but their power in question is extremely limited against the default window station as admins can only just enumerate the available and valid handle stations within a desktop.

…g a window station When creating a window station with CreateWindowStationW, the function ignores the security descriptor provided by the caller and instead it uses whatever descriptor the system can find.

Refactor the security related code of Winlogon and move it to its own dedicated place, security.c. This includes code for preparation of security descriptors for window station and desktop objects when their created, helper functions which give allow access to such objects for the logged in user and whatnot. ==== DO NOTE ==== Currently new desktop security assignment fails because for whatever reason the system thinks the application desktop has no prior security even though a descriptor has been created for it before. See the FIXME comment on code for information.

HHHHHHHHHHAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACCCCCCCCCCCCCCCCCCCCCCCKKKKKKKKKKKKKKKKKK!!! There are two problems concerning with network services. First, a window station should be created for every network service process that gets started although this doesn't happen. Instead, network services like RPCSS and DNS service host process (svchost.exe) attempt to access the default window station (Winsta0). This is because the access token of these two network service processes have an authentication ID that is uniquely generated. This is incorrect, because NetworkService is a special account with its own designed authentication ID for it. As a matter of fact, no window station is created for a network service and as such both RPCSS and DNS svchost.exe attempt to access Winsta0 which they cannot. The second problem, albeit not quite relevant to the first one but still worth mentioning nevertheless, is that network services have an access token that is primary which it should be an impersonation token. These problems all come from LSASS as LSA infrastructure is responsible for creating access tokens with security context for objects. For the moment being, add a hack on Winlogon that gives allow access to the default window station to network services. When LSASS and involved components are fixed, this hack must be removed.

Implement code that deals with the security side of NLS, more specifically, create two security descriptors for NLS directory and NLS section names and let the server use such code.

Currently Kernel32 doesn't make any server call to Basesrv in order to create NLS section names, instead it's Kernel32 itself that handles the job of NLS section names. With that said, let Kernel32 assign a security descriptor to NLS section names. See the FIXME comment on code for further dtails

Refactor the function in such a way that it can jump to a single exit but most importantly, implement a "rinse and repeat" mechanism where we assign a primary token to process by disabling impersonation first and retry with impersonation later. More info can be found in the documention within the code.

…sUserCommon internal function Currently CreateProcessAsUserCommon doesn't set a default descriptor for the newly duplicated token object for the new process nor it sets any security information for both the process and thread. This is wrong, because when the process is created on behalf of the user's security context, it still uses the previous security information of the creator that initially gave birth to the process. CreateDefaultProcessSecurityCommon function will serve as a placeholder until CreatePrivateObjectSecurity is implemented.

rpcrt4_create_pipe_security function will be held in charge to set up security descriptors specific for each named pipe upon creation in rpcrt4_conn_create_pipe. The descriptor is then freed after the pipe is no longer needed.

…vent

ReactOS Setup is an integral component that is part of the operating system responsible for the installation of ROS during 2nd installation stage. The situation with current master branch is like this -- the Services component always tries to create the process on behalf of the logged in user with its own security context. That user doesn't have the privileges and access rights like SYSTEM thus the Services component tries to create the process but it fails to do so because of lacking of required access right, TOKEN_DUPLICATE, in order for the calling thread to impersonate as self.

…ll 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.

ACCESS_ALLOWED_OBJECT_ACE and ACCESS_DENIED_OBJECT_ACE structures must be in the XDK section of SDK as these will be used in the future in the security subsystem of the kernel.

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.

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.

…bject 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 rights based on the ACE policy that represents the same SID. 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

…ection security. Partially revert some aspects of commits 5696e4b and bf40c7a. (See PR reactos#4340.) In order for Win2k3 kernel32.dll to operate with our basesrv.dll (or our kernel32.dll to operate with Win2k3 basesrv.dll), we need in particular to have the CreateNlsSecurityDescriptor() helper to exactly take the expected parameters. Namely, a pointer to a **user-allocated** SECURITY_DESCRIPTOR buffer, its size (and an access mask). The function expects its caller to provide all this, and the caller expects the function to initialize the security descriptor buffer. Note that the function does *NOT* allocate a new descriptor buffer to be returned! Indeed, with the way it currently is in master, using Win2k3 kernel32 with our basesrv is now failing with the errors: ``` NLSAPI: Could NOT Create ACL - c0000023. (subsystems/win/basesrv/nls.c:279) NLS: CreateNlsSecurityDescriptor FAILED!: c0000023 NLSAPI: Could NOT initialize Server - c0000023. (dll/ntdll/ldr/ldrinit.c:867) LDR: DLL_PROCESS_ATTACH for dll "kernel32.dll" (InitRoutine: 77E40D95) failed ``` (and, if we ever attempted to increase the so-claimed "dummy parameter" descriptor size in the basesrv call, we would end up with its stack corrupted and a crash). Conversely, using our kernel32 with Win2k3 basesrv, would end up with basesrv receiving a wrongly-initialized descriptor that would not work (the buffer not being initialized with the contents of a descriptor, but instead receiving some address to a descriptor allocated somewhere else).

…ection security. (reactos#4828) Partially revert some aspects of commits 5696e4b and bf40c7a. (See PR reactos#4340.) In order for Win2k3 kernel32.dll to operate with our basesrv.dll (or our kernel32.dll to operate with Win2k3 basesrv.dll), we need in particular to have the CreateNlsSecurityDescriptor() helper to exactly take the expected parameters. Namely, a pointer to a **user-allocated** SECURITY_DESCRIPTOR buffer, its size (and an access mask). The function expects its caller to provide all this, and the caller expects the function to initialize the security descriptor buffer. Note that the function does *NOT* allocate a new descriptor buffer to be returned! Indeed, with the way it currently is in master, using Win2k3 kernel32 with our basesrv is now failing with the errors: ``` NLSAPI: Could NOT Create ACL - c0000023. (subsystems/win/basesrv/nls.c:279) NLS: CreateNlsSecurityDescriptor FAILED!: c0000023 NLSAPI: Could NOT initialize Server - c0000023. (dll/ntdll/ldr/ldrinit.c:867) LDR: DLL_PROCESS_ATTACH for dll "kernel32.dll" (InitRoutine: 77E40D95) failed ``` (and, if we ever attempted to increase the so-claimed "dummy parameter" descriptor size in the basesrv call, we would end up with its stack corrupted and a crash). Conversely, using our kernel32 with Win2k3 basesrv, would end up with basesrv receiving a wrongly-initialized descriptor that would not work (the buffer not being initialized with the contents of a descriptor, but instead receiving some address to a descriptor allocated somewhere else).

…ction security. (reactos#4828) Partially revert some aspects of commits 5696e4b and bf40c7a. (See PR reactos#4340.) In order for Win2k3 kernel32.dll to operate with our basesrv.dll (or our kernel32.dll to operate with Win2k3 basesrv.dll), we need in particular to have the CreateNlsSecurityDescriptor() helper to exactly take the expected parameters. Namely, a pointer to a **user-allocated** SECURITY_DESCRIPTOR buffer, its size (and an access mask). The function expects its caller to provide all this, and the caller expects the function to initialize the security descriptor buffer. Note that the function does *NOT* allocate a new descriptor buffer to be returned! Indeed, with the way it currently is in master, using Win2k3 kernel32 with our basesrv is now failing with the errors: ``` NLSAPI: Could NOT Create ACL - c0000023. (subsystems/win/basesrv/nls.c:279) NLS: CreateNlsSecurityDescriptor FAILED!: c0000023 NLSAPI: Could NOT initialize Server - c0000023. (dll/ntdll/ldr/ldrinit.c:867) LDR: DLL_PROCESS_ATTACH for dll "kernel32.dll" (InitRoutine: 77E40D95) failed ``` (and, if we ever attempted to increase the so-claimed "dummy parameter" descriptor size in the basesrv call, we would end up with its stack corrupted and a crash). Conversely, using our kernel32 with Win2k3 basesrv, would end up with basesrv receiving a wrongly-initialized descriptor that would not work (the buffer not being initialized with the contents of a descriptor, but instead receiving some address to a descriptor allocated somewhere else).

…ction security. (#4828) Partially revert some aspects of commits 5696e4b and bf40c7a. (See PR #4340.) In order for Win2k3 kernel32.dll to operate with our basesrv.dll (or our kernel32.dll to operate with Win2k3 basesrv.dll), we need in particular to have the CreateNlsSecurityDescriptor() helper to exactly take the expected parameters. Namely, a pointer to a **user-allocated** SECURITY_DESCRIPTOR buffer, its size (and an access mask). The function expects its caller to provide all this, and the caller expects the function to initialize the security descriptor buffer. Note that the function does *NOT* allocate a new descriptor buffer to be returned! Indeed, with the way it currently is in master, using Win2k3 kernel32 with our basesrv is now failing with the errors: ``` NLSAPI: Could NOT Create ACL - c0000023. (subsystems/win/basesrv/nls.c:279) NLS: CreateNlsSecurityDescriptor FAILED!: c0000023 NLSAPI: Could NOT initialize Server - c0000023. (dll/ntdll/ldr/ldrinit.c:867) LDR: DLL_PROCESS_ATTACH for dll "kernel32.dll" (InitRoutine: 77E40D95) failed ``` (and, if we ever attempted to increase the so-claimed "dummy parameter" descriptor size in the basesrv call, we would end up with its stack corrupted and a crash). Conversely, using our kernel32 with Win2k3 basesrv, would end up with basesrv receiving a wrongly-initialized descriptor that would not work (the buffer not being initialized with the contents of a descriptor, but instead receiving some address to a descriptor allocated somewhere else).

GeoB99 added enhancement For PRs with an enhancement/new feature. bugfix For bugfix PRs. refactoring For refactoring changes. kernel&hal Code changes to the ntoskrnl and HAL labels Feb 5, 2022

GeoB99 self-assigned this Feb 5, 2022

GeoB99 added this to New PRs in ReactOS PRs via automation Feb 5, 2022

GeoB99 force-pushed the se-accesschk branch 3 times, most recently from bab7c27 to c818c60 Compare February 13, 2022 18:59

GeoB99 force-pushed the se-accesschk branch 9 times, most recently from 66d6154 to 91d7cac Compare February 21, 2022 10:28

GeoB99 force-pushed the se-accesschk branch 5 times, most recently from 977f2ca to c8cdc89 Compare March 2, 2022 20:55

GeoB99 force-pushed the se-accesschk branch 4 times, most recently from 2fef34d to 95aaa11 Compare March 16, 2022 20:18

GeoB99 force-pushed the se-accesschk branch 2 times, most recently from 082b972 to fb775ce Compare March 20, 2022 19:13

GeoB99 added 20 commits May 6, 2022 10:09

[USER32] Capture the security descriptor from the caller when creatin…

65a72fb

…g a window station When creating a window station with CreateWindowStationW, the function ignores the security descriptor provided by the caller and instead it uses whatever descriptor the system can find.

[KERNEL32][BASESRV] Implement NLS section security

5696e4b

Implement code that deals with the security side of NLS, more specifically, create two security descriptors for NLS directory and NLS section names and let the server use such code.

[DHCPCSVC] Set up a security descriptor for DHCP named pipe

eccae20

[RPCRT4] Set up a security descriptor for RPC named pipes

56a2c0f

rpcrt4_create_pipe_security function will be held in charge to set up security descriptors specific for each named pipe upon creation in rpcrt4_conn_create_pipe. The descriptor is then freed after the pipe is no longer needed.

[POWRPROF] Create a security descriptor for power management semaphore

fd25e2d

[UMPNPMGR] Create a security descriptor for PnP installation device e…

cd1070d

…vent

[NTOSKRNL] Add security access check rights pool tag

9101a5d

[XDK] Move security object related structures to appropriate place

bcf0aeb

ACCESS_ALLOWED_OBJECT_ACE and ACCESS_DENIED_OBJECT_ACE structures must be in the XDK section of SDK as these will be used in the future in the security subsystem of the kernel.

[NTOS:SE] Add SepGetSidFromAce prototype & Niscellaneous Stuff

c93bf84

[NTOS:SE] Implement SepGetSidFromAce

bac67a6

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.

GeoB99 force-pushed the se-accesschk branch from 700f17c to 55c117c Compare May 6, 2022 08:10

GeoB99 merged commit 55c117c into reactos:master May 6, 2022

ReactOS PRs automation moved this from New PRs to Done May 6, 2022

GeoB99 deleted the se-accesschk branch May 6, 2022 09:37

HBelusca mentioned this pull request Oct 30, 2022

[KERNEL32][BASESRV] Fix interoperability with Win2k3 regarding NLS section security. #4828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NTOS:SE] Access Check Overhaul -- Deny access to the caller if not permitted #4340

[NTOS:SE] Access Check Overhaul -- Deny access to the caller if not permitted #4340

GeoB99 commented Feb 5, 2022 •

edited

GeoB99 commented Feb 5, 2022

[NTOS:SE] Access Check Overhaul -- Deny access to the caller if not permitted #4340

[NTOS:SE] Access Check Overhaul -- Deny access to the caller if not permitted #4340

Conversation

GeoB99 commented Feb 5, 2022 • edited

TODO

GeoB99 commented Feb 5, 2022

GeoB99 commented Feb 5, 2022 •

edited