[PATCH 0/3] MR11001: ntdll, ntoskrnl.exe: account for read-only cookie in loader

[Rose Hellsing (＠axtlos)]

Until now wine tried to write the cookie even if the cookie was located in a read-only section, causing some applications to crash. With this change the loader temporarily marks the memory section as read-write to update the cookie.

The same fix was applied to the ntdll and ntoskrnl.exe loader, as they had pretty much the same set_security_cookie() function.

A test to check for correctness with read-only cookies was also added.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51928

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

[PATCH 1/3] ntdll: account for read-only cookie in loader

[Rose Hellsing]

From: Rose Hellsing <[e-mail]>

Until now wine tried to write the cookie even if the cookie was located in a read-only section, causing some applications to crash. With this change the loader temporarily marks the memory section as read-write to update the cookie.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51928
---
 dlls/ntdll/loader.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c
index b19bf78a456..77d55268abd 100644
--- a/dlls/ntdll/loader.c
+++ b/dlls/ntdll/loader.c
@@ -2105,26 +2105,36 @@ NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name,
 static void set_security_cookie( ULONG_PTR *cookie )
 {
     static ULONG seed;
+    ULONG_PTR new_cookie = 0;
+    SIZE_T size;
+    void *addr;
+    ULONG old_prot;
 
     TRACE( "initializing security cookie %p\n", cookie );
 
     if (!seed) seed = NtGetTickCount() ^ GetCurrentProcessId();
-    for (;;)
-    {
-        if (*cookie == DEFAULT_SECURITY_COOKIE_16)
-            *cookie = RtlRandom( &seed ) >> 16; /* leave the high word clear */
-        else if (*cookie == DEFAULT_SECURITY_COOKIE_32)
-            *cookie = RtlRandom( &seed );
+
+    if (*cookie == DEFAULT_SECURITY_COOKIE_16)
+        new_cookie = RtlRandom( &seed ) >> 16; /* leave the high word clear */
+    else if (*cookie == DEFAULT_SECURITY_COOKIE_32)
+        new_cookie = RtlRandom( &seed );
 #ifdef DEFAULT_SECURITY_COOKIE_64
-        else if (*cookie == DEFAULT_SECURITY_COOKIE_64)
-        {
-            *cookie = RtlRandom( &seed );
-            /* fill up, but keep the highest word clear */
-            *cookie ^= (ULONG_PTR)RtlRandom( &seed ) << 16;
-        }
+    else if (*cookie == DEFAULT_SECURITY_COOKIE_64)
+    {
+        new_cookie = RtlRandom( &seed );
+        /* fill up, but keep the highest word clear */
+        new_cookie ^= (ULONG_PTR)RtlRandom( &seed ) << 16;
+    }
 #endif
-        else
-            break;
+    else
+        return;
+
+    addr = cookie;
+    size = sizeof(*cookie);
+    if (!NtProtectVirtualMemory( NtCurrentProcess(), &addr, &size, PAGE_READWRITE, &old_prot ))
+    {
+        *cookie = new_cookie;
+        NtProtectVirtualMemory( NtCurrentProcess(), &addr, &size, old_prot, &old_prot );
     }
 }
 
-- 
GitLab


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

[PATCH 2/3] ntoskernel.exe: account for read-only cookies

[Rose Hellsing]

From: Rose Hellsing <[e-mail]>

Similiar to the previous read-only cookie change in ntdll, ntoskernel.exe had the same issue, the same fix is applied.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51928
---
 dlls/ntoskrnl.exe/ntoskrnl.c | 37 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
index 015ecec3603..a36c96ccdf5 100644
--- a/dlls/ntoskrnl.exe/ntoskrnl.c
+++ b/dlls/ntoskrnl.exe/ntoskrnl.c
@@ -3912,25 +3912,36 @@ static void update_security_cookie( void *module, IMAGE_NT_HEADERS *nt )
     {
         static ULONG seed;
         ULONG_PTR *cookie = (ULONG_PTR *)cfg->SecurityCookie;
+        ULONG_PTR new_cookie = 0;
+        SIZE_T prot_size;
+        void *prot_addr;
+        ULONG old_prot;
 
         TRACE( "initializing security cookie %p\n", cookie );
 
         if (!seed) seed = NtGetTickCount() ^ GetCurrentProcessId();
-        for (;;)
-        {
-            if (*cookie == DEFAULT_SECURITY_COOKIE_16)
-                *cookie = RtlRandom( &seed ) >> 16; /* leave the high word clear */
-            else if (*cookie == DEFAULT_SECURITY_COOKIE_32)
-                *cookie = RtlRandom( &seed );
+
+        if (*cookie == DEFAULT_SECURITY_COOKIE_16)
+            new_cookie = RtlRandom( &seed ) >> 16; /* leave the high word clear */
+        else if (*cookie == DEFAULT_SECURITY_COOKIE_32)
+            new_cookie = RtlRandom( &seed );
 #ifdef DEFAULT_SECURITY_COOKIE_64
-            else if (*cookie == DEFAULT_SECURITY_COOKIE_64)
-            {
-                *cookie = RtlRandom( &seed );
-                /* fill up, but keep the highest word clear */
-                *cookie ^= (ULONG_PTR)RtlRandom( &seed ) << 16;
-            }
+        else if (*cookie == DEFAULT_SECURITY_COOKIE_64)
+        {
+            new_cookie = RtlRandom( &seed );
+            /* fill up, but keep the highest word clear */
+            new_cookie ^= (ULONG_PTR)RtlRandom( &seed ) << 16;
+        }
 #endif
-            else break;
+        else
+            return;
+
+        prot_addr = cookie;
+        prot_size = sizeof(*cookie);
+        if (!NtProtectVirtualMemory( NtCurrentProcess(), &prot_addr, &prot_size, PAGE_READWRITE, &old_prot ))
+        {
+            *cookie = new_cookie;
+            NtProtectVirtualMemory( NtCurrentProcess(), &prot_addr, &prot_size, old_prot, &old_prot );
         }
     }
 }
-- 
GitLab


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

[PATCH 3/3] kernel32: Add test for read-only cookie in loader

[Rose Hellsing]

From: Rose Hellsing <[e-mail]>

Adds a regression test to verify the validity of the changes done in the ntdll and ntoskrnl.exe loader to allow for read-only cookies.
---
 dlls/kernel32/tests/loader.c | 90 ++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c
index 5bb32a5fd5b..2e28e9bc25f 100644
--- a/dlls/kernel32/tests/loader.c
+++ b/dlls/kernel32/tests/loader.c
@@ -2117,6 +2117,95 @@ static void test_section_access(void)
     }
 }
 
+static void test_security_cookie_readonly(void)
+{
+    /* a PE whose load-config SecurityCookie points into a
+     * read-only section (e.g. .rdata) must still load successfully.  The
+     * loader is expected to temporarily make the page writable, initialize
+     * the cookie and restore the original protection. */
+#ifdef _WIN64
+    static const ULONG_PTR default_cookie = (((ULONG_PTR)0x00002b99 << 32) | 0x2ddfa232);
+#else
+    static const ULONG_PTR default_cookie = 0xbb40e64e;
+#endif
+    IMAGE_NT_HEADERS nt_header;
+    IMAGE_SECTION_HEADER sections[1];
+    BYTE section_data[0x200];
+    IMAGE_LOAD_CONFIG_DIRECTORY *cfg = (IMAGE_LOAD_CONFIG_DIRECTORY *)section_data;
+    const DWORD cookie_offset = 0x100; /* inside .rdata, away from cfg */
+    ULONG_PTR *cookie_slot = (ULONG_PTR *)(section_data + cookie_offset);
+    char dll_name[MAX_PATH];
+    HMODULE hlib;
+    MEMORY_BASIC_INFORMATION info;
+    SIZE_T size;
+    ULONG_PTR final_cookie;
+
+    memset(section_data, 0, sizeof(section_data));
+
+    nt_header = nt_header_template;
+    nt_header.FileHeader.NumberOfSections = 1;
+    /* clear DYNAMIC_BASE so the loader uses the preferred ImageBase */
+    nt_header.OptionalHeader.DllCharacteristics &= ~IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE;
+    nt_header.OptionalHeader.SectionAlignment = page_size;
+    nt_header.OptionalHeader.FileAlignment = 0x200;
+    nt_header.OptionalHeader.SizeOfImage = page_size * 2;
+    nt_header.OptionalHeader.SizeOfHeaders = nt_header.OptionalHeader.FileAlignment;
+
+    memset(sections, 0, sizeof(sections));
+    memcpy(sections[0].Name, ".rdata", 7);
+    sections[0].Misc.VirtualSize = sizeof(section_data);
+    sections[0].VirtualAddress = page_size;
+    sections[0].SizeOfRawData = sizeof(section_data);
+    sections[0].PointerToRawData = nt_header.OptionalHeader.FileAlignment;
+    /* READ only, so no IMAGE_SCN_MEM_WRITE */
+    sections[0].Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ;
+
+    /* place the cookie inside .rdata so the loader decides to write it */
+    *cookie_slot = default_cookie;
+
+    /* fill in the load-config directory at the start of .rdata */
+    cfg->Size = sizeof(*cfg);
+    cfg->SecurityCookie = (ULONG_PTR)nt_header.OptionalHeader.ImageBase
+                          + sections[0].VirtualAddress + cookie_offset;
+
+    nt_header.OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_LOAD_CONFIG].VirtualAddress = sections[0].VirtualAddress;
+    nt_header.OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_LOAD_CONFIG].Size = sizeof(*cfg);
+
+    create_test_dll_sections(&dos_header, &nt_header, sections, section_data, dll_name);
+
+    SetLastError(0xdeadbeef);
+    hlib = LoadLibraryA(dll_name);
+    ok(hlib != NULL, "LoadLibrary failed err %lu\n", GetLastError());
+    if (!hlib)
+    {
+        DeleteFileA(dll_name);
+        return;
+    }
+
+    if ((ULONG_PTR)hlib != nt_header.OptionalHeader.ImageBase)
+    {
+        skip("DLL was relocated (%p vs preferred %p); cannot verify cookie\n",
+             hlib, (void *)(ULONG_PTR)nt_header.OptionalHeader.ImageBase);
+        FreeLibrary(hlib);
+        DeleteFileA(dll_name);
+        return;
+    }
+
+    final_cookie = *(ULONG_PTR *)((char *)hlib + sections[0].VirtualAddress + cookie_offset);
+    ok(final_cookie != default_cookie,
+       "security cookie was not initialized (still %#Ix)\n", final_cookie);
+
+    /* page protection should be restored to PAGE_READONLY after cookie init */
+    size = VirtualQuery((char *)hlib + sections[0].VirtualAddress, &info, sizeof(info));
+    ok(size == sizeof(info), "VirtualQuery error %lu\n", GetLastError());
+    ok(info.Protect == PAGE_READONLY,
+       "section protection not restored: got %#lx, expected PAGE_READONLY\n",
+       info.Protect);
+
+    FreeLibrary(hlib);
+    DeleteFileA(dll_name);
+}
+
 static void check_tls_index(HANDLE dll, BOOL tls_initialized)
 {
     BOOL found_dll = FALSE;
@@ -4913,6 +5002,7 @@ START_TEST(loader)
     test_ResolveDelayLoadedAPI();
     test_ImportDescriptors();
     test_section_access();
+    test_security_cookie_readonly();
     test_import_resolution();
     test_export_forwarder_dep_chain();
     test_ExitProcess();
-- 
GitLab

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