Closed Bug 1599005 (CVE-2019-17015) Opened 11 months ago Closed 10 months ago

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

Categories

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

All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox-esr68 72+ fixed
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 + fixed
firefox73 + fixed

People

(Reporter: mastho64, Assigned: bobowen)

References

Details

(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main72+][adv-esr68.4+])

Attachments

(4 files, 1 obsolete file)

Attached file 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

Flags: sec-bounty?

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.

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!

Group: firefox-core-security → core-security
Type: task → defect
Component: Security → Security: Process Sandboxing
Flags: needinfo?(mastho64)
Product: Firefox → Core

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

Flags: needinfo?(bobowencode)
See Also: → CVE-2019-17021

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?

Flags: needinfo?(mastho64)

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.

Assignee: nobody → bobowencode
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bobowencode)
OS: Unspecified → Windows
Priority: -- → P1
Hardware: Unspecified → All

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.

See Also: CVE-2019-17021

(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 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.
Attachment #9111552 - Flags: sec-approval?
Group: core-security → dom-core-security
Attachment #9112008 - Attachment description: *ESR68 patch* Bug 1599005: Move shared memory duplication after initialization. r=handymanbug1599005esr68.patch → *ESR68 patch* Bug 1599005: Move shared memory duplication after initialization. r=handyman

Thanks!

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

(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.

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

Flags: needinfo?(lhenry)
Flags: needinfo?(bobowencode)

+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.

Flags: needinfo?(jcristau)

(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.

Flags: needinfo?(bobowencode)

Sounds good, thanks for the heads-up.

Flags: needinfo?(jcristau)

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).

Flags: needinfo?(bobowencode)
Attachment #9111552 - Flags: sec-approval? → sec-approval+

Thanks Dan.

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

Flags: needinfo?(bobowencode) → needinfo?(tom)
Flags: needinfo?(tom) → needinfo?(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."

Flags: needinfo?(dveditz)

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.

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

(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.

Please request uplift on this.

Flags: needinfo?(bobowencode)

(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?

Flags: needinfo?(bobowencode) → needinfo?(jcristau)

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

Flags: needinfo?(jcristau)

(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.

Flags: needinfo?(jcristau)

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.

Flags: needinfo?(bobowencode)

(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).

Flags: needinfo?(jcristau)
Flags: needinfo?(bobowencode)

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

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

No problem, thanks!

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

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
Flags: qe-verify-
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main72+]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main72+] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main72+][adv-esr68.4+]
Flags: sec-bounty? → sec-bounty+

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)?

(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.

Group: core-security-release

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.

Flags: needinfo?(mastho64)

(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.

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

Flags: needinfo?(mastho64)

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!

You need to log in before you can comment on or make changes to this bug.