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.
NormalContext and NormalRoutine are just for good measure, but
SystemArgument2 is actually used by the function.
And yes, this appears to be a bug in Win 2003.
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.
Only resources of HAL were checked against conflicts, not those of PnP Manager
Let IoReportResourceForDetection() make a silent conflict check.
Otherwise IopCheckResourceDescriptor() will always return 'no conflict'.
CORE-17789
Previous code did not detect equal resource ranges as conflicting.
Thanks Hervé Poussineau for pointing this out!
Meanwhile, simplify the code to make it more readable.
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.
Certainly due to copy-pasta error from the original code.
A consequence of this oversight, was that the IoGetDeviceObjectPointer()
calls on these device names, in fltmgr!DriverEntry() couldn't work.
(See drivers/filters/fltmgr/Interface.c, line 1880 and below.)
- NtQuerySymbolicLinkObject(): Use an intermediate variable for the object header.
- Simplify code in ObpLookupEntryDirectory() by calling ObpReleaseLookupContextObject() instead.
- Use TAG_OBJECT_TYPE instead of hardcoded tag values.
- Disentangle the usage of ObpAcquireDirectoryLockExclusive() when it's
used only for accessing a directory structure, or as part of a lookup
operation.
The Obp*DirectoryLock*() -- both shared and exclusive -- functions
are only for locking an OB directory, for reading or writing its
structure members.
When performing lookup operations (insertions/deletions of entries
within a directory), use a ObpAcquireLookupContextLock() function that
exclusively locks the directory and saves extra lock state, that can
be used by ObpReleaseLookupContextObject() for cleanup.
- Add documentation for these functions.
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).
Addendum to 608032bd and 835c3023.
The IRQL is actually raised by KeFreezeExecution() and lowered by
KeThawExecution(), always to HIGH_IRQL on MP systems, or if necessary
on UP. These functions are called respectively by KdEnterDebugger()
and KdExitDebugger().
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 processing:
Make sure that the process is not terminating.
Make sure that the process WorkingSet is still valid
Protect accessing & writing to PTEs by acquiring the working set lock
CORE-17595 CORE-17642
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.
Note to SELF and EVERYONE: the commit implements the initial logon session termination notification implementation, the SeMarkLogonSessionForTerminationNotification function, but as it currently stands there are several other tasks to be addressed in the future in order for the logon termination notification to be fully completed. The tasks as of which are.
1. Our SepRmDereferenceLogonSession is not fully implemented, as it doesn't inform the LSA and filesystems of logon deletion notification
2. Implement two worker routines that are actually in charge of such tasks of informing LSA and FSDs
3. Perform logon deletion
4. Do further investigations and check whatever that is left to address, if any
* Quality of service kernel stuff bears nothing with security descriptors in anyway, so just have a file specifically for it
* Annotate the function arguments parameters with SAL
* Document the functions
Use REG_OPTION_NON_VOLATILE instead of REG_OPTION_VOLATILE in all ZwCreateKey calls of OpenRegistryHandlesFromSymbolicLink, since the keys created/opened by this function, should be non-volatile (in other words, be saved after reboot).
Also Device Parameters subkey that is created in IoOpenDeviceInterfaceRegistryKey (which uses that routine as well), is non-volatile too, so the parent keys whose contain it, cannot be volatile.
It will fix an error with status 0xc0000181 (STATUS_CHILD_MUST_BE_VOLATILE) occuring during loading kernel mode audio drivers from Windows XP/2003, especially checked (debug) versions, with my IoGetDeviceInterfaceAlias implementation. Also it may fix other error cases.
CORE-17361
We allocate memory pool for a new security descriptor with specific info filled by the caller but we don't set the control flag bits for the newly allocated descriptor, which is wrong. Originally spotted by Vadim Galyant.
CORE-17650
KD64: Raise to HIGH_LEVEL when entering trap
KDBG: lower to DISPATCH_LEVEL when applying IRQL hack & use a worker thread to load symbols
KD&KDBG: Actually unload symbols when required
Raise IRQL before entering debugger, so that KeAcquireSpinLockAtDpcLevel works as expected.
- HIGH_LEVEL since we don't know where we are coming from.
Do not try to read debug symbol from files in KDBG.
- There is no reason that this works if Mm didn't map it in the first place.
GCC has some functions, variables & type attributes which can be used as aliases
for some of the SAL annotations. Although it's not as rich & precise, it's still useful
since we actually enable -Werror on GCC builds whereas we don't use such an option
on MSVC builds.
For now, _Must_inspect_result_ is aliased to warn_result_unused attribute.
ProcessUserModeIOPL, ProcessWow64Information and ThreadZeroTlsCell classes fail on AMD64 build because of wrong IQS values assigned to them. Also explicitly tell the compiler that ProcessUserModeIOPL is strictly for x86 as user mode I/O privilege level is 32-bit stuff.
In addition to that, implement IQS_NO_TYPE_LENGTH macro which it'll be used for classes such as ProcessUserModeIOPL where type length is not required and that it should be 0. With that said, we indirectly fix a size length mismatch issue with ProcessUserModeIOPL on 32-bit of ReactOS as well.
- Remove a redundant call of ObReferenceObjectByHandle. Not only it didn't make much sense (we reference the object from thread handle and the new thread object referencing the same handle!), specifying a request access of THREAD_ALL_ACCESS for the thread object is kind of suspicious and all of these access rights are unwanted.
- Add some failure checks involving the CopyOnOpen code paths
- Add some DPRINT1 debug prints (concerning the CopyOnOpen code paths as usual)
driverName.Buffer leaked when the "(!NT_SUCCESS(status) || ServiceName != NULL)"
case is taken because ServiceName != NULL, and some of the functions fail.
- Fix CID 1477246: Uninitialized pointer read (UNINIT) (happens in
the last ExFreePoolWithTag(basicInfo, TAG_IO) call when the
"(!NT_SUCCESS(status) || ServiceName != NULL)" case is not taken).
- Centralize all the ExFreePoolWithTag(basicInfo, TAG_IO) cleanups
at the end of the function.
- Both cases "(driverName.Buffer == NULL)" and "(ServiceName != NULL)"
can only be taken when basicInfo != NULL, so assert on this fact.
- Fix/add comments;
- Reduce indentation level;
- Direct copy for registry integer values;
- Use for-loops for linked lists;
- Use ULONG when the API uses it (sizes for Ob, or REG_DWORD data in registry).
When freeing pages, free page entries with pages num == 3 were
incorrectly treated as entries with pages num >= 4 and thus
their re-insertion was not triggered. That lead to non paged pool
fragmentation (can be triggered by kmtest:ExPools, for example)
Also, altered the index acquisition code for MmNonPagedPoolFreeList
entries so it looks more clear
- Manage the lifetime of the temporary 'PartitionBuffer' buffer where
it is locally used only, and free it as soon as possible, just after
calculating the sector checksum. No need to then free it outside of
the main for-loop.
- When the 'DriveLayout' buffer is freed, ensure the pointer is NULL-ed
(and assert this at the top of the main for-loop), since it can also
be freed at cleanup outside this for-loop, and in this case a NULL
check is performed.
This will avoid the scenario of possibly double-freeing a pointer,
in the case the 'DriveLayout' was previously freed (when e.g. reading
the sector for checksum calculation failed), then the for-loop goes to
the next disk and stops early.
The purpose of 'SingleDisk' is the same as in the IopCreateArcNames()
function. It is an optimization for that when looking up the
firmware-recognized ARC disks list, in order to match one of these with
the current NT disk being analysed (see e.g. also in IopCreateArcNamesDisk()),
we avoid a possible IopVerifyDiskSignature() call and directly build a
corresponding ARC name NT symbolic link for it.
'SingleDisk' will actually be TRUE, whether the DiskSignatureListHead
list is empty or contains only one element: Indeed in only both these
cases, 'DiskSignatureListHead.Flink->Flink' will refer to the list head.
(If the list is empty but 'SingleDisk' is TRUE, this does not matter,
because the DiskSignatureListHead looking-up loop never starts.)
In addition to that, here are some stuff done in this commit whilst testing:
- ICIF_QUERY_SIZE_VARIABLE and friends were badly misused, they should be used only when an information class whose information length size is dyanmic and not fixed. By removing such flags from erroneous classes, this fixes the STATUS_INFO_LENGTH_MISMATCH testcases.
- Use CHAR instead of UCHAR for classes that do not need alignment probing, as every other class in the table do, for the sake of consistency.
- ProcessEnableAlignmentFaultFixup uses BOOLEAN as type size, not CHAR. This fixes a testcase failure on ROS.
- Check for information length size before proceeding further on querying the process' cookie information.
- ProcessHandleTracing wants an alignment of a ULONG, not CHAR.
- Move PROCESS_LDT_INFORMATION and PROCESS_LDT_SIZE outside of NTOS_MODE_USER macro case. This fixes a compilation issue when enabling the alignment probing. My mistake of having them inside NTOS_MODE_USER case, sorry.
- On functions like NtQueryInformationThread and the Process equivalent, complete probing is not done at the beginning of the function, complete probing including if the buffer is writable alongside with datatype misalignment check that is. Instead such check is done on each information class case basis. With that said, we have to explicitly tell DefaultQueryInfoBufferCheck if we want a complete probing or not initially.
KiSetTrapContext is an asm wrapper around RtlSetUnwindContext, which first stores an exception frame to assure that all non-volatile registers were put on the stack, then calls RtlSetUnwindContext to update their first saving positions on the stack and finally restore the exception frame to potentially load any updated registers, that haven't been saved elsewhere on the stack.
A few of these classes have fixed size lengths, the rest are arbitrary. Also the TokenAuditPolicy class hasn't a size length type specified in the table, which is wrong (and move the corresponding TOKEN_AUDIT_POLICY_INFORMATION structure into the private header).
PsImpersonateClient blindly impersonates the requested client even though it doesn't know if the actual token given to the call can be impersonated for the thread of the client which we are going to begin impersonation. In the case where impersonation is not possible, make a copy of the given token and assign the newly one for impersonation instead.
CORE-17539
Implement SepImpersonateAnonymousToken private helpers, which is necessary for the complete implementation of NtImpersonateAnonymousToken function and thus finally we're able to impersonate the anonymous logon token.
As of now the Object Manager private service, ObpCloseHandleTableEntry, looks for OBJ_PROTECT_CLOSE attribute if a handle should not be closed. However, in ObDuplicateObject if an attribute of OBJ_PROTECT_CLOSE is found as it's been filled to the caller (see L2466) this attribute is removed from the attributes list of the new handle and ObpAccessProtectCloseBit access is granted to the newly duplicated object handle.
With that being said ObpCloseHandleTableEntry indiscriminately closes the object handle albeit it shouldn't do so. As a matter of fact in Windows Server 2003 SP2 this service indeed checks for ObpAccessProtectCloseBit flag bit and if the condition is met then it returns STATUS_HANDLE_NOT_CLOSABLE as it should. Therefore we should do the same.
Now NtClose can properly warn the calling thread the object handle can't be closed which fixes a testcase failure within NtDuplicateObject NTDLL APITEST where this function gives handle close protection bit as requested by the caller.
* Guard the token in a lock whilst querying stuff
* Remove the piece of code that checks if the information class provided is above the maximum information class threshold. That code literally duplicates the inner functionality of the default case in the switch block, where the code falls in that case if an invalid information class is provided anyway.
* Remove the redundant information classes. Internally, this function in Windows has 12 switch case blocks (11 token info classes + the default case) and the other classes are supported in NtQueryInformationToken only so it doesn't make any logical sense to keep them in the codebase.
* Annotate the argument parameters with SAL and add documentation header
Properly handle PDE refcounting
Clean-up of the internal API
Enforce attaching to the process when modifying its memory layout, instead of
making circonvoluted mappings which always end up being broken.
- Do not lock the section segment when we are serving a fault for a process private page.
- Do not keep the process address space lock while writing to pagefile.
- Do not wait for an event that might never be set.
Mute debug prints of MmDoesFileHaveUserWritableReferences and SeAuditingFileEventsWithContext stubs.
These stubs are very noisy and create a lot of spam in the log when using Microsoft NTFS driver in ReactOS (with some other improvements applied).
Implementing those functions isn't badly required for the proper work of this driver, so better way for now is just mute these stubs a bit.
After my changes, they will be displayed only once, and the log will be more clear, so it will seem to be enough to understand that the driver calls these routines.
CORE-17409
This function is used during the Security kernel module phase initialisation to set up the system process token which the phase initialisation procedure in itself is stored in the INIT section. With that being said, do the same for SepCreateSystemProcessToken too and add a header documentation as an addition.
These private functions are needed to set up two different kinds of system's anonymous logon tokens: one that includes everyone in the group and the other that doesn't. These functions are needed as next step closer to the
implementation of NtImpersonateAnonymousToken system call.
