Closed Bug 1577563 Opened 5 months ago Closed 5 months ago

IPC: crash [@mozilla::AntiTrackingCommon::CreateStoragePermissionKey]

Categories

(Core :: Privacy: Anti-Tracking, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 + fixed
firefox71 --- verified

People

(Reporter: posidron, Assigned: ehsan)

References

(Regression, )

Details

(Keywords: oss-fuzz, regression)

Attachments

(2 files)

Task

Item Description
Crash Type Null-dereference READ
Sanitizer address (ASAN)
Platform linux
Job Type libfuzzer_asan_firefox
Fuzz Target ContentParentIPC
Reliably Reproduces YES

Callstack

==1==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f38aab693a0 bp 0x7ffeb78b2710 sp 0x7ffeb78b2600 T0)
==1==The signal is caused by a READ memory access.
==1==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
    #0 0x7f38aab6939f in mozilla::AntiTrackingCommon::CreateStoragePermissionKey(nsIPrincipal*, nsTSubstring<char>&) mozilla-central/toolkit/components/antitracking/AntiTrackingCommon.cpp:0:29
    #1 0x7f38a4ce89ce in mozilla::dom::Document::AutomaticStorageAccessCanBeGranted(nsIPrincipal*) mozilla-central/dom/base/Document.cpp:15625:3
    #2 0x7f38a7da59b3 in mozilla::dom::ContentParent::RecvAutomaticStorageAccessCanBeGranted(IPC::Principal const&, std::function<void (bool const&)>&&) mozilla-central/dom/ipc/ContentParent.cpp:5750:13
    #3 0x7f38a2faac73 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /work/obj-fuzz/ipc/ipdl/PContentParent.cpp:10461:57
    #4 0x7f38a17d6ed2 in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, nsTArray<nsTString<char> > const&) /work/obj-fuzz/dist/include/ProtocolFuzzer.h:96:18
    #5 0x7f38a17d67e8 in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) mozilla-central/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:27:3
    #6 0x5618f06cc29f in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long)
    #7 0x5618f06b89de in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long)
    #8 0x5618f06bac99 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long))
    #9 0x7f38ab01b791 in mozilla::FuzzerRunner::Run(int*, char***) mozilla-central/tools/fuzzing/interface/harness/FuzzerRunner.cpp:61:10
    #10 0x7f38aaf6c6ae in XREMain::XRE_mainStartup(bool*) mozilla-central/toolkit/xre/nsAppRunner.cpp:3758:35
    #11 0x7f38aaf74e15 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) mozilla-central/toolkit/xre/nsAppRunner.cpp:4698:12
    #12 0x7f38aaf757c1 in XRE_main(int, char**, mozilla::BootstrapConfig const&) mozilla-central/toolkit/xre/nsAppRunner.cpp:4792:21
    #13 0x5618f05d0b4a in do_main(int, char**, char**)
    #14 0x5618f05d0332 in main
    #15 0x7f38bcc8e82f in __libc_start_main
    #16 0x5618f04f2028 in _start
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_firefox_6180546f5e5f1d75138bf6cea3784693af7a2aa9/revisions/firefox/libxul.so+0x142fd39f)
==1==ABORTING
Attachment #9089129 - Attachment mime type: application/octet-stream → text/plain
Attachment #9089129 - Attachment mime type: text/plain → application/octet-stream

Hmm, how can the test case be used?

Flags: needinfo?(cdiehl)

Oh I think I understand, this is through a corrupt IPC message. We're failing to check for null IPC input.

Flags: needinfo?(cdiehl)
Keywords: regression
Regressed by: 1573236

[Tracking Requested - why for this release]: A compromised content process can crash the parent process.

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9530fc5f5e32
Add proper argument checking in AntiTrackingCommon::CreateStoragePermissionKey(); r=johannh

Comment on attachment 9089570 [details]
Bug 1577563 - Add proper argument checking in AntiTrackingCommon::CreateStoragePermissionKey();

Beta/Release Uplift Approval Request

  • User impact if declined: A malicious web page can cause Firefox (the entire browser) to crash.
  • 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): This patch only adds a null check for the arguments we receive through IPC from the content process.
  • String changes made/needed: None
Attachment #9089570 - Flags: approval-mozilla-beta?

Hey Christoph, can oss-fuzz verify the fix? Thanks!

Flags: needinfo?(cdiehl)

:ehsan Shouldn't we be null checking in the ipc method instead: https://searchfox.org/mozilla-central/rev/e04021f29e6d8a37753ba2b510432315ce05a8d7/dom/base/Document.cpp#15614-15616 Also should we be checking for the return value of the access key method also?

Flags: needinfo?(ehsan)

I want to understand if there might be more failures like this in our code. Do you see value in generally checking for null pointers in the Recv... functions in dom/ipc/ContentParent.cpp rather than all the various callees?

Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

:ehsan, this seems fixed.

Flags: needinfo?(cdiehl)

Comment on attachment 9089570 [details]
Bug 1577563 - Add proper argument checking in AntiTrackingCommon::CreateStoragePermissionKey();

Fix for potential crash, verified in nightly, let's uplift for beta 5.

Attachment #9089570 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

(In reply to Jonathan Kingston [:jkt] from comment #9)

:ehsan Shouldn't we be null checking in the ipc method instead: https://searchfox.org/mozilla-central/rev/e04021f29e6d8a37753ba2b510432315ce05a8d7/dom/base/Document.cpp#15614-15616

Is there a difference? (Technically the "ipc method" is this one FWIW).

Also should we be checking for the return value of the access key method also?

Both true or false are valid return values. Not sure what we should check.

(In reply to Frederik Braun [:freddyb] from comment #10)

I want to understand if there might be more failures like this in our code.

Almost certainly.

Do you see value in generally checking for null pointers in the Recv... functions in dom/ipc/ContentParent.cpp rather than all the various callees?

That seems like a very haphazard way of checking things to me. If this method returned a Maybe<nsIPrincipal*> then it would automatically force all callers to do The Right Thing, currently any code that receives an ipc::Principal could be tripped over the contained nsIPrincipal* being null...

Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.