[PATCH v4 0/3] MR11001: ntdll, ntoskrnl.exe: account for read-only cookie in loader
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 -- v4: kernel32: Add test for read-only cookie in loader ntoskernel.exe: account for read-only cookies ntdll: account for read-only cookie in loader https://gitlab.winehq.org/wine/wine/-/merge_requests/11001
From: Rose Hellsing <rose@pinkro.se> 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 | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c index b19bf78a456..443b9841170 100644 --- a/dlls/ntdll/loader.c +++ b/dlls/ntdll/loader.c @@ -2105,27 +2105,42 @@ NTSTATUS WINAPI LdrGetProcedureAddress(HMODULE module, const ANSI_STRING *name, static void set_security_cookie( ULONG_PTR *cookie ) { static ULONG seed; + ULONG_PTR new_cookie; + SIZE_T size; + void *addr; + ULONG old_prot; TRACE( "initializing security cookie %p\n", cookie ); if (!seed) seed = NtGetTickCount() ^ GetCurrentProcessId(); + new_cookie = *cookie; 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 (new_cookie == DEFAULT_SECURITY_COOKIE_16) + new_cookie = RtlRandom( &seed ) >> 16; /* leave the high word clear */ + else if (new_cookie == DEFAULT_SECURITY_COOKIE_32) + new_cookie = RtlRandom( &seed ); #ifdef DEFAULT_SECURITY_COOKIE_64 - else if (*cookie == DEFAULT_SECURITY_COOKIE_64) + else if (new_cookie == DEFAULT_SECURITY_COOKIE_64) { - *cookie = RtlRandom( &seed ); + new_cookie = RtlRandom( &seed ); /* fill up, but keep the highest word clear */ - *cookie ^= (ULONG_PTR)RtlRandom( &seed ) << 16; + new_cookie ^= (ULONG_PTR)RtlRandom( &seed ) << 16; } #endif else break; } + + if (new_cookie == *cookie) return; /* already initialized */ + + 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
From: Rose Hellsing <rose@pinkro.se> 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 | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c index 015ecec3603..bf6554debe1 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/ntoskrnl.c @@ -3912,25 +3912,42 @@ 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; + SIZE_T prot_size; + void *prot_addr; + ULONG old_prot; TRACE( "initializing security cookie %p\n", cookie ); if (!seed) seed = NtGetTickCount() ^ GetCurrentProcessId(); + + new_cookie = *cookie; 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 (new_cookie == DEFAULT_SECURITY_COOKIE_16) + new_cookie = RtlRandom( &seed ) >> 16; /* leave the high word clear */ + else if (new_cookie == DEFAULT_SECURITY_COOKIE_32) + new_cookie = RtlRandom( &seed ); #ifdef DEFAULT_SECURITY_COOKIE_64 - else if (*cookie == DEFAULT_SECURITY_COOKIE_64) + else if (new_cookie == DEFAULT_SECURITY_COOKIE_64) { - *cookie = RtlRandom( &seed ); + new_cookie = RtlRandom( &seed ); /* fill up, but keep the highest word clear */ - *cookie ^= (ULONG_PTR)RtlRandom( &seed ) << 16; + new_cookie ^= (ULONG_PTR)RtlRandom( &seed ) << 16; } #endif - else break; + else + break; + } + + if (new_cookie == *cookie) return; /* already initialized */ + + 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
From: Rose Hellsing <rose@pinkro.se> 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
On Wed Jun 3 18:57:43 2026 +0000, Alexandre Julliard wrote:
Sure, but low is not zero. Some apps crash when the cookie is not set. Makes sense, I pushed a new version of the patch that preserves the loop.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11001#note_142128
Alexandre Julliard (@julliard) commented about dlls/kernel32/tests/loader.c:
+ + 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); Why do you need to skip here?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11001#note_142130
On Wed Jun 3 20:40:28 2026 +0000, Alexandre Julliard wrote:
Why do you need to skip here? The security cookie field points to an absolute address, so a relocation caused by the address already being in use or ASLR being forced can make the address stale, which would break the tests.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11001#note_142133
On Wed Jun 3 21:06:21 2026 +0000, Rose Hellsing wrote:
The security cookie field points to an absolute address, so a relocation caused by the address already being in use or ASLR being forced can make the address stale, which would break the tests. It doesn't seem hard to account for that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11001#note_142134
On Wed Jun 3 21:28:57 2026 +0000, Alexandre Julliard wrote:
It doesn't seem hard to account for that. My main idea to fix this would be to add a .reloc header for the synthetic PE, but that would add a bit of complexity, if that'd be worth it or there's a better way to do it that I'm not aware of I'd be able to add that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11001#note_142145
participants (3)
-
Alexandre Julliard (＠julliard) -
Rose Hellsing
-
Rose Hellsing (＠axtlos)