[ros-diffs] [sir_richard] 46027: [WIN32K]: Stub-plement NtUserSetInformationThread for UserInitiateShutdown and UserEndShutdown. I think we're supposed to notify Winlogon at some point, and check for races. Part of new shutdown code.
sir_richard at svn.reactos.org
sir_richard at svn.reactos.org
Tue Mar 9 10:26:46 CET 2010
- Previous message: [ros-diffs] [jimtabor] 46026: - [User32_winetest] Restore Timer test.
- Next message: [ros-diffs] [sir_richard] 46028: [NTOS]: While attempting to ressucitate the user-mode shutdown code in CSRSS (disabled since 2006), it seemed clear that one of the main steps is to obtain the caller's LUID in order to kill the right processes. This LUID is obtained from the current thread's token, and we know it's the callers since we're supposed to impersonate the caller. Unfortunately, impersonation failed, making the whole process fail. Impersonation failed because NtImpersonateThread was actually inverting the THREAD_IMPERSONATE rights, asking the client thread for the server's permissions, and vice versa. Fixing this resulted in yet another failure. Analysis of this failure showed that even opening the server (ie: current) thread for THREAD_DIRECT_IMPERSONATION failed, which is unusual since the current thread should have access to all its rights. This is determined in PspCreateThread when the ETHREAD->GrantedAccess field is set. Continuing onto this path, tracing revealed that GrantedAccess was merely 0x1607F and not 0x1F3FF as expected, meaning that not all rights were given, including the impersonate right (0x200), explaining the failure, but not the deeper reason behind it. Attempting to identify which code path set this GrantedAccess, the SepAccessCheck routine came to light. A bug there caused MAXIMUM_ALLOWED accesses to fail in certain scenarios, such as when the object had no security descriptor, because MAXIMUM_ALLOWED would be granted as an absolute value, when instead of it is a flag that should grant GENERIC_ALL. Fixing that bug, the failure continued. Further analysis identified that the Administrators SID was being found with GENERIC_READ + WRITE + EXECUTE access, and no SID was found for GENERIC_ALL access. This happened when searching the system token's default DACL, which is assigned to the kernel (but for kernel-mode callers, this check was skipped), smss, winlogon, etc. The code for creating this system token was heavily hacked, so the function to create the system token, as well as NtCreateToken were refactored to use a common piece of token-creating code. Furthermode, the system token was correctly created with Local System as the user, and Administrators as one of the groups. Finally, SeDefaultDacl was used (already setup properly) instead of a badly configured Default DACL. The new shared code also correctly sets the SE_GROUP_ENABLED flag on all SE_GROUP_MANDATORY groups, and scans tokens to set the TOKEN_HAS_ADMIN_GROUP and TOKEN_HAS_TRAVERSE_PRIVILEGE flags which were not previously set. With the correct system token and default DACL, the Local System SID was now found, but the failure continued. This was revealed to be due to an even deeper rooted problem, as the SepIsSidInToken routine checked for SE_GROUP_ENABLED before listing a SID as "present". Although this is correct for actual groups, the User SID will never have the SE_GROUP_ENABLED flag as it is not a group. This caused any granted access ACE belonging to a user (instead of a group) to be ignored, causing either access check failures, or limited rights returned (in the MAXIMUM_ALLOWED case). Upon fixing this bug, the NtImpersonateThread call finally returned success, since the rights were now correct. Promptly upon calling NtOpenThreadToken to query the LUID however, the system ASSERTED with FALSE. The code at fault was a line in NtOpenThreadTokenEx which forcefully ASSERTed if the impersonation code path was taken, presumably because this was never tested, and ReactOS would actually always fail impersonation attempts due to the bugs fixed above. This routine was thus quickly reworked to fix some mistakes (such as forgetting to actually impersonate the client, incorrect referencing/dereferencing of thread/tokens, and assumptions about DACL creation success). Having fixed the NtOpenThreadTokenEx routine, the LUID query now went through for the impersonated token, but soon causing a user-mode crash, due to an incorrect parameter reference in the CsrGetProcessLuid function in the csrsrv code by Alex (which I copy/pasted to reduce code duplication). Fixing this final bug finally allowed the correct LUID to be queried and I was able to continue development of not-yet-commited user-mode shutdown code.
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
Author: sir_richard
Date: Tue Mar 9 10:26:46 2010
New Revision: 46027
URL: http://svn.reactos.org/svn/reactos?rev=46027&view=rev
Log:
[WIN32K]: Stub-plement NtUserSetInformationThread for UserInitiateShutdown and UserEndShutdown. I think we're supposed to notify Winlogon at some point, and check for races. Part of new shutdown code.
Modified:
trunk/reactos/include/reactos/win32k/ntuser.h
trunk/reactos/subsystems/win32/win32k/ntuser/ntstubs.c
Modified: trunk/reactos/include/reactos/win32k/ntuser.h
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/include/reactos/win32k/ntuser.h?rev=46027&r1=46026&r2=46027&view=diff
==============================================================================
--- trunk/reactos/include/reactos/win32k/ntuser.h [iso-8859-1] (original)
+++ trunk/reactos/include/reactos/win32k/ntuser.h [iso-8859-1] Tue Mar 9 10:26:46 2010
@@ -5,6 +5,21 @@
typedef struct _THREADINFO *PTHREADINFO;
struct _DESKTOP;
struct _WND;
+
+typedef enum _USERTHREADINFOCLASS
+{
+ UserThreadShutdownInformation,
+ UserThreadFlags,
+ UserThreadTaskName,
+ UserThreadWOWInformation,
+ UserThreadHungStatus,
+ UserThreadInitiateShutdown,
+ UserThreadEndShutdown,
+ UserThreadUseActiveDesktop,
+ UserThreadUseDesktop,
+ UserThreadRestoreDesktop,
+ UserThreadCsrApiPort,
+} USERTHREADINFOCLASS;
typedef struct _LARGE_UNICODE_STRING
{
@@ -2618,13 +2633,14 @@
DWORD dwUnknown3,
DWORD dwUnknown4);
-DWORD
+NTSTATUS
NTAPI
NtUserSetInformationThread(
- DWORD dwUnknown1,
- DWORD dwUnknown2,
- DWORD dwUnknown3,
- DWORD dwUnknown4);
+ IN HANDLE ThreadHandle,
+ IN USERTHREADINFOCLASS ThreadInformationClass,
+ IN PVOID ThreadInformation,
+ IN ULONG ThreadInformationLength
+);
DWORD
NTAPI
Modified: trunk/reactos/subsystems/win32/win32k/ntuser/ntstubs.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/ntuser/ntstubs.c?rev=46027&r1=46026&r2=46027&view=diff
==============================================================================
--- trunk/reactos/subsystems/win32/win32k/ntuser/ntstubs.c [iso-8859-1] (original)
+++ trunk/reactos/subsystems/win32/win32k/ntuser/ntstubs.c [iso-8859-1] Tue Mar 9 10:26:46 2010
@@ -1029,16 +1029,28 @@
return 0;
}
-DWORD
-APIENTRY
-NtUserSetInformationThread(
- DWORD dwUnknown1,
- DWORD dwUnknown2,
- DWORD dwUnknown3,
- DWORD dwUnknown4)
-{
- UNIMPLEMENTED;
- return 0;
+NTSTATUS
+APIENTRY
+NtUserSetInformationThread(IN HANDLE ThreadHandle,
+ IN USERTHREADINFOCLASS ThreadInformationClass,
+ IN PVOID ThreadInformation,
+ IN ULONG ThreadInformationLength)
+
+{
+ if (ThreadInformationClass == UserThreadInitiateShutdown)
+ {
+ DPRINT1("Shutdown initiated\n");
+ }
+ else if (ThreadInformationClass == UserThreadEndShutdown)
+ {
+ DPRINT1("Shutdown ended\n");
+ }
+ else
+ {
+ UNIMPLEMENTED;
+ }
+
+ return STATUS_SUCCESS;
}
DWORD
- Previous message: [ros-diffs] [jimtabor] 46026: - [User32_winetest] Restore Timer test.
- Next message: [ros-diffs] [sir_richard] 46028: [NTOS]: While attempting to ressucitate the user-mode shutdown code in CSRSS (disabled since 2006), it seemed clear that one of the main steps is to obtain the caller's LUID in order to kill the right processes. This LUID is obtained from the current thread's token, and we know it's the callers since we're supposed to impersonate the caller. Unfortunately, impersonation failed, making the whole process fail. Impersonation failed because NtImpersonateThread was actually inverting the THREAD_IMPERSONATE rights, asking the client thread for the server's permissions, and vice versa. Fixing this resulted in yet another failure. Analysis of this failure showed that even opening the server (ie: current) thread for THREAD_DIRECT_IMPERSONATION failed, which is unusual since the current thread should have access to all its rights. This is determined in PspCreateThread when the ETHREAD->GrantedAccess field is set. Continuing onto this path, tracing revealed that GrantedAccess was merely 0x1607F and not 0x1F3FF as expected, meaning that not all rights were given, including the impersonate right (0x200), explaining the failure, but not the deeper reason behind it. Attempting to identify which code path set this GrantedAccess, the SepAccessCheck routine came to light. A bug there caused MAXIMUM_ALLOWED accesses to fail in certain scenarios, such as when the object had no security descriptor, because MAXIMUM_ALLOWED would be granted as an absolute value, when instead of it is a flag that should grant GENERIC_ALL. Fixing that bug, the failure continued. Further analysis identified that the Administrators SID was being found with GENERIC_READ + WRITE + EXECUTE access, and no SID was found for GENERIC_ALL access. This happened when searching the system token's default DACL, which is assigned to the kernel (but for kernel-mode callers, this check was skipped), smss, winlogon, etc. The code for creating this system token was heavily hacked, so the function to create the system token, as well as NtCreateToken were refactored to use a common piece of token-creating code. Furthermode, the system token was correctly created with Local System as the user, and Administrators as one of the groups. Finally, SeDefaultDacl was used (already setup properly) instead of a badly configured Default DACL. The new shared code also correctly sets the SE_GROUP_ENABLED flag on all SE_GROUP_MANDATORY groups, and scans tokens to set the TOKEN_HAS_ADMIN_GROUP and TOKEN_HAS_TRAVERSE_PRIVILEGE flags which were not previously set. With the correct system token and default DACL, the Local System SID was now found, but the failure continued. This was revealed to be due to an even deeper rooted problem, as the SepIsSidInToken routine checked for SE_GROUP_ENABLED before listing a SID as "present". Although this is correct for actual groups, the User SID will never have the SE_GROUP_ENABLED flag as it is not a group. This caused any granted access ACE belonging to a user (instead of a group) to be ignored, causing either access check failures, or limited rights returned (in the MAXIMUM_ALLOWED case). Upon fixing this bug, the NtImpersonateThread call finally returned success, since the rights were now correct. Promptly upon calling NtOpenThreadToken to query the LUID however, the system ASSERTED with FALSE. The code at fault was a line in NtOpenThreadTokenEx which forcefully ASSERTed if the impersonation code path was taken, presumably because this was never tested, and ReactOS would actually always fail impersonation attempts due to the bugs fixed above. This routine was thus quickly reworked to fix some mistakes (such as forgetting to actually impersonate the client, incorrect referencing/dereferencing of thread/tokens, and assumptions about DACL creation success). Having fixed the NtOpenThreadTokenEx routine, the LUID query now went through for the impersonated token, but soon causing a user-mode crash, due to an incorrect parameter reference in the CsrGetProcessLuid function in the csrsrv code by Alex (which I copy/pasted to reduce code duplication). Fixing this final bug finally allowed the correct LUID to be queried and I was able to continue development of not-yet-commited user-mode shutdown code.
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
More information about the Ros-diffs
mailing list