1608645 - Crash in [@ mozilla::nt::PEHeaders::FindExportAddressTableEntry]

Status: RESOLVED WONTFIX

(Core :: mozglue, defect)

Description

[Calixte Denizet (:calixte)]

This bug is for crash report bp-09602295-650e-456b-9870-230080200111.

Top 10 frames of crashing thread:

0 firefox.exe mozilla::nt::PEHeaders::FindExportAddressTableEntry mozglue/misc/NativeNt.h:569
1 firefox.exe mozilla::interceptor::MMPolicyOutOfProcess::GetProcAddress const mozglue/misc/interceptor/MMPolicies.h:682
2 firefox.exe mozilla::interceptor::FuncHookCrossProcess<mozilla::interceptor::WindowsDllInterceptor<mozilla::interceptor::VMSharingPolicyUnique<mozilla::interceptor::MMPolicyOutOfProcess> >, long  mozglue/misc/nsWindowsDllInterceptor.h:231
3 firefox.exe mozilla::InitializeDllBlocklistOOP browser/app/winlauncher/DllBlocklistInit.cpp:43
4 xul.dll mozilla::SandboxBroker::LaunchApp security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:339
5 xul.dll mozilla::ipc::WindowsProcessLauncher::DoLaunch ipc/glue/GeckoChildProcessHost.cpp:1492
6 xul.dll mozilla::ipc::BaseProcessLauncher::PerformAsyncLaunch ipc/glue/GeckoChildProcessHost.cpp:959
7 xul.dll mozilla::detail::ProxyRunnable<mozilla::MozPromise<mozilla::ipc::LaunchResults, mozilla::ipc::LaunchError, 0>, RefPtr<mozilla::MozPromise<mozilla::ipc::LaunchResults, mozilla::ipc::LaunchError, 0> >  xpcom/threads/MozPromise.h:1344
8 xul.dll mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:207
9 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1248

There are 7 crashes (from 1 installation) in nightly 74 with buildid 20200111094829. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1604008.

[1] https://hg.mozilla.org/mozilla-central/rev?node=f0890a32d6bb

Comment 1

[Toshihito Kikuchi [:toshi]]

Thank you for letting me know. All instances have cyinjct.dll/cyvrtrap.dll/cyvera.dll in the process. It seems that they tamper Export Table. We should use RVAToPtrUnchecked instead of RVAToPtr in FindExportAddressTableEntry.

Comment 2

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1608645 - Ensure FindExportAddressTableEntry can handle a modified Export Table. r=aklotz

A third-party application can modify the export directory, the export address/name/ordinal
tables, or an entry in those tables. If that happens, we will see an RVA is located outside
the mapped image and RVAToPtr returns null. This patch makes sure we don't hit null AV
when modification is detected.

FindExportAddressTableEntry should not return a pointer to the modified table entry because
we dereference it in another process to cross-process detour.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Hi Aaron, I know this area isn't technically your responsibility anymore, but this bug is blocking a topcrash for Fx73 from being uplifted. Can you please look at this review soon as there's only 2 weeks left this cycle before 73 goes to RC (and an All Hands in there for good measure). Thanks!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f74adc43b654
Ensure FindExportAddressTableEntry can handle a modified Export Table.  r=aklotz

Comment 5

[Daniel Varga [:dvarga]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Nightly crash data is looking good so far. Please nominate this for Beta approval when you get a chance.

Comment 7

[Toshihito Kikuchi [:toshi]]

Let us wait one more night . I'll add an uplift request for this tomorrow morning.

Comment 8

[Toshihito Kikuchi [:toshi]]

Comment on attachment 9120512 [details]
Bug 1608645 - Ensure FindExportAddressTableEntry can handle a modified Export Table. r=aklotz

Beta/Release Uplift Approval Request

• User impact if declined: Firefox crashes if ntdll's export table is tampered by an external application.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1604008
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is to add null check and give up detouring if tampering is detected. If we fail to detour a function because of that, it may cause another problem, but it should be a different compatibility issue and better than this crash.
• String changes made/needed: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9120512 [details]
Bug 1608645 - Ensure FindExportAddressTableEntry can handle a modified Export Table. r=aklotz

Follow-up fix for bug 1604008. Approved for 73.0b9.

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d569074605c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out from Beta along with bug 1604008 to avoid shipping bug 1610790 in 73.0b9.
https://hg.mozilla.org/releases/mozilla-beta/rev/404293e0e5aa

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9120512 [details]
Bug 1608645 - Ensure FindExportAddressTableEntry can handle a modified Export Table. r=aklotz

Per bug 1604008 comment 50.

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/302a75886c58
Backed out changeset f74adc43b654 as requested by tkikuchi (toshi).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Per bug 1604008 comments 54 & 55.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/826c04066745

Comment 16

[Myron]

Confirmation that FF beta 74.0b3 (64-bit) and 0patch v19.11.15.10650 appear to be operating correctly with 0patch's mitigation removed.