1372849 - (CVE-2017-7804) WindowsDllDetourPatcher Destructor Exploit Primitive

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Stephen Fewer]

WindowsDllDetourPatcher Destructor Exploit Primitive

The destructor function for the WindowsDllDetourPatcher class can be re-purposed by an attacker during the exploitation of a memory corruption exploit to force attacker controlled data to be written to an attacker controlled location in memory. Furthermore the destructor function will modify the protection of the memory being written-to, to be writable, before reverting it back to the original protection. This allows an attacker to write arbitrary code to an existing code section before executing it. This issue was found as part of a security review and a working exploit using this technique was developed against FF 52.0 64-Bit on Windows 10.0.15063 (See magic_selfrando_pdatascan1.html in https://bugzilla.mozilla.org/show_bug.cgi?id=1371002).

The purpose of the ~WindowsDllDetourPatcher function is to remove existing hooks in place by overwriting the destination pointer from the "JMP pointer" (x86) or "MOV R11, pointer; JMP R11" (x64) code bytes previously written by CreateTrampoline back to the address of the trampoline, which effectively removes the hook. The mHookPage member variable is a pointer to a buffer that holds several items for every hook that has been added; including a pointer to the original location where the hook was inserted and the trampoline code itself.

An attacker with a read/write primitive can discover the location of the mozilla::internal::WindowsDllDetourPatcher::~WindowsDllDetourPatcher function and modify an existing object in heap memory in order to change a vftable entry to allow the ~WindowsDllDetourPatcher function to be called. When ~WindowsDllDetourPatcher is called, it will use member variables stored in the corrupted object and thus the attacker can control the mHookPage and mCurHooks member variables.

Note: This issue is separate from https://bugzilla.mozilla.org/show_bug.cgi?id=1344034 as the attacker can set the mHookPage pointer to point to a RW buffer of their choosing. No existing hooks will be in place (as we forge a new WindowsDllDetourPatcher instance) so this buffer never needs to be executable.

The attacker can then specify a target location to write to (for example a location in a RX .text section is a good candidate). The data written is an address within the mHookPage buffer (what would be the location of the trampoline code). As the attacker can control this address, the attacker can choose an arbitrary low byte when setting the address in order to force a byte of attacker controlled data to be written to the target address. This operation can be repeated multiple times to allow an arbitrary sequence of bytes to be written.

The above technique can be used during any memory corruption exploit where an attacker has a read/write primitive. In order to harden the ~WindowsDllDetourPatcher function from being re-purposed by an attacker, several options are available:

	1. Encode the origBytes pointers held in the mHookPage buffer using a secure technique like EncodePointer/DecodePointer (which use a secret held in the kernel to avoid an attacker leaking it). Note: This API is available on XPSP2 and up.
	
	2. Ensure the byte(s) preceding the location being written to are the expected ones. As the write replaces the dest pointer for a jmp, we can always expect the preceding byte on x86 to be 0xE9 for a "JMP ptr" instruction and on x64 it can be expected to be 0x49, 0xBB for a "MOV R11, ptr; JMP R11" sequence. If the expected bytes are not found the process should __fastfail.

	3. Enable CFG as ~WindowsDllDetourPatcher would not be an indirect call target as the class (or the destructor) is not virtual, so methods are called directly and not indirectly via a vftable. This would prevent an attacker calling the destructor via a corrupted object.

	4. Enable immutable code pages on systems that support it, e.g. on Windows 10, this is available in the form of Arbitrary Code Guard (ACG). However this is a non trivial change which will probably require out of process JIT engine, out of process hooking and so on.
	
Options 1 and 2 are the best short term solutions, with 3 the best mid term and 4 the best long term solution.

Comment 1

[Stephen Fewer]

Attachment: 1372849.patch

Attached is a patch for consideration. It implements options 1 and 2 as previously mentioned. I have verified that this breaks the exploit I developed. I chose to use 'continue;' in the destructors for loop if an unexpected byte sequence was seen rather that use __fastfail() as I couldn't see any evidence anywhere else in the code base that you use fast failing for these type of conditions.

Comment 2

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: 1372849.patch

The attached is a patch that applied against -central and can be imported.

Stephen, I wrote the patch in your name with a commit message - I'm flagging you for ni to confirm you're okay with your name being attached to the patch and message.

Comment 3

[Stephen Fewer]

Yes that's fine to use may name, cheers :)

Comment 4

[(Away)]

Thanks for the great writeup, Stephen! I support both of the approaches in the patch.

Regarding the GetProcAddress: is there any reason not to link against EncodePointer/DecodePointer directly? We haven't supported anything below XPSP2 for 4+ years.

Also I'm wondering if we need to re-word the patch description per the security guidelines to avoid calling attention to the exploit details -- although that may be difficult, since the hidden bug with this code change makes it pretty clear what problem we're patching. :/ I wonder if we should land these as "stability" fixes in a public bug, maybe say to avoid conflicting with other apps that re-hook our same APIs.

Comment 5

[Stephen Fewer]

Thanks David, I didn't realise XPSP1 or below was no longer supported hence why the GetProcAddress. It makes much better sense to link directly to these API's rather than dynamically resolve them if it doesn't affect compatibility.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

LGTM. Style nit re: spaces surrounding parameters to EncodePointer and DecodePointer

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: 1372849.patch

I've updated the patch to remove the indirection and fix the nit. I haven't compiled it (no easy way to do so) but it 'should work'? =)

Comment 8

[(Away)]

Flagging for thoughts about the patch description and comment 4.

Comment 9

[Daniel Veditz [:dveditz]]

Tom: can you run the patch through a try build?

Comment 10

[Daniel Veditz [:dveditz]]

I think we need to keep the in-line comments just to make sense of the code in the future (e.g. "Ensure the JMP from CreateTrampoline is where we expect it to be.") which limits how obscure we can make this. We should drop the details from the commit message, though. A simple "Bug 1372849 Improve the security of WindowsDllDetourPatcher" should be OK, doesn't give away much beyond what's already obvious.

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Daniel Veditz [:dveditz] from comment #9)
> Tom: can you run the patch through a try build?


Yes, duh of course - sorry.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ed3dd6deea30275167fca5f07024183c605701a

Comment 12

[Julien Cristau [:jcristau]]

tracking as sec-high

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Adding Bug 1344034 as a 'See Also' which Stephen identified alongside this one, but was reported earlier by a Tor developer.

Comment 14

[(Away)]

Comment on attachment 8877731 [details] [diff] [review]
1372849.patch

tjr, do you want to fill out the sec-approval on behalf of Stephen?

Comment 15

[(Away)]

Comment on attachment 8877731 [details] [diff] [review]
1372849.patch

Oops

Comment 16

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 8877731 [details] [diff] [review]
1372849.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? 

The patch itself does not fix an immediately exploitable condition (like a UAF or int overflow). But given such a vulnerability, this issue could help build an exploit. The patch makes it very obvious we are hardening this structure to make it more difficult to build exploits with it.


Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yup. Even if edit the commit to "Improve the security of WindowsDllDetourPatcher" as Dan suggested.


Which older supported branches are affected by this flaw?

Lots of them. This code goes back a couple years.


Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Backporting to esr52 won't be difficult. Other than that, no, as 54 was indicated 'wontfix'. 


How likely is this patch to cause regressions; how much testing does it need?

Medium likely, just because we really shouldn't see any problems with adding the assembly check, but I wouldn't be too surprised if there was some crazy case we hadn't considered. Perhaps we should change this from 'continue' to an assert.



If people agree I'll update the patch
 - Use asserts instead of continues to fast-crash
 - Trim the commit message to 'Improve the security of WindowsDllDetourPatcher'


LMK!

Comment 17

[(Away)]

(In reply to Tom Ritter [:tjr] from comment #16)
> If people agree I'll update the patch
>  - Use asserts instead of continues to fast-crash

I think I'm okay with the continue. Most (all?) of our interceptors are statics that stay alive for the entire process. If there's badness happening, an assert wouldn't fire in time to be of much benefit. And it might lead to unactionable crash reports if our hooks collide with some injected AV or something.

>  - Trim the commit message to 'Improve the security of WindowsDllDetourPatcher'

Ok.

Comment 18

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

sec-approval+ for checkin on trunk the week of July 3, three weeks into the current cycle.
We'll want beta and ESR52 patches nominated and approved to go in after it lands on trunk.

Comment 19

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: 1372849.patch

Update patch commit message

Comment 20

[Frederik Braun [:freddy]]

*Nudge*
Is this ready to land?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87cf3117088c9d2f613ebd1f916c1d55b9c91458

This grafts cleanly to Beta but will need a rebased patch for ESR52. And some approval nominations too when you get a chance :)

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87cf3117088c

Comment 23

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: 1372849-esr52.patch

Added patch rebased for esr52

Comment 24

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 8883916 [details] [diff] [review]
1372849-esr52.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is sec-high

User impact if declined: ESR users will be vulnerable to an exploit writing mechanism that is fixed on release.

Fix Landed on Version: 56

Risk to taking this patch (and alternatives if risky): The browser on Windows crashes. I believe this would happen reliably.

String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8883916 [details] [diff] [review]
1372849-esr52.patch

Sec-high, ESR52+

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8879247 [details] [diff] [review]
1372849.patch

Sec-high, Beta55+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/a8bd61fc89cc

Comment 28

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-beta/rev/d231d2ff17b13723fe84c358f11ec4b3fbb9cd07