Fix LoadLibraryExW hooking crash #89
Conversation
src/os.windows.cpp
Outdated
| if (from_mbi.AllocationBase == virtual_protect_mbi.AllocationBase || | ||
| to_mbi.AllocationBase == virtual_protect_mbi.AllocationBase) { | ||
| new_protect = PAGE_EXECUTE_READWRITE; | ||
| } |
There was a problem hiding this comment.
Can this check be tightened up to test just the page/pages VirtualProtect is in instead of filtering the entire DLL VirtualProtect is in (which is what testing the AllocationBase will do)? I don't even think the VirtualQuery will be necessary in this case. Just take from, align it down to the start of a page, take from+len, align it up to the start of the next page, and see if VirtualProtect to VirtualProtect+14 is in that range.
There was a problem hiding this comment.
Ah my bad- I got mixed up between mbi's AllocationBase and BaseAddress, which I think does what I intended.
I see what you're saying - does that mean doing a GetSystemInfo to determine the page size so as to do the alignment? Sorry, I'm still quite inexperienced at this kind of stuff haha.
There was a problem hiding this comment.
BaseAddress will be the start of the region containing the queried address within the allocated range where all consecutive pages have the same protect. In this case, it would likely be the start of the .text section of the module containing VirtualProtect. So we're back to filtering way more than is strictly necessary, essentially disabling thread trapping for all hooks in kernel32.dll.
You're on the right track with determining the page size and going from there.
|
Let me know if I've done that correctly there, maybe I messed up the comparisons. Also, on my machine VirtualProtect is just 7 bytes (a jump to KERNELBASE) though was allocated 0x20, I'm not sure how much that differs. |
src/os.windows.cpp
Outdated
| if (reinterpret_cast<uint8_t*>(&VirtualProtect) <= align_up(from + len, si.page_size) && | ||
| align_down(from, si.page_size) <= reinterpret_cast<uint8_t*>(&VirtualProtect) + 0x20) { |
There was a problem hiding this comment.
The condition is kind of hard to reason about. I like to give things names to make it clearer.
auto *from_page_start = align_down(from, si.page_size);
auto *from_page_end = align_up(from + len, si.page_size);
auto *vp_start = reinterpret_cast<uint8_t*>(&VirtualProtect);
auto *vp_end = vp_start + 0x20;
if (!(from_page_end < vp_start || vp_end < from_page_start)) {
...There was a problem hiding this comment.
Good idea. On another note: I noticed while testing this that the issue technically also exists if you want to hook any of the undocumented functions on the same page as NtProtectVirtualMemory for obvious reasons (this includes NtMapViewOfSection as mentioned here). I don't personally have a use case for hooking any of that stuff- I'm not sure if it's worth adding yet another check for that, particularly considering the GetProcAddress shenanigans that would be required
Closes #70. Adds check if from/to address is on same page as VirtualProtect, if so use PAGE_EXECUTE_READWRITE (ie no trapping) instead.