Hi, yes, the patch looks fine to me. Your changes looking fine to me. Nevertheless we should just put this code change just in master, as we need to let people test new code.
Please go ahead and commit to master. Thanks, Kai 2017-07-09 21:25 GMT+02:00 Arthur D. Edelstein <arthuredelst...@gmail.com>: > Turns out I could remove one more line, because a local variable > becomes unused. Here is a revised patch. > > On Sat, Jul 8, 2017 at 11:18 PM, Arthur D. Edelstein > <arthuredelst...@gmail.com> wrote: >> It seems my patch attachment was maybe blocked. Here it is again with >> a .txt extension. >> >> On Sat, Jul 8, 2017 at 11:06 PM, Arthur D. Edelstein >> <arthuredelst...@gmail.com> wrote: >>> Hi, I am proposing a patch here to deal with a problem found in a >>> cross-compiled Tor Browser and Tor Expert Bundle. Using VMMap on >>> Windows, we observed Execute/Read/Write (XRW) and >>> Execute/Copy-on-Write pages (X/COW) in some DLLs and the Tor >>> executable, even though the .text sections for these binaries were >>> marked with the Execute/Read-only (XR) flag. Obviously this is >>> undesirable from a security viewpoint. >>> >>> So I used WinDbg to try to see what was happening. The problem was >>> caused by VirtualProtect calls in pseudo-reloc.c. Here is an example >>> in zlib1.dll (one of the DLLs bundled with tor.exe): >>> >>> Just before VirtualProtect runs, we have the following executable region: >>> >>> 706c1000 706d4000 13000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_READ >>> >>> Then mark_section_writable calls VirtualProtect at 706C1000 to XRW and we >>> get: >>> >>> 706c1000 706d2000 11000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_WRITECOPY >>> 706d2000 706d3000 1000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_READWRITE >>> 706d3000 706d4000 1000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_WRITECOPY >>> >>> It's odd that some pages are XRW (as requested) but some are X/COW. >>> Perhaps using X/COW is a Microsoft strategy to conserve memory. >>> >>> In any case, just before the restore_modified_sections calls >>> VirtualProtect, another area has somehow been converted to XRW, >>> presumably by a copy-on-write event: >>> >>> 706c1000 706cd000 c000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_READWRITE >>> 706cd000 706ce000 1000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_WRITECOPY >>> 706ce000 706d3000 5000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_READWRITE >>> 706d3000 706d4000 1000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_WRITECOPY >>> >>> Finally, after VirtualProtect is called at 706C1000 by >>> restore_modified_sections, we have mostly XR but some undesirable >>> leftover X/COW and XRW pages: >>> >>> 706c1000 706cd000 c000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_READ >>> 706cd000 706ce000 1000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_WRITECOPY >>> 706ce000 706d3000 5000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_READWRITE >>> 706d3000 706d4000 1000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_WRITECOPY >>> >>> Why do we have these leftovers? The function restore_modified_sections >>> is calling VirtualQuery to decide what region of memory to call >>> VirtualProtect on. Unfortunately, VirtualQuery's behavior is not quite >>> ideal for this situation: >>> >>> "The function [VirtualQuery] determines the attributes of the first >>> page in the region and then scans subsequent pages until it scans the >>> entire range of pages or until it encounters a page with a nonmatching >>> set of attributes. The function returns the attributes and the size of >>> the region of pages with matching attributes, in bytes." (From >>> https://msdn.microsoft.com/en-us/library/windows/desktop/aa366902(v=vs.85).aspx) >>> >>> So, because, after mark_section_writable, we have a mixture of XRW and >>> X/COW pages, then in restore_modified_sections, VirtualQuery is given >>> a truncated region that corresponds to only the first stretch of XRW >>> pages, terminating at the first X/COW page. And >>> restore_modified_sections only applies VirtualProtect to the truncated >>> region, leaving some XRW and X/COW pages unrestored. >>> >>> Therefore my patch changes the behavior of pseudo-reloc.c slightly. In >>> mark_section_writable, I propose storing the original bounds of the >>> region that will be VirtualProtect'd to XRW (in >>> the_secs[i].base_address and the_secs[i].region_size). Then these >>> original bounds are used by restore_modified_sections to make sure we >>> apply VirtualProtect to restore the whole modified region. >>> >>> Many thanks to Jonathan Yong and Kai Tietz for helpful discussions. > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Mingw-w64-public mailing list > Mingw-w64-public@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/mingw-w64-public > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
