1542581 - (CVE-2019-9818) Race condition in google_breakpad::CrashGenerationServer::AddClient leading to UAF write in broker (Sandbox escape / LPE)

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect, P1)

Description

Attachment: race_asan_report.txt

Hi,

There is a race condition in the Windows implementation of the CrashGenerationServer leading to a write of a freed object in Firefox main process. This crash server is enabled by default on Firefox release build and any sandboxed processes can send messages on a pipe : "\.\pipe\gecko-crash-server-pipe.{ParentPID}" (passed to the sandbox in the command line).

The vulnerability exists in the following function:

bool CrashGenerationServer::AddClient(ClientInfo* client_info) {
  HANDLE request_wait_handle = NULL;
  /* ... */ 
  // OnClientEnd will be called when the client process terminates.
  HANDLE process_wait_handle = NULL;
  if (!RegisterWaitForSingleObject(&process_wait_handle,
                                   client_info->process_handle(),
                                   OnClientEnd, // This callback can FREE client_info (no lock)
                                   client_info,
                                   INFINITE,
                                   WT_EXECUTEONLYONCE)) {
    return false;
  }

  client_info->set_process_exit_wait_handle(process_wait_handle); // UAF write (crash)
/* ... */
}

The OnClientEnd callback frees the client_info by calling the following function:

CrashGenerationServer::HandleClientProcessExit(ClientInfo* client_info) { // Called by CrashGenerationServer::OnClientEnd
/* ... code that doesn't block / wait  */
  delete client_info; // FREE client_info
}

Crash:

To compile an ASAN build of the client, you need to remove this line from the build config:
ac_add_options --disable-crashreporter
See attachment the ASAN crash report "race_asan_report.txt" reproduced on a local build (changeset 527669:bd1e28b0143b from Mar 29).

Exploitability:

The exploitation assumes having code execution in sandboxed renderers processes (RCE -> LPE step targeted).

The only way to trigger the OnClientEnd callback is to terminate the process being registered.
The pipe is created with maximum one instance (nMaxInstances=1 and FILE_FLAG_FIRST_PIPE_INSTANCE), so you have one shot by process (you can't spray connections to AddClient) but the crash generation server doesn't check if the registered client is already registered and if its PID matches the caller on the pipe (one process can register anyone see HandleReadDoneState).

To exploit this, you can either have two threads in one process, one sending the ProtocolMessage to add the client and the other one terminating the process OR two sandboxed process, one registering the other process and the other killing itself.
If it's not successful, ask the broker to create a new tab (new process), replay your RCE to gain code execution and retry.
If successful, the UAF gives you a HANDLE write at a fixed offset in the broker heap.
By spraying an exploit convenient object (semi controlled by another IPC channel to the broker like IPDL), you can use this vulnerability to corrupt a field (size/flags) in it and with some work, achieve code execution in Firefox main process (Sandbox escape).

Thank you

Comment 1

[:Gijs (he/him)]

Gabriele, could you take a look?

Comment 2

[Gabriele Svelto [:gsvelto]]

Sure, I was just investigating a rare race in the crash reporter we encounter once in a blue moon in Windows tests so this may be it.

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1542581 - Block child process termination until the handler is fully initialized

Comment 4

[Gabriele Svelto [:gsvelto]]

CC'ing Nathan for the patch review.

Comment 5

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9057793 [details]
Bug 1542581 - Block child process termination until the handler is fully initialized

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The exploit description is in comment 0. It doesn't seem very easy but I'm no expert so I might be wrong.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All branches are affected, this is a long-standing issue
• If not all supported branches, which bug introduced the flaw?: Bug 791775
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The patch should apply cleanly to all affected branches including ESR60
• How likely is this patch to cause regressions; how much testing does it need?: The patch is covered by automated tests and should only affect the browser behavior under a narrow race that is very unlikely to happen in normal usage.

Comment 6

[Gabriele Svelto [:gsvelto]]

I mentioned the tests in the comment above but I forgot to mention that I ran all of them locally and also manually tried generating crash reports with the patch applied. No test has been run on try for obvious reasons.

Comment 7

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

sec-approval+ for mozilla-central. We'll want ESR60 and Beta patches nominated as well.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/4cf5365482add7052c58e575d48a0a60cf9f3575
https://hg.mozilla.org/mozilla-central/rev/4cf5365482ad

Comment 9

[Thomas Imbert]

Thanks! The patch LGTM. Is the bug eligible for the client bug bounty program?

Comment 10

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9057793 [details]
Bug 1542581 - Block child process termination until the handler is fully initialized

Beta/Release Uplift Approval Request

• User impact if declined: Critical security issue, see comment 0 for the exploit details
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): I'm flagging this as medium risk - even though the change is rather simple - because it affects locking behaviuor which is always hard to prove correct.
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Critical security issue, see comment 0 for the exploit details
• Fix Landed on Version: 67.0
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): I'm flagging this as medium risk - even though the change is rather simple - because it affects locking behaviuor which is always hard to prove correct.
• String or UUID changes made by this patch: None

Comment 11

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

Comment on attachment 9057793 [details]
Bug 1542581 - Block child process termination until the handler is fully initialized

OK for uplift for beta 13, esr 60.7.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e9f1cf0693663467059f729d897e2f23b4631e9

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/5a2b41b92c32