[PATCH v8 0/3] MR414: fltmgr.sys: Implement FltBuildDefaultSecurityDescriptor

[Alistair Leslie-Hughes (＠alesliehughes)]

Signed-off-by: Alistair Leslie-Hughes

--
  v8: ntoskrnl.exe: Add FltBuildDefaultSecurityDescriptor test
      fltmgr.sys: Create import library
      fltmgr.sys: Implement FltBuildDefaultSecurityDescriptor

https://gitlab.winehq.org/wine/wine/-/merge_requests/414

[PATCH v8 1/3] fltmgr.sys: Implement FltBuildDefaultSecurityDescriptor

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes

Signed-off-by: Alistair Leslie-Hughes 
---
 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);

[PATCH v8 2/3] fltmgr.sys: Create import library

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes

Signed-off-by: Alistair Leslie-Hughes 
---
 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

[PATCH v8 3/3] ntoskrnl.exe: Add FltBuildDefaultSecurityDescriptor test

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes

Signed-off-by: Alistair Leslie-Hughes 
---
 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);

[PATCH v8 3/3] ntoskrnl.exe: Add FltBuildDefaultSecurityDescriptor test

[Marvin]

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=135713

Your paranoid android.

=== w1064_tsign (64 bit report) ===

ntoskrnl.exe:
ntoskrnl.c:1219: Test failed: Got unexpected ret -1.
ntoskrnl.c:1220: Test failed: Received unexpected data.

[Chip Davis (＠cdavis5e)]

Chip Davis (@cdavis5e) commented about dlls/fltmgr.sys/main.c:
> - 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);

You allocated this above with `ExAllocatePool()`. Shouldn't this then use `ExFreePool()`?