Closed Bug 1610426 (CVE-2020-6796) Opened 1 year ago Closed 1 year ago

No validation of array index (key) in xul!mozilla::ipc::CrashReporterMetadataShmem::ReadAppNotes leads to Stack Out-Of-Bounds write in the broker process (Sandbox Escape / LPE)

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox-esr68 73+ fixed
firefox72 --- wontfix
firefox73 + fixed
firefox74 + fixed

People

(Reporter: mastho64, Assigned: gsvelto)

Details

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

Attachments

(3 files)

Attached file stackoobw.zip

Hi,

Vulnerability

Memory corruption (Sandbox escape) in mozilla::ipc::CrashReporterMetadataShmem::ReadAppNotes tested on Firefox 72.0.2.7321

The vulnerability is in the following function:

// https://github.com/mozilla/gecko-dev/blob/master/ipc/glue/CrashReporterMetadataShmem.cpp#L210
typedef mozilla::EnumeratedArray<Annotation, Annotation::Count, nsCString> AnnotationTable;
void CrashReporterMetadataShmem::ReadAppNotes(const Shmem& aShmem,
                                              AnnotationTable& aNotes) {
  for (MetadataShmemReader reader(aShmem); !reader.Done(); reader.Next()) {
    switch (reader.Type()) {
      case EntryType::Annotation: {
        Annotation key; // no sanitization of key (uint32_t)
        nsCString value;
        if (!reader.Read(&key) || !reader.Read(value)) {
          return;
        }
        // Stack OOB write due to key (neither EnumeratedArray[] or Array[] have bounds check in release build)
        aNotes[key] = value; // EnumeratedArray (fixed length) stored in stack of mozilla::ipc::CrashReporterHost::FinalizeCrashReport
        break;
      }
      default:
        NS_ASSERTION(false, "Unknown metadata entry type");
        break;
    }
  }
}

The key variable comes from the shared memory (ShMem) without any validation and it is used as array index for storing the value string. A malicious content process (sandboxed) can modify its CrashReporterClient shared memory, crash itself to trigger the vulnerable code path in the broker process (not sandboxed) and achieve code execution outside the sandbox.

Repro and Crash

To reproduce, install Python 2.7 64 bits and PythonForWindows (https://github.com/hakril/PythonForWindows), compile payload_corrupt.cc (change offset in GetShMem if running different version then nmake) then run inject.py with the payload_corrupt.dll path.

The POC (provided - see attached files) sets the key to 0xfffffff of the first annotation and crash the content process (NULL deref).

The broker process will crash when trying to read the annotation of the crashed child:

xul!nsTSubstring<char>::Assign+0x41:
00007ff9`c3bb1041 0fb7460c        movzx   eax,word ptr [rsi+0Ch] ds:00000085`32ffe164=????

this = 0x00000085`32ffe158 (nsTSubstring<char>::Assign)
00000084`32ffe168 class mozilla::EnumeratedArray<CrashReporter::Annotation,CrashReporter::Annotation::Count,nsTString<char> > annotations (CrashReporterHost::FinalizeCrashReport)
? 0xfffffff * 0x10 + 0x8432ffe168 + c
Evaluate expression: 572086280548 = 00000085`32ffe164

0:000> kc
 # Call Site
00 xul!nsTSubstring<char>::Assign
01 xul!nsTSubstring<char>::Assign
02 xul!nsTString<char>::operator=
03 xul!mozilla::ipc::CrashReporterMetadataShmem::ReadAppNotes
04 xul!mozilla::ipc::CrashReporterHost::FinalizeCrashReport
05 xul!mozilla::ipc::CrashReporterHost::GenerateCrashReport
06 xul!mozilla::dom::ContentParent::ActorDestroy
07 xul!mozilla::ipc::IProtocol::DestroySubtree
08 xul!mozilla::dom::PContentParent::OnChannelError
09 xul!mozilla::dom::ContentParent::OnChannelError
0a xul!mozilla::detail::RunnableMethodArguments<>::applyImpl
0b xul!mozilla::detail::RunnableMethodArguments<>::apply
0c xul!mozilla::detail::RunnableMethodImpl<mozilla::ipc::MessageChannel *,void (mozilla::ipc::MessageChannel::*)(),0,mozilla::RunnableKind::Cancelable>::Run
0d xul!nsThread::ProcessNextEvent
0e xul!NS_ProcessNextEvent
0f xul!mozilla::ipc::MessagePump::Run
10 xul!MessageLoop::RunInternal
11 xul!MessageLoop::RunHandler
12 xul!MessageLoop::Run
13 xul!nsBaseAppShell::Run
14 xul!nsAppShell::Run
15 xul!nsAppStartup::Run
16 xul!XREMain::XRE_mainRun
17 xul!XREMain::XRE_main
18 xul!XRE_main

Exploitation

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

By modifying the key (array index), we can replace objects stored in the stack frame of caller functions.

On Firefox version 72.0.2.7321 on Windows 10 x86-64, some offsets are very interesting for exploitation:

xul!nsCOMPtr_base::~nsCOMPtr_base+0xb5d [inlined in xul!nsThread::ProcessNextEvent+0x14d8]:
00007ff9`c3beab08 ff5010          call    qword ptr [rax+10h] ds:41414141`41414151=???????????????? // rax controlled via the string value
0:000> kc
 # Call Site
00 xul!nsCOMPtr_base::~nsCOMPtr_base
01 xul!mozilla::dom::ScriptSettingsStackEntry::~ScriptSettingsStackEntry
02 xul!mozilla::dom::AutoNoJSAPI::~AutoNoJSAPI
03 xul!mozilla::Maybe<mozilla::dom::AutoNoJSAPI>::reset
04 xul!nsThread::ProcessNextEvent
05 xul!NS_ProcessNextEvent

Definitily with some work, it is possible to achieve code execution in Firefox main process (Sandbox escape).

Thank you

Flags: sec-bounty?

Jed, are you the right person to take a look at this bug?

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

Confirmed, I'll fix it this afternoon.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → gsvelto
Group: core-security → dom-core-security

CC'ing Nathan for the review.

Status: NEW → ASSIGNED
Flags: needinfo?(jld)

Could we add runtime checking to mozilla::Array<>?

I guess this is a little mitigated at least on 64 bit systems by the fact that the index is 32 bit, but writing arbitrary values to arbitrary offsets within that range sounds bad.

It looks like this might be a regression from bug 1348273, which landed in 63. I didn't look too closely, but it looked like it was some kind of hash map before that.

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

Could we add runtime checking to mozilla::Array<>?

I guess this is a little mitigated at least on 64 bit systems by the fact that the index is 32 bit, but writing arbitrary values to arbitrary offsets within that range sounds bad.

We should at least add it to EnumeratedArray, because people will assume their enums are always in bounds. Adding it to Array might be a little harder, as I think Array gets used in some perf-critical code like dom bindings. But maybe we can add an IfYouReallyKnowWhatYouAreDoing template parameter or something.

Yes, previously it was a hash table and everything used strings instead of an enum. The plan calls for this code to be rewritten and moved out of Gecko this year (see bug 1588530) so that we don't need this complex IPC dance between the crashed content process and the main process.

Comment on attachment 9122027 [details]
Bug 1610426 - Discard unknown crash annotations r=froydnj

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: An example is provided in the bug.
  • 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?: 68esr
  • If not all supported branches, which bug introduced the flaw?: Bug 1348273
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?:
  • How likely is this patch to cause regressions; how much testing does it need?: The patch introduces a trivial check so should be risk-free
Attachment #9122027 - Flags: sec-approval?
Group: dom-core-security → firefox-core-security
Component: IPC → Crash Reporting
Product: Core → Toolkit

(In reply to Gabriele Svelto [:gsvelto] from comment #9)

  • How easily could an exploit be constructed based on the patch?: An example is provided in the bug.

That's not quite answering the question. The patch makes it clear what the issue is, as it adds a bounds check to a write to a crash reporter array.

Comment on attachment 9122027 [details]
Bug 1610426 - Discard unknown crash annotations r=froydnj

Approved to land and request uplift.

Attachment #9122027 - Flags: sec-approval? → sec-approval+

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

(In reply to Gabriele Svelto [:gsvelto] from comment #9)

  • How easily could an exploit be constructed based on the patch?: An example is provided in the bug.

That's not quite answering the question. The patch makes it clear what the issue is, as it adds a bounds check to a write to a crash reporter array.

I apologize, my brain stopped processing the question at "How easily could an exploit be constructed" and somehow skipped the "based on the patch" part. While it's not obvious that you need to crash a content process first it's pretty easy to tell what you can do with it.

Landed: https://hg.mozilla.org/integration/autoland/rev/b315803378d515c12f42162332d389f3942b51fc

I hope to have adjusted the flags accordingly. I'll request update for ESR and beta.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Group: firefox-core-security → core-security-release
Target Milestone: --- → mozilla74

I haven't requested the uplifts yet because I'm verifying the fix in nightly first.

I tried verifying the fix but after modifying the exploit code to build (and adjusting the offsets) I can't get the injection script to work.

Comment on attachment 9122027 [details]
Bug 1610426 - Discard unknown crash annotations r=froydnj

Beta/Release Uplift Approval Request

  • User impact if declined: This is a significant security issue even though it would need at least another exploit to be used
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch only adds a bounds check to prevent the out-of-bounds access
  • String changes made/needed: none
Attachment #9122027 - Flags: approval-mozilla-beta?

Comment on attachment 9122027 [details]
Bug 1610426 - Discard unknown crash annotations r=froydnj

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec:high bug
  • User impact if declined: This is a significant security issue even though it would need at least another exploit to be used
  • Fix Landed on Version: 74
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch only adds a bounds check to prevent the out-of-bounds access
  • String or UUID changes made by this patch: none
Attachment #9122027 - Flags: approval-mozilla-esr68?

Comment on attachment 9122027 [details]
Bug 1610426 - Discard unknown crash annotations r=froydnj

Avoids a possible out-of-bounds access. Approved for 73.0b9 and 68.5esr.

Attachment #9122027 - Flags: approval-mozilla-esr68?
Attachment #9122027 - Flags: approval-mozilla-esr68+
Attachment #9122027 - Flags: approval-mozilla-beta?
Attachment #9122027 - Flags: approval-mozilla-beta+

(In reply to Gabriele Svelto [:gsvelto] from comment #16)

I tried verifying the fix but after modifying the exploit code to build (and adjusting the offsets) I can't get the injection script to work.

Please can you check the ACL on the payload_corrupt DLL? Please add Everyone Read and Read & Execute, the renderer process may not have access to the dll file.

Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][adv-main73+] [adv-esr68.5+]
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main73+] [adv-esr68.5+] → [reporter-external] [client-bounty-form] [verif?][adv-main73+] [adv-esr68.5+][postcritsmash-triage]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main73+] [adv-esr68.5+][postcritsmash-triage] → [reporter-external] [client-bounty-form] [verif?][adv-main73+] [adv-esr68.5+][post-critsmash-triage]
QA Whiteboard: [qa-triaged]

Hi Thomas!

It seems that I'm unable to inject the script in order to verify if this issue was fixed or not.

Can you please help us by verifying (using the fixed Fx versions) if this issue is fixed on your side?

Thank you!

QA Whiteboard: [qa-triaged]
Flags: needinfo?(mastho64)
QA Whiteboard: [qa-triaged]

(In reply to Emil Ghitta, QA [:emilghitta] from comment #24)

Hi Thomas!

It seems that I'm unable to inject the script in order to verify if this issue was fixed or not.

Can you please help us by verifying (using the fixed Fx versions) if this issue is fixed on your side?

Thank you!

Hi,

Where can I download the fixed version?

Thomas

Flags: needinfo?(mastho64)

The latest nightly should have the fix: https://www.mozilla.org/it/firefox/channel/desktop/

Thank you,

Steps:

  • Download nightly from https://www.mozilla.org/en-US/firefox/channel/desktop/ (version 74.0a1 (2020-02-02) (64-bit))
  • Download symbols of xul version 74.0a1 and find offset of xul!sClientSingleton symbol
  • Change offset in payload_corrupt.cc (new offset 0x05905710)
  • Open Visual Studio shell and nmake the POC
  • icacls bin\payload_corrupt.dll /grant Everyone:RX
  • Attach debugger to the broker (firefox.exe)
  • Install python 2.7 and PythonForWindows (github) and run inject.py
  • No crash in xul!mozilla::ipc::CrashReporterMetadataShmem::ReadAppNotes

Actually, a bit later there is an exception (Invalid handle - code c0000008) in CrashReporter::PlatformWriter::~PlatformWriter (not a security issue and the exception is handled).

Debugging notes:

xul!mozilla::ipc::CrashReporterMetadataShmem::ReadAppNotes+0xd4:
00007ff9`b8512694 81fe99000000    cmp     esi,99h // rsi=000000000fffffff the OOB index now checked against 0x99 (Annotation::Count)
0:000> t 
xul!mozilla::ipc::CrashReporterMetadataShmem::ReadAppNotes+0xda:
00007ff9`b851269a 7777            ja      xul!mozilla::ipc::CrashReporterMetadataShmem::ReadAppNotes+0x153 (00007ff9`b8512713) [br=1] // branch to return

(In reply to Thomas Imbert from comment #27)

Actually, a bit later there is an exception (Invalid handle - code c0000008) in CrashReporter::PlatformWriter::~PlatformWriter (not a security issue and the exception is handled).

Thank you. I know why that's happening, I'll file a separate issue for that.

Filed bug 1612899, it's a trivial fix.

Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.