[PATCH v7 0/3] MR414: fltmgr.sys: Implement FltBuildDefaultSecurityDescriptor
Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> -- v7: ntoskrnl.exe: Add FltBuildDefaultSecurityDescriptor test fltmgr.sys: Create import library fltmgr.sys: Implement FltBuildDefaultSecurityDescriptor https://gitlab.winehq.org/wine/wine/-/merge_requests/414
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/fltmgr.sys/Makefile.in | 1 + dlls/fltmgr.sys/fltmgr.sys.spec | 4 +- dlls/fltmgr.sys/main.c | 75 ++++++++++++++++++++++++++++++++- include/ddk/fltkernel.h | 3 +- 4 files changed, 79 insertions(+), 4 deletions(-) diff --git a/dlls/fltmgr.sys/Makefile.in b/dlls/fltmgr.sys/Makefile.in index ba106a43831..bb1f34b4896 100644 --- a/dlls/fltmgr.sys/Makefile.in +++ b/dlls/fltmgr.sys/Makefile.in @@ -1,5 +1,6 @@ MODULE = fltmgr.sys EXTRADLLFLAGS = -Wl,--subsystem,native +IMPORTS = ntoskrnl C_SRCS = \ main.c diff --git a/dlls/fltmgr.sys/fltmgr.sys.spec b/dlls/fltmgr.sys/fltmgr.sys.spec index 39ce6798178..8943b9f85cf 100644 --- a/dlls/fltmgr.sys/fltmgr.sys.spec +++ b/dlls/fltmgr.sys/fltmgr.sys.spec @@ -10,7 +10,7 @@ @ stub FltAllocatePoolAlignedWithTag @ stub FltAttachVolume @ stub FltAttachVolumeAtAltitude -@ stub FltBuildDefaultSecurityDescriptor +@ stdcall FltBuildDefaultSecurityDescriptor(ptr long) @ stub FltCancelFileOpen @ stub FltCancelIo @ stub FltCbdqDisable @@ -60,7 +60,7 @@ @ stub FltFreeFileLock @ stub FltFreeGenericWorkItem @ stub FltFreePoolAlignedWithTag -@ stub FltFreeSecurityDescriptor +@ stdcall FltFreeSecurityDescriptor(ptr) @ stub FltFsControlFile @ stub FltGetBottomInstance @ stub FltGetContexts diff --git a/dlls/fltmgr.sys/main.c b/dlls/fltmgr.sys/main.c index e1016a4989c..68f242ab8e8 100644 --- a/dlls/fltmgr.sys/main.c +++ b/dlls/fltmgr.sys/main.c @@ -23,7 +23,6 @@ #include "ntstatus.h" #define WIN32_NO_STATUS #include "windef.h" -#include "winbase.h" #include "winternl.h" #include "ddk/fltkernel.h" @@ -93,3 +92,77 @@ void* WINAPI FltGetRoutineAddress(LPCSTR name) return func; } + +NTSTATUS WINAPI FltBuildDefaultSecurityDescriptor(PSECURITY_DESCRIPTOR *descriptor, ACCESS_MASK access) +{ + PACL dacl; + NTSTATUS ret = STATUS_INSUFFICIENT_RESOURCES; + DWORD sid_len; + SID *sid; + SID *sid_system = NULL; + PSECURITY_DESCRIPTOR sec_desc = NULL; + SID_IDENTIFIER_AUTHORITY auth = { SECURITY_NULL_SID_AUTHORITY }; + + *descriptor = NULL; + + sid_len = RtlLengthRequiredSid(2); + sid = ExAllocatePool(PagedPool, sid_len); + if (!sid) + goto done; + RtlInitializeSid(sid, &auth, 2); + sid->SubAuthority[1] = DOMAIN_GROUP_RID_ADMINS; + sid->SubAuthority[0] = SECURITY_BUILTIN_DOMAIN_RID; + + sid_len = RtlLengthRequiredSid(1); + sid_system = ExAllocatePool(PagedPool, sid_len); + if (!sid_system) + goto done; + RtlInitializeSid(sid_system, &auth, 1); + sid_system->SubAuthority[0] = SECURITY_LOCAL_SYSTEM_RID; + + sid_len = SECURITY_DESCRIPTOR_MIN_LENGTH + sizeof(ACL) + + sizeof(ACCESS_ALLOWED_ACE) + RtlLengthSid(sid) + + sizeof(ACCESS_ALLOWED_ACE) + RtlLengthSid(sid_system); + + sec_desc = ExAllocatePool(PagedPool, sid_len); + if (!sec_desc) + { + ret = STATUS_NO_MEMORY; + goto done; + } + + ret = RtlCreateSecurityDescriptor(sec_desc, SECURITY_DESCRIPTOR_REVISION); + if (ret != STATUS_SUCCESS) + goto done; + + dacl = (PACL)((char*)sec_desc + SECURITY_DESCRIPTOR_MIN_LENGTH); + ret = RtlCreateAcl(dacl, sid_len - SECURITY_DESCRIPTOR_MIN_LENGTH, ACL_REVISION); + if (ret != STATUS_SUCCESS) + goto done; + + ret = RtlAddAccessAllowedAce(dacl, ACL_REVISION, access, sid); + if (ret != STATUS_SUCCESS) + goto done; + + ret = RtlAddAccessAllowedAce(dacl, ACL_REVISION, access, sid_system); + if (ret != STATUS_SUCCESS) + goto done; + + ret = RtlSetDaclSecurityDescriptor(sec_desc, 1, dacl, 0); + if (ret == STATUS_SUCCESS) + *descriptor = sec_desc; + +done: + if (ret != STATUS_SUCCESS) + ExFreePool(sec_desc); + + ExFreePool(sid); + ExFreePool(sid_system); + + return ret; +} + +void WINAPI FltFreeSecurityDescriptor(PSECURITY_DESCRIPTOR descriptor) +{ + RtlFreeHeap(GetProcessHeap(), 0, descriptor); +} \ No newline at end of file diff --git a/include/ddk/fltkernel.h b/include/ddk/fltkernel.h index 8ebebfa2e81..9ece0990810 100644 --- a/include/ddk/fltkernel.h +++ b/include/ddk/fltkernel.h @@ -653,7 +653,8 @@ typedef struct _FLT_REGISTRATION PFLT_SECTION_CONFLICT_NOTIFICATION_CALLBACK SectionNotificationCallback; } FLT_REGISTRATION, *PFLT_REGISTRATION; - +NTSTATUS WINAPI FltBuildDefaultSecurityDescriptor(PSECURITY_DESCRIPTOR *, ACCESS_MASK); +void WINAPI FltFreeSecurityDescriptor(PSECURITY_DESCRIPTOR); void* WINAPI FltGetRoutineAddress(LPCSTR name); NTSTATUS WINAPI FltRegisterFilter(PDRIVER_OBJECT, const FLT_REGISTRATION *, PFLT_FILTER *); NTSTATUS WINAPI FltStartFiltering(PFLT_FILTER); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/414
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/fltmgr.sys/Makefile.in | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/fltmgr.sys/Makefile.in b/dlls/fltmgr.sys/Makefile.in index bb1f34b4896..5540df35d6a 100644 --- a/dlls/fltmgr.sys/Makefile.in +++ b/dlls/fltmgr.sys/Makefile.in @@ -1,4 +1,5 @@ MODULE = fltmgr.sys +IMPORTLIB = fltmgr EXTRADLLFLAGS = -Wl,--subsystem,native IMPORTS = ntoskrnl -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/414
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/ntoskrnl.exe/tests/Makefile.in | 2 +- dlls/ntoskrnl.exe/tests/driver.c | 65 +++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/dlls/ntoskrnl.exe/tests/Makefile.in b/dlls/ntoskrnl.exe/tests/Makefile.in index ab1db85adbb..9c89e44e70a 100644 --- a/dlls/ntoskrnl.exe/tests/Makefile.in +++ b/dlls/ntoskrnl.exe/tests/Makefile.in @@ -1,7 +1,7 @@ TESTDLL = ntoskrnl.exe IMPORTS = advapi32 crypt32 newdev setupapi user32 wintrust ws2_32 hid -driver_IMPORTS = winecrt0 ntoskrnl hal +driver_IMPORTS = winecrt0 ntoskrnl hal fltmgr driver_EXTRADLLFLAGS = -nodefaultlibs -nostartfiles -Wl,--subsystem,native driver2_IMPORTS = winecrt0 ntoskrnl hal driver2_EXTRADLLFLAGS = -nodefaultlibs -nostartfiles -Wl,--subsystem,native diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c index bd388a6ec9e..66a5103c47b 100644 --- a/dlls/ntoskrnl.exe/tests/driver.c +++ b/dlls/ntoskrnl.exe/tests/driver.c @@ -32,6 +32,7 @@ #include "ddk/ntddk.h" #include "ddk/ntifs.h" #include "ddk/wdm.h" +#include "ddk/fltkernel.h" #include "driver.h" @@ -2324,6 +2325,69 @@ static void test_driver_object_extension(void) ok(get_obj_ext == NULL, "got %p\n", get_obj_ext); } +static void test_default_security(void) +{ + PSECURITY_DESCRIPTOR sd = NULL; + NTSTATUS status; + PSID group = NULL, owner = NULL; + BOOLEAN isdefault, present; + PACL acl = NULL; + PACCESS_ALLOWED_ACE ace; + SID_IDENTIFIER_AUTHORITY auth = { SECURITY_NULL_SID_AUTHORITY }; + PSID sid1, sid2; + + status = FltBuildDefaultSecurityDescriptor(&sd, STANDARD_RIGHTS_ALL); + ok(status == STATUS_SUCCESS, "got %#lx\n", status); + ok(sd != NULL, "Failed to return descriptor\n"); + + status = RtlGetGroupSecurityDescriptor(sd, &group, &isdefault); + ok(status == STATUS_SUCCESS, "got %#lx\n", status); + ok(group == NULL, "group isn't NULL\n"); + + status = RtlGetOwnerSecurityDescriptor(sd, &owner, &isdefault); + ok(status == STATUS_SUCCESS, "got %#lx\n", status); + ok(owner == NULL, "owner isn't NULL\n"); + + status = RtlGetDaclSecurityDescriptor(sd, &present, &acl, &isdefault); + ok(status == STATUS_SUCCESS, "got %#lx\n", status); + ok(acl != NULL, "acl is NULL\n"); + ok(acl->AceCount == 2, "got %d\n", acl->AceCount); + + sid1 = ExAllocatePool(NonPagedPool, RtlLengthRequiredSid(2)); + RtlInitializeSid(sid1, &auth, 2); + *RtlSubAuthoritySid(sid1, 0) = SECURITY_BUILTIN_DOMAIN_RID; + *RtlSubAuthoritySid(sid1, 1) = DOMAIN_GROUP_RID_ADMINS; + + sid2 = ExAllocatePool(NonPagedPool, RtlLengthRequiredSid(1)); + RtlInitializeSid(sid2, &auth, 1); + *RtlSubAuthoritySid(sid2, 0) = SECURITY_LOCAL_SYSTEM_RID; + + /* SECURITY_BUILTIN_DOMAIN_RID */ + status = RtlGetAce(acl, 0, (void**)&ace); + ok(status == STATUS_SUCCESS, "got %#lx\n", status); + + ok(ace->Header.AceType == ACCESS_ALLOWED_ACE_TYPE, "got %#x\n", ace->Header.AceType); + ok(ace->Header.AceFlags == 0, "got %#x\n", ace->Header.AceFlags); + ok(ace->Mask == STANDARD_RIGHTS_ALL, "got %#lx\n", ace->Mask); + + ok(RtlEqualSid(sid1, (PSID)&ace->SidStart), "SID not equal\n"); + + /* SECURITY_LOCAL_SYSTEM_RID */ + status = RtlGetAce(acl, 1, (void**)&ace); + ok(status == STATUS_SUCCESS, "got %#lx\n", status); + + ok(ace->Header.AceType == ACCESS_ALLOWED_ACE_TYPE, "got %#x\n", ace->Header.AceType); + ok(ace->Header.AceFlags == 0, "got %#x\n", ace->Header.AceFlags); + ok(ace->Mask == STANDARD_RIGHTS_ALL, "got %#lx\n", ace->Mask); + + ok(RtlEqualSid(sid2, (PSID)&ace->SidStart), "SID not equal\n"); + + ExFreePool(sid1); + ExFreePool(sid2); + + FltFreeSecurityDescriptor(sd); +} + static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *stack) { void *buffer = irp->AssociatedIrp.SystemBuffer; @@ -2368,6 +2432,7 @@ static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *st test_process_memory(test_input); test_permanence(); test_driver_object_extension(); + test_default_security(); IoMarkIrpPending(irp); IoQueueWorkItem(work_item, main_test_task, DelayedWorkQueue, irp); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/414
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=135664 Your paranoid android. === w7u_2qxl (32 bit report) === ntoskrnl.exe: driver.c:2373: Test failed: SID not equal driver.c:2383: Test failed: SID not equal === w7u_el (32 bit report) === ntoskrnl.exe: driver.c:2373: Test failed: SID not equal driver.c:2383: Test failed: SID not equal
On Fri Aug 4 09:42:32 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=135664 Your paranoid android. === w7u_2qxl (32 bit report) === ntoskrnl.exe: driver.c:2373: Test failed: SID not equal driver.c:2383: Test failed: SID not equal === w7u_el (32 bit report) === ntoskrnl.exe: driver.c:2373: Test failed: SID not equal driver.c:2383: Test failed: SID not equal ``` Should I just add a Broken for this case? As all the other boxes pass.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/414#note_41320
Should I just add a Broken for this case? As all the other boxes pass.
I'd imagine we should at least find out why it's different first, and determine if the difference is valid or reasonable. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/414#note_41408
participants (4)
-
Alistair Leslie-Hughes -
Alistair Leslie-Hughes (＠alesliehughes)
-
Marvin -
Zebediah Figura (＠zfigura)