1742770 - Assertion failure: uri->SchemeIs("moz-nullprincipal"), at caps/NullPrincipal.cpp:134

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Christian Holler (:decoder)]

I've prototyped a new libfuzzer-based fuzzing target that uses dom::ipc::StructuredCloneData as its entry point to cover the StructuredCloneReader code that is browser-only. The target code has not landed on mozilla-central yet, if you need it to reproduce (in case stack + test attached do not suffice), please let me know.

The attached testcase crashes on mozilla-central revision bf638341288e+:

Backtrace:

==12538==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fc6bd8a6500 bp 0x7ffd8eaabd90 sp 0x7ffd8eaab9e0 T0)
==12538==The signal is caused by a WRITE memory access.
==12538==Hint: address points to the zero page.
    #0 0x7fc6bd8a6500 in mozilla::NullPrincipal::CreateInternal(mozilla::OriginAttributes const&, bool, nsIURI*, nsIPrincipal*) caps/NullPrincipal.cpp:134:3
    #1 0x7fc6bd8a7175 in mozilla::NullPrincipal::Create(mozilla::OriginAttributes const&, nsIURI*) caps/NullPrincipal.cpp:71:10
    #2 0x7fc6bc047b82 in mozilla::ipc::PrincipalInfoToPrincipal(mozilla::ipc::PrincipalInfo const&) ipc/glue/BackgroundUtils.cpp:75:19
    #3 0x7fc6bd8b09b5 in nsJSPrincipals::ReadKnownPrincipalType(JSContext*, JSStructuredCloneReader*, unsigned int, JSPrincipals**) caps/nsJSPrincipals.cpp:318:25
    #4 0x7fc6bf3d1470 in mozilla::dom::StructuredCloneHolder::ReadFullySerializableObjects(JSContext*, JSStructuredCloneReader*, unsigned int) dom/base/StructuredCloneHolder.cpp:437:10
    #5 0x7fc6bf3d4bab in mozilla::dom::StructuredCloneHolder::CustomReadHandler(JSContext*, JSStructuredCloneReader*, JS::CloneDataPolicy const&, unsigned int, unsigned int) dom/base/StructuredCloneHolder.cpp:1008:10
    #6 0x7fc6cd76aead in JSStructuredCloneReader::startRead(JS::MutableHandle<JS::Value>, js::gc::InitialHeap) js/src/vm/StructuredClone.cpp:2815:11
    #7 0x7fc6cd74a4fb in JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>, unsigned long) js/src/vm/StructuredClone.cpp:3227:8
    #8 0x7fc6cd749cf2 in ReadStructuredClone(JSContext*, JSStructuredCloneData const&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:698:12
    #9 0x7fc6cd775927 in JS_ReadStructuredClone(JSContext*, JSStructuredCloneData const&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:3383:10
    #10 0x7fc6bf3d116e in mozilla::dom::StructuredCloneHolder::ReadFromBuffer(nsIGlobalObject*, JSContext*, JSStructuredCloneData&, unsigned int, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) dom/base/StructuredCloneHolder.cpp:409:8
    #11 0x7fc6bf3d1074 in mozilla::dom::StructuredCloneHolder::ReadFromBuffer(nsIGlobalObject*, JSContext*, JSStructuredCloneData&, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) dom/base/StructuredCloneHolder.cpp:395:3
    #12 0x7fc6c650454c in mozilla::dom::ipc::StructuredCloneData::Read(JSContext*, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) dom/ipc/StructuredCloneData.cpp:116:3
    #13 0x7fc6c64f570c in mozilla::dom::ipc::StructuredCloneData::Read(JSContext*, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) dom/ipc/StructuredCloneData.cpp:104:3
    #14 0x7fc6b80c0dec in FuzzingRunDomSC(unsigned char const*, unsigned long) dom/base/fuzztest/FuzzStructuredClone.cpp:59:10
    [...]

This is a release assert. Can we handle this case instead to make this fuzzer work? It has found a few bugs already it would be great to be able to run this on a regular basis. Marking s-s because the toplevel effort is s-s for now.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

Nika, should we validate and handle this error in the PrincipalInfo::TNullPrincipalInfo case in mozilla::ipc::PrincipalInfoToPrincipal instead? I can try to come up with a patch.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Christian Holler (:decoder) from comment #0)

I've prototyped a new libfuzzer-based fuzzing target that uses dom::ipc::StructuredCloneData as its entry point to cover the StructuredCloneReader code that is browser-only. The target code has not landed on mozilla-central yet, if you need it to reproduce (in case stack + test attached do not suffice), please let me know.

That's a great idea! Thanks for doing this, I imagine we'll find some useful bugs.

(In reply to Christian Holler (:decoder) from comment #3)

Nika, should we validate and handle this error in the PrincipalInfo::TNullPrincipalInfo case in mozilla::ipc::PrincipalInfoToPrincipal instead? I can try to come up with a patch.

Yeah, we should definitely validate that we're getting a valid moz-nullprincipal: URI when deserializing a null principal from PrincipalInfoToPrincipal. It'd be nice if we could validate more than just the moz-nullprincipal scheme as well, though I suppose that'll be more work.

Every valid moz-nullprincipal URI is currently created in NullPrincipal::CreateURI and so follows the pattern of moz-nullprincipal:{uuidhere-edae-4733-9a29-99abcace2a48} with an optional ?https://precursor.spec query at the end.

that being said, it occurs to me that we might want to include more validation in general when serializing & deserializing URIs, as right now the process might have some validation holes (potentially which this process will help us find!)

Comment 5

[Christian Holler (:decoder)]

Attachment: Bug 1742770 - Properly validate URI for null principals. r?nika

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Properly validate URI for null principals. r=nika
https://hg.mozilla.org/integration/autoland/rev/718e41d0127adc4e0facb59656e18131a677dc34
https://hg.mozilla.org/mozilla-central/rev/718e41d0127a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR approval when you get a chance. It grafts cleanly.

Comment 8

[Christian Holler (:decoder)]

I don't think we need this on ESR. This is only marked as sec-bug because of the parent dependency and the validation here is optional (without it, we still have the release assert in place). So I'd let this ride the trains.