[Mingw-w64-public] [PATCH] VirtualProtect problem in pseudo-reloc.c

[Arthur D. Edelstein]

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.

[Arthur D. Edelstein]

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

Post by Arthur D. Edelstein
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
706c1000 706d4000 13000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_READ
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,
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
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
"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.

[Arthur D. Edelstein]

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

Post by Arthur D. Edelstein
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

Post by Arthur D. Edelstein
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
706c1000 706d4000 13000 MEM_IMAGE MEM_COMMIT PAGE_EXECUTE_READ
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,
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
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
"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.

[Liu Hao]

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

Done.

[Liu Hao]

Post by Arthur D. Edelstein
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.
(... abridged ...)

I haven't tested your patch, but after a quick view of your patch (from
the other mail) I shall vote for it because that is also what I do in my
own implementation of runtime pseudo relocation, which obtains the size
of memory areas to be unprotected or reprotected by querying the section
table from the PE header, rather than calling `VirtualQuery()` after the
memory is unprotected.

Furthermore, should the sizes of sections be obtained from the PE
header, it is possible to unprotect all sections before performing the
actual relocation, instead of checking whether the address has been
marked writeable for each entry in the reloc table. The `the_secs` table
is then initialized and never modified, eliminating the need to push
elements into it inside `__write_memory()` by calling
`mark_section_writable()`. This not only improvements the performance of
the entire process, but also simplifies the design.