Closed Bug 1608645 Opened 3 months ago Closed 3 months ago

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

Categories

(Core :: mozglue, defect)

Unspecified
Windows 10
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox-esr68 --- unaffected
firefox72 --- unaffected
firefox73 + wontfix
firefox74 + disabled
firefox75 --- disabled

People

(Reporter: calixte, Assigned: toshi)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 obsolete file)

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

Flags: needinfo?(tkikuchi)

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.

Flags: needinfo?(tkikuchi)
Assignee: nobody → tkikuchi

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.

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!

Flags: needinfo?(aklotz)
Flags: needinfo?(aklotz)
Pushed by rmaries@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f74adc43b654
Ensure FindExportAddressTableEntry can handle a modified Export Table.  r=aklotz
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

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

Flags: needinfo?(tkikuchi)

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

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
Flags: needinfo?(tkikuchi)
Attachment #9120512 - Flags: approval-mozilla-beta?

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.

Attachment #9120512 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Regressions: 1610790
Attachment #9120512 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?

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

Per bug 1604008 comment 50.

Attachment #9120512 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/302a75886c58
Backed out changeset f74adc43b654 as requested by tkikuchi (toshi).

Per bug 1604008 comments 54 & 55.

Resolution: FIXED → WONTFIX
Target Milestone: mozilla74 → ---
Attachment #9120512 - Flags: approval-mozilla-release?
Attachment #9120512 - Attachment is obsolete: true

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

You need to log in before you can comment on or make changes to this bug.