As it currently stands the PsImpersonateClient routine does the following approach.
If impersonation couldn't be granted to a client the routine will make a copy
of the client's access token. As it makes a copy of the said token PsImpersonateClient
will reference the copied token after impersonation info have been filled out.
In the same code path we are assigning the desired level for impersonation to thread
impersonation info.
This is wrong for two reasons:
- On a copy situation the SeCopyClientToken routine holds a reference as the object
has been created. Referencing it at the bottom of the PsImpersonateClient routine
will make it that the token is referenced twice and whenever a server stops
impersonation the token still has an extra reference count which keeps the token
still alive in object database and memory space.
- If client impersonation is not possible the thread impersonation info should
have been assigned SecurityIdentification level to further indicate that the
actual impersonation of the thread is not currently in force but instead we
are assigning the impersonation level that is supplied by the caller. For instance
if the requested level is SecurityDelegation but impersonation is not possible
the level will be assigned that of SecurityDelegation yet the token has an
impersonation level of SecurityIdentification. This could lead to erratic behaviors
as well as potential impersonation escalation.
Fix the aforementioned issues by avoiding a double reference and properly assign
the impersonation level to SecurityIdentification if the server is not able to
impersonate the target client.
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
Ensure that when we're cleaning up the EPROCESS object, that we are dereferencing the quota block the process in question was using. The routine will automatically request a quota block cleanup if the process that dereferenced the quota block was the last.
-- Rewrite PspChargeProcessQuotaSpecifiedPool and PspReturnProcessQuotaSpecifiedPool private kernel routines, with the goal to implement the algorithms necessary to manage the fields of quota blocks (Usage, Return, Limit and Peak).
-- Invoke the Mm if quota limit raising or returning has to be done
-- When destroying a quota block, make sure that we're giving back all the rest of non-returned quotas to Memory Mm
-- Crash the system with QUOTA_UNDERFLOW if someone is returning way too much quota than it was previously charged
-- When a process exits, ensure that it doesn't hold up any charged quotas in QuotaUsage field of the process object, that way we're enforcing proper kernel consistency
-- Implement PsChargeSharedPoolQuota and PsChargeProcessPageFileQuota functions, used exclusively by the Object Manager. These routines are used to charge or return amount of quotas of a newly created object.
-- On PspInheritQuota, when assigning to process the default quota block if no parent process is given, we must increment the reference counts as we're using it
-- Handle the ProcessCount reference field, as it wasn't used
-- Annotate the functions with SAL
-- Document the code
=== REMARKS ===
Windows LogOn (Winlogon) is responsible for setting up a different quota block for all the processes within an interactive session, which is what we don't do. What we're currently doing instead is we're using the default block, PspDefaultQuotaBlock, for all the processes
across the system. The default block contains the default limits of -1 (which would imply no limits). By definition, the kernel won't ever return STATUS_QUOTA_EXCEEDED as we literally don't set up a definite limit for regular processes. This situation has to be tackled
in the future.
=== TODO FOR FUTURE ===
Most of the code in PspChargeProcessQuotaSpecifiedPool and PspReturnProcessQuotaSpecifiedPool private routines must be refactored in order to reduce the usage of the quota spin lock, possibly wrapping such code in a loop and whatnot.
CORE-17784
We have a special file, tag.h, which serves as a place to store whatever kernel pool allocation tag yet we still have some tags sparse over the kernel code... So just re-group them in one unique place.
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.
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.
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
Prior to acquiring a quota from the process and do whatever it's needed to do (charge it or return it back), we must guard ourselves with a spinlock so that we may not get into potential race conditions. In Windows Server 2003, PspGivebackQuota and PspExpandQuota do the same thing and they're the equivalent to PspReturnProcessQuotaSpecifiedPool and PspChargeProcessQuotaSpecifiedPool in our codebase.
- Change INIT_FUNCTION and INIT_SECTION to CODE_SEG("INIT") and DATA_SEG("INIT") respectively
- Remove INIT_FUNCTION from function prototypes
- Remove alloc_text pragma calls as they are not needed anymore
- Always include kd64.h
- Change KdpPrompt() prototype to be compatible between KDBG and _WINDK_
- Rename KdComponentTable to KdpComponentTable to prevent a conflict
- Add some functions stubs and global variables
This allows avoiding one of the previous implementation limits:
leaked IRP not queued to a thread are now totally visible since
we look directly in the memory pool.
This is far from perfect, and totally doesn't match the
WinDBG way of doing it. Instead of browsing pool to find
matching 'IRP' tags, we just browse all the processes
to find the queued IRP. This requires the IRPs to be queued,
obviously, and will make us miss the leaked IRPs, for instance.
Proper way to do it would be to implement !poolfind and then
rely on its implementation to find our IRPs.
Perhaps later ;-)
* Add an NDK header to define INIT_FUNCTION/INIT_SECTION globally
* Use _declspec(allocate(x)) and _declspec(code_seg(x)) on MSVC versions that support it
* Use INIT_FUNCTION on functions only and INIT_SECTION on data only (required by MSVC)
* Place INIT_FUNCTION before the return type (required by MSVC)
* Make sure declarations and implementations share the same modifiers (required by MSVC)
* Add a global linker option to suppress warnings about defined but unused INIT section
* Merge INIT section into .text in freeldr
The PROCESS_DEVICEMAP_INFORMATION union has 2 fields, one is a handle, the other one is a structure of 36 bytes (independent of architecture). The handle forces 64 bit alignment on 64 bit builds, making the structure 4 bytes bigger than on 32 bit builds. The site is checked in NtQueryInformationProcess (case ProcessDeviceMap). The expected size on x64 is the size of the Query structure without alignment. autocheck correctly passes the site of the Query union member, while smss passes the full size of PROCESS_DEVICEMAP_INFORMATION. Packing the structure is not an option, since it is defined in public headers without packing. Using the original headers sizeof(PROCESS_DEVICEMAP_INFORMATION) is 0x28, sizeof(PROCESS_DEVICEMAP_INFORMATION::Query) is 0x24.
