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

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

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

Comment 1

[Christoph Diehl [:posidron]]

Attachment: clusterfuzz-testcase-minimized-ContentParentIPC-5129161198272512

Comment 2

[(no longer active)]

Hmm, how can the test case be used?

Comment 3

[(no longer active)]

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

Comment 4

[(no longer active)]

Attachment: Bug 1577563 - Add proper argument checking in AntiTrackingCommon::CreateStoragePermissionKey();

Comment 5

[(no longer active)]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9530fc5f5e32
Add proper argument checking in AntiTrackingCommon::CreateStoragePermissionKey(); r=johannh

Comment 7

[(no longer active)]

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

Comment 8

[(no longer active)]

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

Comment 9

[Jonathan Kingston [:jkt] he/him]

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

Comment 10

[Frederik Braun [:freddy]]

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?

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/9530fc5f5e32

Comment 12

[Christoph Diehl [:posidron]]

:ehsan, this seems fixed.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dfc989703283

Comment 15

[(no longer active)]

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