1599005 - (CVE-2019-17015) Race condition in firefox!sandbox::SharedMemIPCServer::Init leading to relative out-of-bounds read/write in the broker process (Sandbox escape / LPE)

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[Thomas Imbert]

Attachment: race.zip

Hi,

These issues only affects Windows client, I found two race condition in the shared memory of Firefox sandbox implementation.

• Information Disclosure of broker heap address in CopyPolicyToTarget (race described in the next issue)
• Memory corruption (Sandbox escape) in sandbox::SharedMemIPCServer::ThreadPingEventReady (described in this issue)

Vulnerability:

Since Firefox (current version tested 70.0.1) configures the renderer sandbox with USER_LIMITED and Initial Integrity Level == Delayed Integrity Level (INTEGRITY_LEVEL_LOW), any renderer process can interact with other renderers process (read/write/createthread/duplicatehandles) even while new renderer process bootstrapping (when a new tab process is spawned).
Google Chrome is not affected because the renderer processes runs with a USER_LOCKDOWN token (also the initial integrity level = LOW and delayed IL is UNTRUSTED) which prevent access to other renderer processes.

The race condition vulnerability happens during new process bootstrapping, when a new tab process is created, the broker does:
1/ CreateProcess SUSPENDED with the lockdown token (USER_LIMITED for Firefox renderer) and low IL in TargetProcess::Create
2/ Assign target process to JOB object
3/ Change target thread token to initial token (USER_RESTRICTED_SAME_ACCESS) for initialisation later
4/ Create IPCServer shared memory in TargetProcess::Init
5/ DuplicateHandle the shared memory with the target in TargetProcess::Init
6/ Initialise the shared memory in SharedMemIPCServer::Init

So, the target process is suspended but other tabs are not and they can access the target process (thanks to USER_LIMITED token and same IL) and duplicate the IPC shared memory handle.
The shared memory initialisation is vulnerable to race condition when initialising service_context->channel_buffer in SharedMemIPCServer::Init (6):
bool SharedMemIPCServer::Init(void* shared_mem,
uint32_t shared_size,
uint32_t channel_size) {
..
client_control_ = reinterpret_cast<IPCControl*>(shared_mem); // client_control_ points to shared memory
..
for (size_t ix = 0; ix != channel_count; ++ix) {
ChannelControl* client_context = &client_control_->channels[ix]; // client_context points to shared memory
..
client_context->channel_base = base_start; // (A) Write to shared memory (channel_base)
client_context->state = kFreeChannel;
..
service_context->shared_base = reinterpret_cast<char*>(shared_mem);
service_context->channel_size = channel_size;
service_context->channel = client_context;
service_context->channel_buffer =
service_context->shared_base + client_context->channel_base; // (B) Race condition between (A) and (B), read from shared memory (the value client_context->channel_base can be different)
Source code in:
https://github.com/mozilla/gecko-dev/blob/master/security/sandbox/chromium/sandbox/win/src/sharedmem_ipc_server.cc#L122

This vulnerability can lead to relative out-of-bounds read and write in the broker process in sandbox::SharedMemIPCServer::ThreadPingEventReady when the ping event is signaled.
The pointer service_context->channel_buffer is fully controlled (64 bits added offset) due to the race condition.

Crash:

firefox!sandbox::CrossCallParams::GetParamsCount:
00007ff7c6ae4d60 8b4160 mov eax,dword ptr [rcx+60h] ds:424243c6124242a2=????????
0:018> kc

Call Site

00 firefox!sandbox::CrossCallParams::GetParamsCount
01 firefox!sandbox::CrossCallParamsEx::CreateFromBuffer
02 firefox!sandbox::SharedMemIPCServer::InvokeCallback
03 firefox!sandbox::SharedMemIPCServer::ThreadPingEventReady

This exception is a first chance and handled by SEH, the second chance exception is:
firefox!sandbox::SharedMemIPCServer::ThreadPingEventReady+0x8d:
00007ff7c6aff1bd 48894758 mov qword ptr [rdi+58h],rax ds:424243c61242429a=????????????????
during memcpy(call_params->GetCallReturn(), &call_result, sizeof(call_result));

Repro:

I didn't work a lot to make the race reliable so the provided POC (attachment payload_corrupt) rarely triggers the vulnerability.
To reproduce, please debug and break in the broker process between (A) and (B) to help the race on the first subchannel channel_base.
Install Python 2.7 64 bits and PythonForWindows (https://github.com/hakril/PythonForWindows) then run inject.py with the payload_corrupt.dll path.

Exploitability:

The exploitation assumes having code execution in sandboxed renderers processes (RCE -> LPE step targeted).
Since the race condition allow to corrupt the offset of the IPC channel buffer, you can overwrite data near the shared memory base in the broker without requiring an information disclosure vulnerability.

Notice that the shared_base is close to the service_context address which is stored in the broker heap.
dt firefox!sandbox::SharedMemIPCServer::ServerControl 0x00000183d630eec0
+0x018 channel_buffer : 0x424243c612424242 "--- memory read error at address 0x424243c612424242 ---"
+0x020 shared_base : 0x00000183d0000000 "???" +0x028 channel : 0x00000183d0000010 sandbox::ChannelControl
+0x030 dispatcher : 0x00000183`d7f36a00 sandbox::Dispatcher

So, by spraying heap using an exploit convenient object (not researched), you can use this vulnerability to corrupt other heap allocation (craft fake IPC crosscall in the heap and corrupt adjacent allocations) and with some work, achieve code execution in Firefox main process (Sandbox escape).

Thank you

Comment 1

[Thomas Imbert]

I forgot one part: to reproduce, you need to manually start new tabs (ctrl+click on url). The POC could be improved to call IPC CreateWindowInDifferentProcess to trigger tab creation and exploit the race.

Comment 2

[:Gijs (out for now; he/him)]

From a very very casual look as a triage person, this look like it affects chromium code - do you know if Chrome/Chromium is also affected, and if so, have you filed a corresponding ticket in their tracker, and if so, can you link to it so we can coordinate if necessary? Thank you!

Comment 3

[Andrew McCreight [:mccr8]]

Bob, do you know who might be able to take a look at this? Thanks.

Comment 4

[Thomas Imbert]

The vulnerability cannot be triggered in Chrome (it depends on the sandbox configuration):
"Google Chrome is not affected because the renderer processes runs with a USER_LOCKDOWN token (also the initial integrity level = LOW and delayed IL is UNTRUSTED) which prevent access to other renderer processes."

I didn't open a bug in Chromium tracker, would you like me to do it?

Comment 5

[Bob Owen (:bobowen)]

Thanks for these two reports Thomas.

(In reply to Andrew McCreight [:mccr8] from comment #3)

Bob, do you know who might be able to take a look at this? Thanks.

I'll pick both up.

I think the simplest fix, is to change the order in the setup, so that we duplicate the shared memory handle to the child after the rest of the initialisation. This has the added advantage of not highlighting specifically where the problems lie.

While chromium isn't directly affected in the same way, I think we would still want to upstream the fix, because it is possible that a non-chromium compromised process could use this to attack the chromium parent. Although the risk is much smaller.
In the upstream fix I think it makes sense to change the line you note as (B) to use base_start because it just makes more sense, but I'd prefer to keep our initial fix as small as possible and delaying the duplication of the shared memory, will fix both.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Bug 1599005: Move shared memory duplication after initialization. r=handyman!

Comment 7

[Andrew McCreight [:mccr8]]

It sounds like this is a sandbox escape that requires arbitrary code execution in a content process, so I'm going to mark this sec-high.

Comment 8

[Bob Owen (:bobowen)]

(In reply to Andrew McCreight [:mccr8] from comment #7)

It sounds like this is a sandbox escape that requires arbitrary code execution in a content process, so I'm going to mark this sec-high.

Just for context this is not a sandbox escape in itself, but it is a way of corrupting a data structure that might lead to an exploitable crash in the parent.
service_context->channel_buffer is set to the base of the shared memory plus an offset.
It is that offset that can be changed, leading to service_context->channel_buffer, being corrupted.

Actually causing an exploitable crash and exploiting it to get a sandbox escape is not covered.

Comment 9

[Bob Owen (:bobowen)]

Comment on attachment 9111552 [details]
Bug 1599005: Move shared memory duplication after initialization. r=handyman!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch moves the duplication of the shared memory to the child until after it has been properly initialized.
  So, to find the actual issues would require further code inspection.
  I think landing it under an unrelated unsecure bug would make sense.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This code was updated in Fx69, so I'll need to create a patch for ESR68.
  Should be straight forward.
• How likely is this patch to cause regressions; how much testing does it need?: I think the change is pretty safe given that it is just moving code that shouldn't affect any of the intermediate code we're moving it after.
  However, this is very sensitive code.
  It gets hit on every sandboxed child launch.

Comment 10

[Bob Owen (:bobowen)]

Attachment: *ESR68 patch* Bug 1599005: Move shared memory duplication after initialization. r=handyman

Patch for ESR68.

Comment 11

[Thomas Imbert]

Thanks!

Should I open a Chromium bug? Just to give them a heads up and send them bugzilla link of 1599008 and this one?

Comment 12

[Bob Owen (:bobowen)]

(In reply to Thomas Imbert from comment #11)

Thanks!

Should I open a Chromium bug? Just to give them a heads up and send them bugzilla link of 1599008 and this one?

Given that Chromium isn't really affected, can we wait until we've decided when we're going to land this.
I suspect I'll end up picking up the Chromium bug anyway and there's no point adding the small risk of things getting out of sync and a fix landing on Chromium first.

Comment 13

[Tom Ritter [:tjr]]

This is another I would ideally like to land/uplift inside another (public) bug, can we do that?

Comment 14

[Tom Ritter [:tjr]]

+Julien. If this can't be landed (soon/this week) under a public bug, we should land and uplift it regardless because it's in critical code.

Comment 15

[Bob Owen (:bobowen)]

(In reply to Tom Ritter [:tjr] (needinfo for responses to sec-[approval/ratings/advisories/cve's]) from comment #13)

This is another I would ideally like to land/uplift inside another (public) bug, can we do that?

Yeah, as I said in the approval request, I think it's a good candidate for landing under a public bug.

Comment 16

[Julien Cristau [:jcristau]]

Sounds good, thanks for the heads-up.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 9111552 [details]
Bug 1599005: Move shared memory duplication after initialization. r=handyman!

Bob: not quite sure how to handle this request. If I put sec-approval in a public bug that defeats the purpose of creating the public bug, but the public-bug patch might be different if this is slipped in with some other work. Guess I'll approve it this version here anyway. Obviously don't mention sec-approval on the actual check-in.

When you know what the other bug will be please mention it here in a comment (not See Also or Depends On which will show up in the other bug).

Comment 18

[Bob Owen (:bobowen)]

Thanks Dan.

Tom, was there as bug you had in mind or is this something that you will coordinate?

Comment 19

[Daniel Veditz [:dveditz]]

No, coming up with a plausible public bug is not something we do. We don't know what else you might be working on in a related area (if anything!) that could obfuscate this, or what plausible non-security explanation that could explain this change in a separate clean-up type bug.

And then when you uplift it to ESR the jig is up anyway.

What's your holiday work schedule? I don't know what Tom had in mind for a cover bug, and I don't know at all what sandbox work is in-flight, so I'm inclined to say just land it as late as possible (before the Jan 2 soft freeze), uplift everywhere, and hope that's a short enough window.

What kind of commit message are you going to use? Looks like "Move shared memory duplication after initialization" which says "we're using an uninitialized copy" (which at least doesn't hint at a race condition). Could you change it to something else, maybe something misdirectional like "Don't waste time duplicating memory if we're just going to error out; do all other error checks first."

Comment 20

[Bob Owen (:bobowen)]

OK thanks Dan.
I thought there might have been some other coordination with the sheriffs about this, but if not I'll land before 2 Jan in a new public bug with a changed commit message as you suggest.

Comment 21

[Julien Cristau [:jcristau]]

We're building the 72.0 release candidate on Dec 30, this would have to land before then.

Comment 22

[Bob Owen (:bobowen)]

(In reply to Julien Cristau [:jcristau] from comment #21)

We're building the 72.0 release candidate on Dec 30, this would have to land before then.

Noted, I'll make sure it is, thanks.

Comment 23

[Julien Cristau [:jcristau]]

Please request uplift on this.

Comment 24

[Bob Owen (:bobowen)]

(In reply to Julien Cristau [:jcristau] (afk Dec 25-29) from comment #23)

Please request uplift on this.

I was going to land and request uplift on the 28th, do you want me to do it earlier?

Comment 25

[Julien Cristau [:jcristau]]

If you can request uplift earlier that'd be great. It doesn't have to wait until the patch lands on trunk.

Comment 26

[Bob Owen (:bobowen)]

(In reply to Julien Cristau [:jcristau] (afk Dec 25-29) from comment #25)

If you can request uplift earlier that'd be great. It doesn't have to wait until the patch lands on trunk.

The only problem is that I'm going to land and uplift from a new public bug (I've just created bug 1605867 for this), so I would ask for approval on that bug, but it would mean uploading the patches to it now, which I'd rather avoid.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

If you can get the patches landed/uplift request in today then we can include the patches in tonight's beta build. If not, then for the RC.

Comment 28

[Bob Owen (:bobowen)]

(In reply to Liz Henry (:lizzard) from comment #27)

If you can get the patches landed/uplift request in today then we can include the patches in tonight's beta build. If not, then for the RC.

Hi Liz - patch is on autoland (bug 1605867), uplift requested to Beta and ESR68 (with a separate patch on that bug).

Comment 29

[Thomas Imbert]

Is this bug eligible and bug 1599008 for the client bug bounty program?

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Thomas, if you can wait for dveditz or tritter to get back after the holidays, they can answer that.

Comment 31

[Thomas Imbert]

No problem, thanks!

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Noting the patch from bug 1605867 is now on beta (and made it into beta 11).

Comment 33

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/b9ced0dd1026

Comment 34

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 35

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 36

[Thomas Imbert]

Sorry, I didn't receive any emails from abillings regarding the bounty (1599005+1599008).
My situation has changed since the last bounty, who should I contact to update my information (form W-9)?

Comment 37

[Tom Ritter [:tjr]]

(In reply to Thomas Imbert from comment #36)

Sorry, I didn't receive any emails from abillings regarding the bounty (1599005+1599008).
My situation has changed since the last bounty, who should I contact to update my information (form W-9)?

That's me - I'll email you.

Comment 38

[Will Harris]

Hi Thomas Imbert, Will from Chromium here - we'd appreciate you filing a bug for this in our bug tracker for issues like this, even if they don't affect Chrome. We can always coordinate with Mozilla on the release timelines if it affects Firefox. Filing a bug in Chromium bug tracker ensures that all downstream users of the Chromium sandbox get a fix as soon as possible.

Comment 39

[Bob Owen (:bobowen)]

(In reply to Will Harris from comment #38)

Hi Thomas Imbert, Will from Chromium here - we'd appreciate you filing a bug for this in our bug tracker for issues like this, even if they don't affect Chrome. We can always coordinate with Mozilla on the release timelines if it affects Firefox. Filing a bug in Chromium bug tracker ensures that all downstream users of the Chromium sandbox get a fix as soon as possible.

Sorry Will, that's probably partly my fault.
I should have circled back to get Thomas to file a bug on Chromium once we'd decided on when we were landing this.

If Thomas isn't following bug mail for some reason, I'll file on his behalf.

Comment 40

[Thomas Imbert]

Hi Will,

Oh sorry, I misunderstood Bob thinking he will port the fix to Chromium and did not check afterwards.

My fault, I can report these bugs (#1599005 + #1599008, probably should report #1542581 too) to Chromium bug tracker (https://bugs.chromium.org/p/chromium/issues/list Should I file them here as New issue ?) even if Chrome is not affected and I will do it for future issues in Google products.

Thomas

Comment 41

[Will Harris]

hi Bob, Thomas - Thanks for your replies :)

Yes - please go ahead and file into https://bugs.chromium.org/p/chromium - even if Chromium is not directly affected, we'd like to upstream your fixe(s) and make sure we didn't forget or miss anything, so please enclose all the details, and we can make sure we apply any fixes to chromium sandbox.

Thanks again!

Comment 42

[Thomas Imbert]

No problem, reported both sandbox races https://bugs.chromium.org/p/chromium/issues/detail?id=1092010 and old breakpad issue just in case https://bugs.chromium.org/p/google-breakpad/issues/detail?id=823