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.
From 9d12c6c2ed03be97e7c94f3b43bf361293bb6710 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 | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/mingw-w64-crt/crt/pseudo-reloc.c b/mingw-w64-crt/crt/pseudo-reloc.c
index 7709208..6210368 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))
@@ -223,22 +227,14 @@ static void
restore_modified_sections (void)
{
int i;
- MEMORY_BASIC_INFORMATION b;
DWORD oldprot;
for (i = 0; i < maxSections; i++)
{
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
