The "failed to grant access rights" message isn't enough to understand what kind of access rights haven't been granted and why. Dumping information of the captured security descriptor, the ACL and its ACEs with mask rights and token SIDs should be enough to understand the reason of the failure in question.
debug.c will serve as a centralized facility for security debugging routines and everything related to that. This file will be expanded with further debug functions for the Security subsystem if needed.
Not only primary group assignation was broken but new dynamic length calculation is also broken. The length of the captured SID is not taken into account so the new dynamic length gets only the size of the default ACL present in an access token.
Therefore, the condition is always FALSE and the code never jumps to the STATUS_ALLOTTED_SPACE_EXCEEDED branch because the length will always be small than the charged dynamic length.
Addendum to 86bde3c.
With current master, what happens is that when someone wants to assign a new primary group SID for an access token, it results in an instant page fault because the primary group variable doesn't get assigned the dynamic part's address.
So the primary group variable gets an address which is basically a representation of the ACL size, hence the said address is bogus and it's where the page fault kicks in.
CORE-18249
On current master, ReactOS faces these problems:
- ObCreateObject charges both paged and non paged pool a size of TOKEN structure, not the actual dynamic contents of WHAT IS inside a token. For paged pool charge the size is that of the dynamic area (primary group + default DACL if any). This is basically what DynamicCharged is for.
For the non paged pool charge, the actual charge is that of TOKEN structure upon creation. On duplication and filtering however, the paged pool charge size is that of the inherited dynamic charged space from an existing token whereas the non paged pool size is that of the calculated token body
length for the new duplicated/filtered token. On current master, we're literally cheating the kernel by charging the wrong amount of quota not taking into account the dynamic contents which they come from UM.
- Both DynamicCharged and DynamicAvailable are not fully handled (DynamicAvailable is pretty much poorly handled with some cases still to be taking into account). DynamicCharged is barely handled, like at all.
- As a result of these two points above, NtSetInformationToken doesn't check when the caller wants to set up a new default token DACL or primary group if the newly DACL or the said group exceeds the dynamic charged boundary. So what happens is that I'm going to act like a smug bastard fat politician and whack
the primary group and DACL of an token however I want to, because why in the hell not? In reality no, the kernel has to punish whoever attempts to do that, although we currently don't.
- The dynamic area (aka DynamicPart) only picks up the default DACL but not the primary group as well. Generally the dynamic part is composed of primary group and default DACL, if provided.
In addition to that, we aren't returning the dynamic charged and available area in token statistics. SepComputeAvailableDynamicSpace helper is here to accommodate that. Apparently Windows is calculating the dynamic available area rather than just querying the DynamicAvailable field directly from the token.
My theory regarding this is like the following: on Windows both TokenDefaultDacl and TokenPrimaryGroup classes are barely used by the system components during startup (LSASS provides both a DACL and primary group when calling NtCreateToken anyway). In fact DynamicAvailable is 0 during token creation, duplication and filtering when inspecting a token with WinDBG. So
if an application wants to query token statistics that application will face a dynamic available space of 0.
TokenGroupsAndPrivileges is the younger sister of two TokenGroups and TokenPrivileges classes. In its purpose there's no huge substantial differences apart that this class comes with its own structure, TOKEN_GROUPS_AND_PRIVILEGES, and that this structure comes with extra information.
In NtQueryInformationToken function, remove the useless and redundant NULL check for two primary reasons. First, DefaultQueryInfoBufferCheck already does the necessary probing validation checks and second, ReturnLength must NEVER be NULL!
If the caller does not respect the calling rules of NtQueryInformationToken, the caller is expected to be miserably punished.
NtQueryInformationToken is by far the only system call in NT where ReturnLength simply cannot be optional. On Windows this parameter is always probed and an argument to NULL directly leads to an access violation exception.
This is due to the fact of how tokens work, as its information contents (token user, owner, primary group, et al) are dynamic and can vary throughout over time in memory.
What happens on current ReactOS master however is that ReturnLength is only probed if the parameter is not NULL. On a NULL case scenario the probing checks succeed and NtQueryInformationToken fails later. For this, just get rid of CompleteProbing
parameter and opt in for a bit mask flag based approach, with ICIF_FORCE_RETURN_LENGTH_PROBE being set on DefaultQueryInfoBufferCheck which NtQueryInformationToken calls it to do sanity checks.
In addition to that...
- Document the ICIF probe helpers
- Annotate the ICIF prope helpers with SAL
- With the riddance of CompleteProbing and adoption of flags based approach, add ICIF_PROBE_READ_WRITE and ICIF_PROBE_READ flags alongside with ICIF_FORCE_RETURN_LENGTH_PROBE
The current state of Security manager's code is kind of a mess. Mainly, there's code scattered around places where they shouldn't belong and token implementation (token.c) is already of a bloat in itself as it is. The file has over 6k lines and it's subject to grow exponentially with improvements, features, whatever that is.
With that being said, the token implementation code in the kernel will be split accordingly and rest of the code moved to appropriate places. The new layout will look as follows (excluding the already existing files):
- client.c (Client security implementation code)
- objtype.c (Object type list implementation code -- more code related to object types will be put here when I'm going to implement object type access checks in the future)
- subject.c (Subject security context support)
The token implementation in the kernel will be split in 4 distinct files as shown:
- token.c (Base token support routines)
- tokenlif.c (Life management of a token object -- that is Duplication, Creation and Filtering)
- tokencls.c (Token Query/Set Information Classes support)
- tokenadj.c (Token privileges/groups adjusting support)
In addition to that, tidy up the internal header and reorganize it as well.
The problem is that EndMem is changed to point to the DynamicPart of
the token, but the code after that expects it to still point into the
VariablePart instead.
Problem fixed by moving the insertion of RestrictedSids much sooner
(where the original ones are also being copied).
Shared locking must be used on the source token as it is accessed for
reading only. This fixes in particular the kmtest SeTokenFiltering that
would hang otherwise on a (wrong) exclusive locking.
- SepPerformTokenFiltering(): Always shared-lock the source token.
Its callers (NtFilterToken and SeFilterToken) just need to sanitize and
put the parameters in correct form before calling this helper function.
- Sync comments in NtFilterToken() with SeFilterToken().
This function is either called inter-kernel (in which case, all
parameters must be valid, and if not, we have to bugcheck), or, it
is called with **captured** parameters (from NtFilterToken) and those
latter ones are now expected to be valid and reside in kernel-mode.
Finally, data copied between token structures reside in kernel-mode
only and again are expected to be valid (if not, we bugcheck).
- The ACL is however not validated when the function is called within
kernel mode and no capture is actually being done.
- Simplify aspects of the function (returning early when possible).
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
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.
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.
Implement initial token debug code. For now debug information that is being tracked are: process image file name, process and thread client IDs and token creation method. More specific debug code can be added later only if needed.
As for the token creation method, this follows the same principle as on Windows where the creation method is defined by a value denoting the first letter of the said method of creation. That is, 0xC is for token creation, 0xD is for token duplication and 0xF is for token filtering. The debug field names are taken from Windows PDB symbols for WinDBG debug extension support purposes. The names must not be changed!
This reverts 8479509 commit which pretty much does nothing at all (the captured pointer is NULL within the stack of the function has no effect outside of the function). My mistake, sorry.
Whenever a captured security property such as privilege or SID is released, we must not have such captured property point at random address in memory but rather we must assign it as NULL after it's been freed from pool memory. This avoids potential double-after-free situations where we might release a buffer twice.
This is exactly the case with token filtering.
SIDs are variadic by nature which means their lengths can vary in a given amount of time and certain factors that allow for this happen. This also especially can lead to issues when capturing SIDs and attributes because SeCaptureSidAndAttributesArray might end up overwriting the buffer during the time it's been called.
Therefore when we're copying the SIDs, validate their lengths. In addition to that, update the documentation header accordingly and add some debug prints in code.
This implements the support of token filtering within the kernel, where the kernel can create restricted tokens of regular ones on demand by the caller. The implementation can be accessed thorough a NT syscall, NtFilterToken, and a kernel mode routine, SeFilterToken.
The continue statements do not server any useful purpose in these loops so they're basically pointless. These have been introduced by mistake so my bad.
A scenario where it happens that an access token belongs to an administrators group but it's disabled (that is, SeAliasAdminsSid has no attributes or it doesn't have SE_GROUP_ENABLED turn ON), the function removes this group from the token but still has TOKEN_HAS_ADMIN_GROUP flag which can lead to erratic behavior across the kernel and security modules -- implying that the token still belongs to administrators group.
This is an oversight from my part.
In SeCaptureLuidAndAttributesArray we must ensure that we don't go onto a potential integer overflow scenario by checking against the maximum limit threshold the kernel states. In addition, write an explicit name macro for the value.
The function might assign the flag yet it could possibly fail on creating a DACL and insert an "access allowed" right to the access entry within the DACL. In this case, make sure we actually succeeded on all the tasks and THEN assign the flag that the DACL is truly present.
Also, make sure that the Current buffer size variable gets its new size so that we avoid overidding the memory of the DACL if the security descriptor wants both a DACL and SACL and so that happens that the DACL memory gets overwritten by the SACL.
Implement the portion chunk of code that is responsible for setting the system access control list (SACL) to the World security descriptor, based from SeWorldSid (World security identifier).
When performing access security check, use the security descriptor that we've captured it to determine based on that descriptor if the client can be granted access or not.
As we now have the SEF_* flags declared within the SDK we can simply check for such flags directly wihout having to check for the hard-coded flag values.
This implements the EffectiveOnly option of SepDuplicateToken routine (used by NtDuplicateToken syscall and other functions alike) which makes the access token effective by removing the disabled parts like privileges and groups.
Fix a wrong returned datatype of the function, as SepSinglePrivilegeCheck calls the internal private SepPrivilegeCheck function which returns a BOOLEAN value.
When creating or duplicating an access token object, make sure that the logon session is getting referenced by the token must be inserted onto the logon reference member (a.k.a LogonSession) for proper logon session referencing tracking.
Also when a token object is about to be destroyed or that we are taking away a reference session from it, we must ensure that the referenced logon session data gets removed from the token in question.
CORE-17700
When duplicating an access token, the authentication ID is already copied from the existing token to the new one anyway so there's no point on having the commented call still left in the code.
