[PATCH v9 0/3] MR414: fltmgr.sys: Implement FltBuildDefaultSecurityDescriptor
Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> -- v9: 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..9a85f4b6c82 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) +{ + ExFreePool(descriptor); +} 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 | 73 +++++++++++++++++++++++++++++ 2 files changed, 74 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..c04dffbfc60 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,77 @@ 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; + BOOL ret; + + status = FltBuildDefaultSecurityDescriptor(&sd, STANDARD_RIGHTS_ALL); + ok(status == STATUS_SUCCESS || broken(status == STATUS_INVALID_SID), "got %#lx\n", status); + if (status != STATUS_SUCCESS) + { + win_skip("Skipping FltBuildDefaultSecurityDescriptor tests\n"); + return; + } + 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); + + ret = RtlEqualSid(sid1, (PSID)&ace->SidStart); + ok(ret || broken(!ret) /* win7 */, "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); + + ret = RtlEqualSid(sid2, (PSID)&ace->SidStart); + ok(ret || broken(!ret) /* win7 */, "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 +2440,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
On Fri Aug 4 17:20:40 2023 +0000, Zebediah Figura wrote:
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. Based off the test box, it would be a security issues. Two out of three of the win7 fail, and the only one that is passing is using the admin account.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/414#note_42056
participants (2)
-
Alistair Leslie-Hughes -
Alistair Leslie-Hughes (＠alesliehughes)