1344034 - (CVE-2017-7782) A single RWX page is getting allocated on Windows

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Arthur Edelstein [:arthur]]

On Windows, I frequently see a single RWX ("Execute/Read/Write") 4k block (a single page). Occasionally I see two. This was observed with both Firefox Nightly and Release. This violates Firefox's W^X/DEP protection.

STR:
1. Download VMMap (https://technet.microsoft.com/en-us/sysinternals/vmmap.aspx) and run it.
2. Choose File > Select Process... > Launch and Trace a New Process > Application and select firefox.exe
3. Look at the "Details" block (bottom half of the VMMap window) and sort by "Protection". One or two "Execute/Read/Write" blocks of 4k each are typically visible.

(I have tried to click the "Heap Allocations..." button to examine the RWX page's allocation call stack, but the button remains disabled. I don't know why.)

Desired result:
No "Execute/Read/Write" blocks should be produced.
 
Also, it would be nice to add an automated test that detects such RWX blocks in the future.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Aaron, I was told you might be able to advise on this. Is there a way to tell where this allocation/mapping is happening? Thanks!

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Concurrently, I was pointed at these:

https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/js/src/ctypes/libffi/src/dlmalloc.c#1365
https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/security/sandbox/chromium/sandbox/win/src/Wow64.cc#117
https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/security/sandbox/chromium/sandbox/win/src/interception.cc#414

This may be all, but there may also be other instances.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

On 32-bit builds we actually intercept VM allocation for memory reporting purposes... perhaps you could overload that to look for RWX allocation?

(Of course, this doesn't handle the case where the protection attrs are changed after the fact, but it's something)

https://dxr.mozilla.org/mozilla-central/source/xpcom/base/AvailableMemoryTracker.cpp#138

Comment 4

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1344034-Auto-enforce-W-X-for-WindowsDllIntercept.patch

Using WinDbg, I tracked down the source of the RWX pages. A single RWX page is allocated and assigned to `WindowsDllDetourPatcher::mHookPage`:
https://dxr.mozilla.org/mozilla-central/rev/e61060be36424240058f8bef4c5597f401bc8b7e/xpcom/build/nsWindowsDllInterceptor.h#440

This hook page is unfortunately never reprotected to RX, because   `WindowsDllInterceptor::LockHooks()` isn't getting called anywhere in the codebase where `WindowsDllInterceptor` instances are being used. Instead, in general, the instances and their RWX hook pages hang around for the lifetime of the process.

So here's a patch that changes the allocation of the hook page as RX, and uses `AutoVirtualProtect` to briefly change it to RWX whenever a hook is written and then immediately change it back to RX. This means the `LockHooks()` functions aren't needed at all, so my patch removes them.

I manually tested this patch with WinDbg and VMMap and confirmed that there are no remaining long-lived RWX pages.

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1169064d1ba4&selectedJob=105913557

Comment 5

[(Away)]

Comment on attachment 8876266 [details] [diff] [review]
0001-Bug-1344034-Auto-enforce-W-X-for-WindowsDllIntercept.patch

Review of attachment 8876266 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find!

::: xpcom/build/nsWindowsDllInterceptor.h
@@ +386,5 @@
>  
>    ~WindowsDllDetourPatcher()
>    {
> +    AutoVirtualProtect protectHookPage(mHookPage, mMaxHooks * kHookSize,
> +                                       PAGE_EXECUTE_READWRITE);

Is this AutoVirtualProtect necessary? If I understand correctly, we'll only be writing to the hooked API and not to the trampoline itself.

Comment 6

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1344034-Auto-enforce-W-X-for-WindowsDllIntercept.patch

(In reply to David Major [:dmajor] from comment #5)

Thanks for the review!

> Is this AutoVirtualProtect necessary? If I understand correctly, we'll only
> be writing to the hooked API and not to the trampoline itself.

You're right, it's not needed. I have removed it from this new version of the patch. (Otherwise the patch is the same.)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc20d9ee0ef9
Auto-enforce W^X for WindowsDllInterceptor hook pages. r=dmajor

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cc20d9ee0ef9

Comment 9

[Arthur Edelstein [:arthur]]

Attachment: esr52-Bug-1344034-Auto-enforce-W-X-for-WindowsDllIntercept.patch

Wondering if we could include this patch in ESR52? (Would be helpful for Tor Browser.)

User impact if declined:  W^X violating pages remain.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Nothing I'm aware of. The backport to ESR52 was very straightforward.
String or UUID changes made by this patch: None

Comment 10

[Georg Koppen]

(In reply to Arthur Edelstein [:arthuredelstein] from comment #9)
> Created attachment 8876508 [details] [diff] [review]
> esr52-Bug-1344034-Auto-enforce-W-X-for-WindowsDllIntercept.patch
> 
> Wondering if we could include this patch in ESR52? (Would be helpful for Tor
> Browser.)

This is true. But the more important point IMO is that all the ESR52 Windows users would get the security benefit as well. Otherwise they'd need to wait another couple of months to get it.

Comment 11

[Daniel Veditz [:dveditz]]

as a hole in our exploit mitigation we should give this a security rating. Could be used to store and execute shellcode but would require other bugs to find this one page so sec-moderate sounds about right.

Nominating for a bug bounty at Selena's request.

This missed esr 52.2 but we should get it in for next time.

Comment 12

[Julien Cristau [:jcristau]]

[Tracking Requested - why for this release]: see comment 11.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8876508 [details] [diff] [review]
esr52-Bug-1344034-Auto-enforce-W-X-for-WindowsDllIntercept.patch

Per Dan's suggestion, we want this on ESR52. Approving patch that has been waiting a while for it.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/012e4429ccd9