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)
Tracking
()
People
(Reporter: mastho64, Assigned: gsvelto)
Details
(5 keywords, Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main73+] [adv-esr68.5+][post-critsmash-triage])
Attachments
(3 files)
3.69 KB,
application/x-zip-compressed
|
Details | |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr68+
tjr
:
sec-approval+
|
Details | Review |
293 bytes,
text/plain
|
Details |
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•5 years ago
|
||
Jed, are you the right person to take a look at this bug?
Assignee | ||
Comment 2•5 years ago
|
||
Confirmed, I'll fix it this afternoon.
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
Assignee | ||
Comment 4•5 years ago
|
||
CC'ing Nathan for the review.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 5•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
(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.
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
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.
Assignee | ||
Comment 9•5 years ago
|
||
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
Updated•5 years ago
|
Comment 10•5 years ago
|
||
(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•5 years ago
|
||
Comment on attachment 9122027 [details]
Bug 1610426 - Discard unknown crash annotations r=froydnj
Approved to land and request uplift.
Assignee | ||
Comment 12•5 years ago
|
||
(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.
Assignee | ||
Comment 13•5 years ago
|
||
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•5 years ago
|
||
Assignee | ||
Comment 15•5 years ago
|
||
I haven't requested the uplifts yet because I'm verifying the fix in nightly first.
Assignee | ||
Comment 16•5 years ago
|
||
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.
Assignee | ||
Comment 17•5 years ago
|
||
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
Assignee | ||
Comment 18•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
uplift |
Comment 21•5 years ago
|
||
uplift |
Reporter | ||
Comment 22•5 years ago
|
||
(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.
Updated•5 years ago
|
Comment 23•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 24•5 years ago
|
||
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!
Updated•5 years ago
|
Reporter | ||
Comment 25•5 years ago
|
||
(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
Assignee | ||
Comment 26•5 years ago
|
||
The latest nightly should have the fix: https://www.mozilla.org/it/firefox/channel/desktop/
Reporter | ||
Comment 27•5 years ago
|
||
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
Assignee | ||
Comment 28•5 years ago
|
||
(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.
Assignee | ||
Comment 29•5 years ago
|
||
Filed bug 1612899, it's a trivial fix.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•3 years ago
|
Updated•8 months ago
|
Description
•