1610426 - (CVE-2020-6796) 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)

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Thomas Imbert]

Attachment: 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

Comment 1

[Johann Hofmann [:johannh]]

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

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Confirmed, I'll fix it this afternoon.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Bug 1610426 - Discard unknown crash annotations r=froydnj

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

CC'ing Nathan for the review.

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[Nathan Froyd [:froydnj]]

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

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

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 9

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 10

[Andrew McCreight [:mccr8]]

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

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

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

Approved to land and request uplift.

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

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.

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/b315803378d5

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

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 17

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 18

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/910a7b4b5f2e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8f3b8fef5686

Comment 22

[Thomas Imbert]

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

Comment 23

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

Attachment: advisory.txt

Comment 24

[Emil Ghitta, QA [:emilghitta]]

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!

Comment 25

[Thomas Imbert]

(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

Comment 26

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 27

[Thomas Imbert]

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

Comment 28

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 29

[Gabriele Svelto [:gsvelto] (PTO)]

Filed bug 1612899, it's a trivial fix.