Bug 1344034 (CVE-2017-7782)

A single RWX page is getting allocated on Windows

RESOLVED FIXED in Firefox -esr52



2 years ago
2 years ago


(Reporter: arthur, Assigned: arthur)


({csectype-other, sec-moderate})

csectype-other, sec-moderate
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr5255+ fixed, firefox54 wontfix, firefox55 fixed)


(Whiteboard: [adv-main55+][adv-esr52.3+])


(2 attachments, 1 obsolete attachment)



2 years ago
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.

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)

Flags: needinfo?(aklotz)

Comment 4

2 years ago
Using WinDbg, I tracked down the source of the RWX pages. A single RWX page is allocated and assigned to `WindowsDllDetourPatcher::mHookPage`:

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]

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+

Comment 6

2 years ago
(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


2 years ago
Keywords: checkin-needed
Attachment #8876319 - Flags: review+

Comment 7

2 years ago
Pushed by ryanvm@gmail.com:
Auto-enforce W^X for WindowsDllInterceptor hook pages. r=dmajor
Keywords: checkin-needed
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 9

2 years ago
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?

Comment 10

2 years ago
(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?
Keywords: csectype-other, sec-moderate
See Also: → bug 1372849
[Tracking Requested - why for this release]: see comment 11.
status-firefox54: --- → wontfix
status-firefox-esr52: --- → affected
tracking-firefox-esr52: --- → ?
Flags: sec-bounty? → sec-bounty+
Summary: A single RWX page is getting allocated on Windows. → A single RWX page is getting allocated on Windows
tracking-firefox-esr52: ? → 55+
Whiteboard: [adv-main55+]
Comment on attachment 8876508 [details] [diff] [review]

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+

Comment 14

2 years ago
status-firefox-esr52: affected → fixed
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.