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.
From 8fb7a6a801e7df23db56866efe898201d2389f02 Mon Sep 17 00:00:00 2001
From: Arthur Edelstein <arthuredelst...@gmail.com>
Date: Fri, 7 Jul 2017 22:07:34 -0700
Subject: [PATCH] After pseudo relocation, restore memory protection exactly
Before this patch, under some circumstances, pseudo-reloc.c left
the .text section with PAGE_EXECUTE_READWRITE and
PAGE_EXECUTE_WRITECOPY pages that weren't there before.
---
mingw-w64-crt/crt/pseudo-reloc.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/mingw-w64-crt/crt/pseudo-reloc.c b/mingw-w64-crt/crt/pseudo-reloc.c
index 7709208..9eba368 100644
--- a/mingw-w64-crt/crt/pseudo-reloc.c
+++ b/mingw-w64-crt/crt/pseudo-reloc.c
@@ -169,6 +169,8 @@ extern PBYTE _GetPEImageBase (void);
typedef struct sSecInfo {
/* Keeps altered section flags, or zero if nothing was changed. */
DWORD old_protect;
+ PVOID base_address;
+ SIZE_T region_size;
PBYTE sec_start;
PIMAGE_SECTION_HEADER hash;
} sSecInfo;
@@ -209,6 +211,8 @@ mark_section_writable (LPVOID addr)
if (b.Protect != PAGE_EXECUTE_READWRITE && b.Protect != PAGE_READWRITE
&& b.Protect != PAGE_EXECUTE_WRITECOPY && b.Protect != PAGE_WRITECOPY)
{
+ the_secs[i].base_address = b.BaseAddress;
+ the_secs[i].region_size = b.RegionSize;
if (!VirtualProtect (b.BaseAddress, b.RegionSize,
PAGE_EXECUTE_READWRITE,
&the_secs[i].old_protect))
@@ -230,15 +234,8 @@ restore_modified_sections (void)
{
if (the_secs[i].old_protect == 0)
continue;
- if (!VirtualQuery (the_secs[i].sec_start, &b, sizeof(b)))
- {
- __report_error (" VirtualQuery failed for %d bytes at address %p",
- (int) the_secs[i].hash->Misc.VirtualSize,
- the_secs[i].sec_start);
- return;
- }
- VirtualProtect (b.BaseAddress, b.RegionSize, the_secs[i].old_protect,
- &oldprot);
+ VirtualProtect (the_secs[i].base_address, the_secs[i].region_size,
+ the_secs[i].old_protect, &oldprot);
}
}
--
2.1.4
------------------------------------------------------------------------------
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
