[PATCH v7 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 -- v7: kernel32: Add test 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 | 133 +++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 5bb32a5fd5b..8c0a4e3de06 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -2117,6 +2117,138 @@ 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); + static const WORD reloc_type = IMAGE_REL_BASED_DIR64; +#else + static const ULONG_PTR default_cookie = 0xbb40e64e; + static const WORD reloc_type = IMAGE_REL_BASED_HIGHLOW; +#endif + IMAGE_NT_HEADERS nt_header; + IMAGE_SECTION_HEADER sections[2]; + BYTE rdata[0x200]; + BYTE reloc[0x200]; + IMAGE_LOAD_CONFIG_DIRECTORY *cfg = (IMAGE_LOAD_CONFIG_DIRECTORY *)rdata; + IMAGE_BASE_RELOCATION *base_reloc = (IMAGE_BASE_RELOCATION *)reloc; + WORD *reloc_entries = (WORD *)(reloc + sizeof(*base_reloc)); + const DWORD cookie_offset = 0x100; /* inside .rdata, away from cfg */ + ULONG_PTR *cookie_slot = (ULONG_PTR *)(rdata + cookie_offset); + char temp_path[MAX_PATH], dll_name[MAX_PATH]; + HANDLE hfile; + DWORD dummy; + BOOL ret; + HMODULE hlib; + MEMORY_BASIC_INFORMATION info; + SIZE_T size; + ULONG_PTR final_cookie; + + memset(rdata, 0, sizeof(rdata)); + memset(reloc, 0, sizeof(reloc)); + + nt_header = nt_header_template; + nt_header.FileHeader.NumberOfSections = 2; + nt_header.OptionalHeader.SectionAlignment = page_size; + nt_header.OptionalHeader.FileAlignment = 0x200; + nt_header.OptionalHeader.SizeOfImage = page_size * 3; + nt_header.OptionalHeader.SizeOfHeaders = nt_header.OptionalHeader.FileAlignment; + + memset(sections, 0, sizeof(sections)); + + /* .rdata: holds the load config and the cookie slot, READ only */ + memcpy(sections[0].Name, ".rdata", 7); + sections[0].Misc.VirtualSize = sizeof(rdata); + sections[0].VirtualAddress = page_size; + sections[0].SizeOfRawData = sizeof(rdata); + sections[0].PointerToRawData = nt_header.OptionalHeader.FileAlignment; + sections[0].Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ; + + /* .reloc: fixes up cfg->SecurityCookie when the loader relocates the DLL */ + memcpy(sections[1].Name, ".reloc", 7); + sections[1].Misc.VirtualSize = sizeof(reloc); + sections[1].VirtualAddress = page_size * 2; + sections[1].SizeOfRawData = sizeof(reloc); + sections[1].PointerToRawData = sections[0].PointerToRawData + sections[0].SizeOfRawData; + sections[1].Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA + | IMAGE_SCN_MEM_READ | IMAGE_SCN_MEM_DISCARDABLE; + + /* 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; + + /* one relocation block covering the page that holds cfg->SecurityCookie, + * with a single entry pointing at the SecurityCookie field plus a zero + * terminator entry (WORD-aligned). */ + base_reloc->VirtualAddress = sections[0].VirtualAddress; + base_reloc->SizeOfBlock = sizeof(*base_reloc) + 2 * sizeof(WORD); + reloc_entries[0] = (reloc_type << 12) + | (offsetof(IMAGE_LOAD_CONFIG_DIRECTORY, SecurityCookie) & 0xfff); + reloc_entries[1] = 0; + + 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); + nt_header.OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_BASERELOC].VirtualAddress = sections[1].VirtualAddress; + nt_header.OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_BASERELOC].Size = base_reloc->SizeOfBlock; + + /* hand-roll the DLL file: create_test_dll_sections only supports a single + * shared data blob, but we need distinct payloads for .rdata and .reloc. */ + GetTempPathA(MAX_PATH, temp_path); + GetTempFileNameA(temp_path, "ldr", 0, dll_name); + hfile = CreateFileA(dll_name, GENERIC_WRITE, FILE_SHARE_READ, NULL, CREATE_ALWAYS, 0, 0); + ok(hfile != INVALID_HANDLE_VALUE, "failed to create %s err %lu\n", dll_name, GetLastError()); + if (hfile == INVALID_HANDLE_VALUE) return; + + ret = WriteFile(hfile, &dos_header, sizeof(dos_header), &dummy, NULL); + ok(ret, "WriteFile error %lu\n", GetLastError()); + ret = WriteFile(hfile, &nt_header, + offsetof(IMAGE_NT_HEADERS, OptionalHeader) + nt_header.FileHeader.SizeOfOptionalHeader, + &dummy, NULL); + ok(ret, "WriteFile error %lu\n", GetLastError()); + ret = WriteFile(hfile, sections, sizeof(sections), &dummy, NULL); + ok(ret, "WriteFile error %lu\n", GetLastError()); + + SetFilePointer(hfile, sections[0].PointerToRawData, NULL, FILE_BEGIN); + ret = WriteFile(hfile, rdata, sizeof(rdata), &dummy, NULL); + ok(ret, "WriteFile error %lu\n", GetLastError()); + + SetFilePointer(hfile, sections[1].PointerToRawData, NULL, FILE_BEGIN); + ret = WriteFile(hfile, reloc, sizeof(reloc), &dummy, NULL); + ok(ret, "WriteFile error %lu\n", GetLastError()); + CloseHandle(hfile); + + SetLastError(0xdeadbeef); + hlib = LoadLibraryA(dll_name); + ok(hlib != NULL, "LoadLibrary failed err %lu\n", GetLastError()); + if (!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 +5045,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
participants (2)
-
Rose Hellsing -
Rose Hellsing (＠axtlos)