Closed Bug 1344034 (CVE-2017-7782) Opened 5 years ago Closed 5 years ago

A single RWX page is getting allocated on Windows

Categories

(Core :: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 55+ fixed
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: arthur, Assigned: arthur)

References

Details

(Keywords: csectype-other, sec-moderate, Whiteboard: [adv-main55+][adv-esr52.3+])

Attachments

(2 files, 1 obsolete file)

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.
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!
Flags: needinfo?(aklotz)
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
Flags: needinfo?(aklotz)
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
Assignee: nobody → arthuredelstein
Attachment #8876266 - Flags: review?(dmajor)
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.
Attachment #8876266 - Flags: review?(dmajor) → review+
(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.)
Attachment #8876266 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8876319 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc20d9ee0ef9
Auto-enforce W^X for WindowsDllInterceptor hook pages. r=dmajor
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cc20d9ee0ef9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Attachment #8876508 - Flags: approval-mozilla-esr52?
(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.
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.
Flags: sec-bounty?
[Tracking Requested - why for this release]: see comment 11.
Flags: sec-bounty? → sec-bounty+
Summary: A single RWX page is getting allocated on Windows. → A single RWX page is getting allocated on Windows
Whiteboard: [adv-main55+]
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.
Attachment #8876508 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main55+] → [adv-main55+][adv-esr52.3+]
Alias: CVE-2017-7782
You need to log in before you can comment on or make changes to this bug.