From 3780e42ff997745241aca908afc8c6f8f9785a9c Mon Sep 17 00:00:00 2001 From: Joachim Henze Date: Sat, 27 May 2023 11:19:17 -0400 Subject: [PATCH] [0.4.15][NTOS:SE][NDK][KMTESTS:SE] Fix 'kmtest_.exe SeQueryInfoToken' (#5308) This backport fixes 'kmtest_.exe SeQueryInfoToken' on all testers: VBox x86, KVM x86, WHS x86, Win2003_x64. And according to Thomas description may also prevent a buffer overrun when executing that formerly broken test. Afterwards all 76 tests of this suite do complete on all those builders. Before the patch only 74 of those tests succeeded, 2 failed. The fix is a squashed backport of the following 6 commits from Thomas Faber: 0.4.16-dev-11-g 44bdafa17e2b870c570ea15f3bdb56decfd7aaa5 [KMTESTS:SE] Fix failing tests (#5308) 0.4.16-dev-10-g bf6af0f52eba23b2ca6677813792baa80c6d1f82 [NTOS:SE] Mark output parameters as such (#5308) 0.4.16-dev-9-g 156053cafd3d325ac5d6e6c341dae55bbb99b88c [NDK] Match AUX_ACCESS_DATA definition with publicly available version. - if you allocated only sizeof(AUX_ACCESS_DATA), the test would crash with a 4 byte buffer overflow. (#5308) 0.4.16-dev-8-g ff410211e960b466c9c5c887878eb70823ad3599 [KMTESTS:SE] Don't modify internal data structure, this might cause buffer overrun (#5308) 0.4.16-dev-7-g 206df96bc474cb154934b43453159079520e0512 [KMTESTS:SE] Correctly allocate PrivilegeSet buffers (#5308) 0.4.16-dev-6-g 64a6bd4c3e88780a6e4082f004ad7d2c552bc49e [KMTESTS:SE] Avoid use of uninitialized pool and hardcoded offsets (#5308) WHS x86 before-and-after-state, the after-test had a few fixes from Timos unrelated PR7343 inside unfortunately: https://reactos.org/testman/compare.php?ids=97640,97871 (This is added to prove the test being wrong) I tested it also successfully on my local 2k3sp2 x86 with the releases/0.4.15 afterstate, built with RosBEWin2.2.2 GCC8.4.0dbg x86. Win2003_x64 0.4.16-dev-11-g44bdafa at 2024-09-12 15:19 (after-state): https://reactos.org/testman/compare.php?ids=97791 0.4.16-dev-5-g2913ef5 vs. 0.4.16-dev-11-g44bdafa vs. 0.4.16-dev-23-g53b304e: VBox x86 https://reactos.org/testman/compare.php?ids=97795,97806,97877 0.4.16-dev-5-g2913ef5 vs. 0.4.16-dev-20-g144a8b5 vs. 0.4.16-dev-21-g2af6fd4: KVM x86 https://reactos.org/testman/compare.php?ids=97793,97855,97856 Since we do touch the NTOS and NDK here the fix is not guaranteed to be side-effect-free, but since we are so early in the RC-phase, I dared to pick it, especially since the alternative would have been to disable the test altogether in the releases/0.4.15 which would have been a pity, if we can also have it all green everywhere. --- .../kmtests/ntos_se/SeQueryInfoToken.c | 95 ++++++++++++------- ntoskrnl/ob/obhandle.c | 6 +- ntoskrnl/se/access.c | 20 ++-- ntoskrnl/se/priv.c | 14 +-- sdk/include/ndk/setypes.h | 19 +++- 5 files changed, 96 insertions(+), 58 deletions(-) diff --git a/modules/rostests/kmtests/ntos_se/SeQueryInfoToken.c b/modules/rostests/kmtests/ntos_se/SeQueryInfoToken.c index a506ceb78ca..950cb78e35c 100644 --- a/modules/rostests/kmtests/ntos_se/SeQueryInfoToken.c +++ b/modules/rostests/kmtests/ntos_se/SeQueryInfoToken.c @@ -13,6 +13,19 @@ #define NDEBUG #include +// Copied from PspProcessMapping -- although the values don't matter much for +// the most part. +static GENERIC_MAPPING ProcessGenericMapping = +{ + STANDARD_RIGHTS_READ | PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, + STANDARD_RIGHTS_WRITE | PROCESS_CREATE_PROCESS | PROCESS_CREATE_THREAD | + PROCESS_VM_OPERATION | PROCESS_VM_WRITE | PROCESS_DUP_HANDLE | + PROCESS_TERMINATE | PROCESS_SET_QUOTA | PROCESS_SET_INFORMATION | + PROCESS_SUSPEND_RESUME, + STANDARD_RIGHTS_EXECUTE | SYNCHRONIZE, + PROCESS_ALL_ACCESS +}; + //------------------------------------------------------------------------------// // Testing Functions // //------------------------------------------------------------------------------// @@ -216,14 +229,13 @@ START_TEST(SeQueryInfoToken) NTSTATUS Status = STATUS_SUCCESS; PAUX_ACCESS_DATA AuxData = NULL; PPRIVILEGE_SET NewPrivilegeSet; + ULONG InitialPrivilegeCount; BOOLEAN Checker; PPRIVILEGE_SET Privileges = NULL; PSECURITY_SUBJECT_CONTEXT SubjectContext = NULL; PACCESS_TOKEN Token = NULL; PTOKEN_PRIVILEGES TPrivileges; PVOID Buffer; - POBJECT_TYPE PsProcessType = NULL; - PGENERIC_MAPPING GenericMapping; ULONG i; SubjectContext = ExAllocatePool(PagedPool, sizeof(SECURITY_SUBJECT_CONTEXT)); @@ -240,14 +252,14 @@ START_TEST(SeQueryInfoToken) //----------------------------------------------------------------// AccessState = ExAllocatePool(PagedPool, sizeof(ACCESS_STATE)); - PsProcessType = ExAllocatePool(PagedPool, sizeof(OBJECT_TYPE)); - AuxData = ExAllocatePool(PagedPool, 0xC8); - GenericMapping = ExAllocatePool(PagedPool, sizeof(GENERIC_MAPPING)); + // AUX_ACCESS_DATA gets larger in newer Windows version. + // This is the largest known size, found in Windows 10/11. + AuxData = ExAllocatePoolZero(PagedPool, 0xE0, 'QSmK'); Status = SeCreateAccessState(AccessState, - (PVOID)AuxData, + AuxData, DesiredAccess, - GenericMapping + &ProcessGenericMapping ); ok((Status == STATUS_SUCCESS), "SeCreateAccessState failed with Status 0x%08X\n", Status); @@ -267,43 +279,48 @@ START_TEST(SeQueryInfoToken) // Testing SeAppendPrivileges // //----------------------------------------------------------------// - AuxData->PrivilegeSet->PrivilegeCount = 1; + InitialPrivilegeCount = AuxData->PrivilegesUsed->PrivilegeCount; + trace("Initial privilege count = %lu\n", InitialPrivilegeCount); // Testing SeAppendPrivileges. Must change PrivilegeCount to 2 (1 + 1) - NewPrivilegeSet = ExAllocatePool(PagedPool, sizeof(PRIVILEGE_SET)); + NewPrivilegeSet = ExAllocatePoolZero(PagedPool, + FIELD_OFFSET(PRIVILEGE_SET, Privilege[1]), + 'QSmK'); NewPrivilegeSet->PrivilegeCount = 1; Status = SeAppendPrivileges(AccessState, NewPrivilegeSet); ok(Status == STATUS_SUCCESS, "SeAppendPrivileges failed\n"); - ok((AuxData->PrivilegeSet->PrivilegeCount == 2),"PrivelegeCount must be 2, but it is %d\n", AuxData->PrivilegeSet->PrivilegeCount); - ExFreePool(NewPrivilegeSet); + ok_eq_ulong(AuxData->PrivilegesUsed->PrivilegeCount, InitialPrivilegeCount + 1); + ExFreePoolWithTag(NewPrivilegeSet, 'QSmK'); //----------------------------------------------------------------// // Testing SeAppendPrivileges. Must change PrivilegeCount to 6 (2 + 4) - NewPrivilegeSet = ExAllocatePool(PagedPool, 4*sizeof(PRIVILEGE_SET)); + NewPrivilegeSet = ExAllocatePoolZero(PagedPool, + FIELD_OFFSET(PRIVILEGE_SET, Privilege[4]), + 'QSmK'); NewPrivilegeSet->PrivilegeCount = 4; Status = SeAppendPrivileges(AccessState, NewPrivilegeSet); ok(Status == STATUS_SUCCESS, "SeAppendPrivileges failed\n"); - ok((AuxData->PrivilegeSet->PrivilegeCount == 6),"PrivelegeCount must be 6, but it is %d\n", AuxData->PrivilegeSet->PrivilegeCount); - ExFreePool(NewPrivilegeSet); + ok_eq_ulong(AuxData->PrivilegesUsed->PrivilegeCount, InitialPrivilegeCount + 5); + ExFreePoolWithTag(NewPrivilegeSet, 'QSmK'); //----------------------------------------------------------------// // Testing SePrivilegeCheck // //----------------------------------------------------------------// // KPROCESSOR_MODE is set to KernelMode ===> Always return TRUE - ok(SePrivilegeCheck(AuxData->PrivilegeSet, &(AccessState->SubjectSecurityContext), KernelMode), "SePrivilegeCheck failed with KernelMode mode arg\n"); + ok(SePrivilegeCheck(AuxData->PrivilegesUsed, &(AccessState->SubjectSecurityContext), KernelMode), "SePrivilegeCheck failed with KernelMode mode arg\n"); // and call it again - ok(SePrivilegeCheck(AuxData->PrivilegeSet, &(AccessState->SubjectSecurityContext), KernelMode), "SePrivilegeCheck failed with KernelMode mode arg\n"); + ok(SePrivilegeCheck(AuxData->PrivilegesUsed, &(AccessState->SubjectSecurityContext), KernelMode), "SePrivilegeCheck failed with KernelMode mode arg\n"); //----------------------------------------------------------------// // KPROCESSOR_MODE is set to UserMode. Expect false - ok(!SePrivilegeCheck(AuxData->PrivilegeSet, &(AccessState->SubjectSecurityContext), UserMode), "SePrivilegeCheck unexpected success with UserMode arg\n"); + ok(!SePrivilegeCheck(AuxData->PrivilegesUsed, &(AccessState->SubjectSecurityContext), UserMode), "SePrivilegeCheck unexpected success with UserMode arg\n"); //----------------------------------------------------------------// @@ -311,6 +328,10 @@ START_TEST(SeQueryInfoToken) // Testing SeFreePrivileges // //----------------------------------------------------------------// + // FIXME: KernelMode will automatically get all access granted without + // getting Privileges filled in. For UserMode, Privileges will only get + // filled if either WRITE_OWNER or ACCESS_SYSTEM_SECURITY is requested + // and granted. So this doesn't really test SeFreePrivileges. Privileges = NULL; Checker = SeAccessCheck( AccessState->SecurityDescriptor, @@ -319,17 +340,17 @@ START_TEST(SeQueryInfoToken) AccessState->OriginalDesiredAccess, AccessState->PreviouslyGrantedAccess, &Privileges, - (PGENERIC_MAPPING)((PCHAR*)PsProcessType + 52), + &ProcessGenericMapping, KernelMode, &AccessMask, &Status ); ok(Checker, "Checker is NULL\n"); - ok((Privileges != NULL), "Privileges is NULL\n"); + ok(Privileges == NULL, "Privileges is not NULL\n"); if (Privileges) { - trace("AuxData->PrivilegeSet->PrivilegeCount = %d ; Privileges->PrivilegeCount = %d\n", - AuxData->PrivilegeSet->PrivilegeCount, Privileges->PrivilegeCount); + trace("AuxData->PrivilegesUsed->PrivilegeCount = %d ; Privileges->PrivilegeCount = %d\n", + AuxData->PrivilegesUsed->PrivilegeCount, Privileges->PrivilegeCount); } if (Privileges) SeFreePrivileges(Privileges); @@ -352,25 +373,29 @@ START_TEST(SeQueryInfoToken) TPrivileges = (PTOKEN_PRIVILEGES)(Buffer); //trace("TPCount = %u\n\n", TPrivileges->PrivilegeCount); - NewPrivilegeSet = ExAllocatePool(PagedPool, 14*sizeof(PRIVILEGE_SET)); + NewPrivilegeSet = ExAllocatePoolZero(PagedPool, + FIELD_OFFSET(PRIVILEGE_SET, Privilege[14]), + 'QSmK'); NewPrivilegeSet->PrivilegeCount = 14; ok((SeAppendPrivileges(AccessState, NewPrivilegeSet)) == STATUS_SUCCESS, "SeAppendPrivileges failed\n"); - ok((AuxData->PrivilegeSet->PrivilegeCount == 20),"PrivelegeCount must be 20, but it is %d\n", AuxData->PrivilegeSet->PrivilegeCount); - ExFreePool(NewPrivilegeSet); - for (i = 0; i < AuxData->PrivilegeSet->PrivilegeCount; i++) + ok_eq_ulong(AuxData->PrivilegesUsed->PrivilegeCount, InitialPrivilegeCount + 19); + ExFreePoolWithTag(NewPrivilegeSet, 'QSmK'); + for (i = 0; i < AuxData->PrivilegesUsed->PrivilegeCount; i++) { - AuxData->PrivilegeSet->Privilege[i].Attributes = TPrivileges->Privileges[i].Attributes; - AuxData->PrivilegeSet->Privilege[i].Luid = TPrivileges->Privileges[i].Luid; + AuxData->PrivilegesUsed->Privilege[i].Attributes = TPrivileges->Privileges[i].Attributes; + AuxData->PrivilegesUsed->Privilege[i].Luid = TPrivileges->Privileges[i].Luid; } - //trace("AccessState->privCount = %u\n\n", ((PAUX_ACCESS_DATA)(AccessState->AuxData))->PrivilegeSet->PrivilegeCount); + //trace("AccessState->privCount = %u\n\n", ((PAUX_ACCESS_DATA)(AccessState->AuxData))->PrivilegesUsed->PrivilegeCount); - ok(SePrivilegeCheck(AuxData->PrivilegeSet, &(AccessState->SubjectSecurityContext), UserMode), "SePrivilegeCheck fails in UserMode, but I wish it will success\n"); + ok(SePrivilegeCheck(AuxData->PrivilegesUsed, &(AccessState->SubjectSecurityContext), UserMode), "SePrivilegeCheck fails in UserMode, but I wish it will success\n"); } } // Call SeFreePrivileges again + // FIXME: See other SeAccessCheck call above, we're not really testing + // SeFreePrivileges here. Privileges = NULL; Checker = SeAccessCheck( AccessState->SecurityDescriptor, @@ -379,17 +404,17 @@ START_TEST(SeQueryInfoToken) AccessState->OriginalDesiredAccess, AccessState->PreviouslyGrantedAccess, &Privileges, - (PGENERIC_MAPPING)((PCHAR*)PsProcessType + 52), + &ProcessGenericMapping, KernelMode, &AccessMask, &Status ); ok(Checker, "Checker is NULL\n"); - ok((Privileges != NULL), "Privileges is NULL\n"); + ok(Privileges == NULL, "Privileges is not NULL\n"); if (Privileges) { - trace("AuxData->PrivilegeSet->PrivilegeCount = %d ; Privileges->PrivilegeCount = %d\n", - AuxData->PrivilegeSet->PrivilegeCount, Privileges->PrivilegeCount); + trace("AuxData->PrivilegesUsed->PrivilegeCount = %d ; Privileges->PrivilegeCount = %d\n", + AuxData->PrivilegesUsed->PrivilegeCount, Privileges->PrivilegeCount); } if (Privileges) SeFreePrivileges(Privileges); @@ -402,9 +427,7 @@ START_TEST(SeQueryInfoToken) SeDeleteAccessState(AccessState); - if (GenericMapping) ExFreePool(GenericMapping); - if (PsProcessType) ExFreePool(PsProcessType); if (SubjectContext) ExFreePool(SubjectContext); - if (AuxData) ExFreePool(AuxData); + if (AuxData) ExFreePoolWithTag(AuxData, 'QSmK'); if (AccessState) ExFreePool(AccessState); } diff --git a/ntoskrnl/ob/obhandle.c b/ntoskrnl/ob/obhandle.c index 28a2fc77e36..530e32fbfaf 100644 --- a/ntoskrnl/ob/obhandle.c +++ b/ntoskrnl/ob/obhandle.c @@ -1647,8 +1647,8 @@ ObpCreateHandle(IN OB_OPEN_REASON OpenReason, if (OpenReason == ObCreateHandle) { /* Check if we need to audit the privileges */ - if ((AuxData->PrivilegeSet) && - (AuxData->PrivilegeSet->PrivilegeCount)) + if ((AuxData->PrivilegesUsed) && + (AuxData->PrivilegesUsed->PrivilegeCount)) { /* Do the audit */ #if 0 @@ -1656,7 +1656,7 @@ ObpCreateHandle(IN OB_OPEN_REASON OpenReason, &AccessState-> SubjectSecurityContext, GrantedAccess, - AuxData->PrivilegeSet, + AuxData->PrivilegesUsed, TRUE, ExGetPreviousMode()); #endif diff --git a/ntoskrnl/se/access.c b/ntoskrnl/se/access.c index d9eacc550c7..7a9eedeba66 100644 --- a/ntoskrnl/se/access.c +++ b/ntoskrnl/se/access.c @@ -23,10 +23,10 @@ * @param[in] Process * Valid process object where subject context is to be captured. * - * @param[in,out] AccessState + * @param[out] AccessState * An initialized returned parameter to an access state. * - * @param[in] AuxData + * @param[out] AuxData * Auxiliary security data for access state. * * @param[in] Access @@ -43,8 +43,8 @@ NTAPI SeCreateAccessStateEx( _In_ PETHREAD Thread, _In_ PEPROCESS Process, - _Inout_ PACCESS_STATE AccessState, - _In_ PAUX_ACCESS_DATA AuxData, + _Out_ PACCESS_STATE AccessState, + _Out_ __drv_aliasesMem PAUX_ACCESS_DATA AuxData, _In_ ACCESS_MASK Access, _In_ PGENERIC_MAPPING GenericMapping) { @@ -88,7 +88,7 @@ SeCreateAccessStateEx( } /* Set the Auxiliary Data */ - AuxData->PrivilegeSet = (PPRIVILEGE_SET)((ULONG_PTR)AccessState + + AuxData->PrivilegesUsed = (PPRIVILEGE_SET)((ULONG_PTR)AccessState + FIELD_OFFSET(ACCESS_STATE, Privileges)); if (GenericMapping) AuxData->GenericMapping = *GenericMapping; @@ -101,10 +101,10 @@ SeCreateAccessStateEx( * @brief * Creates an access state. * - * @param[in,out] AccessState + * @param[out] AccessState * An initialized returned parameter to an access state. * - * @param[in] AuxData + * @param[out] AuxData * Auxiliary security data for access state. * * @param[in] Access @@ -119,8 +119,8 @@ SeCreateAccessStateEx( NTSTATUS NTAPI SeCreateAccessState( - _Inout_ PACCESS_STATE AccessState, - _In_ PAUX_ACCESS_DATA AuxData, + _Out_ PACCESS_STATE AccessState, + _Out_ __drv_aliasesMem PAUX_ACCESS_DATA AuxData, _In_ ACCESS_MASK Access, _In_ PGENERIC_MAPPING GenericMapping) { @@ -158,7 +158,7 @@ SeDeleteAccessState( /* Deallocate Privileges */ if (AccessState->PrivilegesAllocated) - ExFreePoolWithTag(AuxData->PrivilegeSet, TAG_PRIVILEGE_SET); + ExFreePoolWithTag(AuxData->PrivilegesUsed, TAG_PRIVILEGE_SET); /* Deallocate Name and Type Name */ if (AccessState->ObjectName.Buffer) diff --git a/ntoskrnl/se/priv.c b/ntoskrnl/se/priv.c index a147d2bd2b7..c7f284d293d 100644 --- a/ntoskrnl/se/priv.c +++ b/ntoskrnl/se/priv.c @@ -601,9 +601,9 @@ SeAppendPrivileges( /* Calculate the size of the old privilege set */ OldPrivilegeSetSize = sizeof(PRIVILEGE_SET) + - (AuxData->PrivilegeSet->PrivilegeCount - 1) * sizeof(LUID_AND_ATTRIBUTES); + (AuxData->PrivilegesUsed->PrivilegeCount - 1) * sizeof(LUID_AND_ATTRIBUTES); - if (AuxData->PrivilegeSet->PrivilegeCount + + if (AuxData->PrivilegesUsed->PrivilegeCount + Privileges->PrivilegeCount > INITIAL_PRIVILEGE_COUNT) { /* Calculate the size of the new privilege set */ @@ -619,7 +619,7 @@ SeAppendPrivileges( /* Copy original privileges from the acess state */ RtlCopyMemory(PrivilegeSet, - AuxData->PrivilegeSet, + AuxData->PrivilegesUsed, OldPrivilegeSetSize); /* Append privileges from the privilege set*/ @@ -632,23 +632,23 @@ SeAppendPrivileges( /* Free the old privilege set if it was allocated */ if (AccessState->PrivilegesAllocated != FALSE) - ExFreePoolWithTag(AuxData->PrivilegeSet, TAG_PRIVILEGE_SET); + ExFreePoolWithTag(AuxData->PrivilegesUsed, TAG_PRIVILEGE_SET); /* Now we are using an allocated privilege set */ AccessState->PrivilegesAllocated = TRUE; /* Assign the new privileges to the access state */ - AuxData->PrivilegeSet = PrivilegeSet; + AuxData->PrivilegesUsed = PrivilegeSet; } else { /* Append privileges */ - RtlCopyMemory((PVOID)((ULONG_PTR)AuxData->PrivilegeSet + OldPrivilegeSetSize), + RtlCopyMemory((PVOID)((ULONG_PTR)AuxData->PrivilegesUsed + OldPrivilegeSetSize), (PVOID)((ULONG_PTR)Privileges + sizeof(PRIVILEGE_SET) - sizeof(LUID_AND_ATTRIBUTES)), Privileges->PrivilegeCount * sizeof(LUID_AND_ATTRIBUTES)); /* Adjust the number of privileges in the target privilege set */ - AuxData->PrivilegeSet->PrivilegeCount += Privileges->PrivilegeCount; + AuxData->PrivilegesUsed->PrivilegeCount += Privileges->PrivilegeCount; } return STATUS_SUCCESS; diff --git a/sdk/include/ndk/setypes.h b/sdk/include/ndk/setypes.h index 1f54e097318..d9fe4c4f9b6 100644 --- a/sdk/include/ndk/setypes.h +++ b/sdk/include/ndk/setypes.h @@ -255,9 +255,24 @@ typedef struct _TOKEN typedef struct _AUX_ACCESS_DATA { - PPRIVILEGE_SET PrivilegeSet; + PPRIVILEGE_SET PrivilegesUsed; GENERIC_MAPPING GenericMapping; - ULONG Reserved; + ACCESS_MASK AccessesToAudit; + ACCESS_MASK MaximumAuditMask; +#if (NTDDI_VERSION >= NTDDI_LONGHORN) + GUID TransactionId; +#endif +#if (NTDDI_VERSION >= NTDDI_WIN7) + PVOID NewSecurityDescriptor; + PVOID ExistingSecurityDescriptor; + PVOID ParentSecurityDescriptor; + VOID (NTAPI *DerefSecurityDescriptor)(PVOID, PVOID); + PVOID SDLock; + ACCESS_REASONS AccessReasons; +#endif +#if (NTDDI_VERSION >= NTDDI_WIN8) + BOOLEAN GenerateStagingEvents; +#endif } AUX_ACCESS_DATA, *PAUX_ACCESS_DATA; //