1591023 - Assertion failure: ((bool)(__builtin_expect(!!(!NS_FAILED_impl(rv)), 1))), at caps/NullPrincipal.cpp:67

Status: RESOLVED INCOMPLETE

(Core :: DOM: Content Processes, defect, P3)

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

Backtrace:

==14204==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x7fafc5b0d2cf bp 0x7ffe44439cd0 sp 0x7ffe44439cb0 T0)
==14204==The signal is caused by a WRITE memory access.
==14204==Hint: address points to the zero page.
    #0 0x7fafc5b0d2ce in mozilla::NullPrincipal::Create(mozilla::OriginAttributes const&, nsIURI*) caps/NullPrincipal.cpp:67:3
    #1 0x7fafc41f158a in mozilla::ipc::PrincipalInfoToPrincipal(mozilla::ipc::PrincipalInfo const&, nsresult*) ipc/glue/BackgroundUtils.cpp:74:19
    #2 0x7fafc5b1748b in nsJSPrincipals::ReadKnownPrincipalType(JSContext*, JSStructuredCloneReader*, unsigned int, JSPrincipals**) caps/nsJSPrincipals.cpp:312:33
    #3 0x7fafc73fc7ae in mozilla::dom::StructuredCloneHolder::ReadFullySerializableObjects(JSContext*, JSStructuredCloneReader*, unsigned int) dom/base/StructuredCloneHolder.cpp:346:10
    #4 0x7fafc73ff828 in mozilla::dom::StructuredCloneHolder::CustomReadHandler(JSContext*, JSStructuredCloneReader*, unsigned int, unsigned int) dom/base/StructuredCloneHolder.cpp:925:10
    #5 0x7fafd1c429f8 in JSStructuredCloneReader::startRead(JS::MutableHandle<JS::Value>) js/src/vm/StructuredClone.cpp:2604:23
    #6 0x7fafd1c2ad64 in JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>) js/src/vm/StructuredClone.cpp:2893:8
    #7 0x7fafd1c2a9b8 in ReadStructuredClone(JSContext*, JSStructuredCloneData&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:644:12
    #8 0x7fafc73fc3ed in ReadFromBuffer dom/base/StructuredCloneHolder.cpp:318:8
    #9 0x7fafc73fc3ed in mozilla::dom::StructuredCloneHolder::ReadFromBuffer(nsISupports*, JSContext*, JSStructuredCloneData&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) dom/base/StructuredCloneHolder.cpp:300:3
    #10 0x7fafc1d21603 in FuzzingRunDomSC(unsigned char const*, unsigned long) dom/base/fuzztest/FuzzStructuredClone.cpp:72:10
    #11 0x557d34e9fbe4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) tools/fuzzing/libfuzzer/FuzzerLoop.cpp:529:15
[...]

The crash hit here is a MOZ_RELEASE_ASSERT so this is not a security problem, only a potential cross-process crash. I am still filing this s-s because the parent bug is currently s-s (we can unhide once the parent landed).

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Jim Mathies [:jimm]]

Jason. looks like you've been working on these structured close fuzzing bugs. Is this something we should get fixed?

Comment 4

[Jason Orendorff [:jorendorff]]

Yes, I recommend fixing this.

As I understand it: Structured cloning is used to communicate between content and chrome processes. That's a trust boundary, so it's security-sensitive. It's not always obvious which parts of deserialization are really attack surface; but for simplicity, we treat crashes in deserialization as presumed-bad.

Good news is, they're usually easy to fix. The most common bug is that the original author just missed the rule: Deserializers must report errors on invalid input, not assert or do UB.

...But in this case maybe it's not that. Maybe the assertion is just wrong. Is it really true NullPrincipal::Init can't fail?

Comment 5

[Jason Orendorff [:jorendorff]]

I mean, it's a release assert, no obvious hole. The bigger questions are, what else is lurking, and have we been systematically failing to follow this rule? So I figure, fix this to get it out of the way, and let the fuzzer tell us those answers.

Comment 6

[Christian Holler (:decoder)]

Attachment: Bug 1591023 - Make NullPrincipal::Create fallible for fuzzing. r?haik

Comment 7

[Christian Holler (:decoder)]

Actually my proposed fix is incomplete because mozilla::NullPrincipal::Create apparently has more callers that don't check the return value and these can also be reached via IPC. I'll revisit my patch and post an update once I tested it.

Comment 8

[Boris Zbarsky [:bzbarsky]]

A few things:

1. The fundamental problem is that most callers of NullPrincipal::Create don't have any sane way to handle failure. We could have two entrypoints, I suppose: an infalliable one and a fallible one, with deserialization using the fallible one...

2. It looks like the deserialization case does have a caller-controlled way to cause deserialization to fail: by passing a bogus URI. That seems to be a regression from bug 1318727 and we should definitely have a fallible thing for that case! For the case when no URI is provided, the crash here is no different from a small-allocation OOM, right? In particular, in that case it can only crash via this release-assert if NullPrincipalURI::Create returns null, which I just looked and can't actually happen. I filed bug 1599470 to fix that.

3. I don't understand the attached testcase.... It's just some sort of binary blob, afaict. How does one use it?

Comment 9

[Haik Aftandilian [:haik]]

I think it's OK to add these #ifdef FUZZING checks, but when possible it's better if we change the production code to deal with the error. Boris's suggestion for a FallibleCreate sounds like a good one. Do you want to address that with this bug or keep it to a FUZZING fix?

Comment 10

[Christian Holler (:decoder)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

3. I don't understand the attached testcase.... It's just some sort of binary blob, afaict. How does one use it?

The attached testcase is the input to a fuzzing target that hasn't been landed to mozilla-central yet (because we are still running it locally to ensure that we are not 0-day'ing ourselves). The target roughly does this:

  StructuredCloneData scdata;

  if (!scdata.CopyExternalData(reinterpret_cast<const char*>(data), size))
    return 0;

  JS::RootedValue result(cx);
  ErrorResult rv;
  scdata.Read(cx, &result, rv);

  rv.SuppressException();

where data is the input attached. Once this target has landed to mozilla-central, it will be easy to run this just by producing a fuzzing build and specifying the target name. Until that, the only way is to add this code into a gtest or ping me for the patch that adds the target (I'm about to push it to bug 1590068).

Comment 11

[Christian Holler (:decoder)]

(In reply to Haik Aftandilian [:haik] from comment #9)

I think it's OK to add these #ifdef FUZZING checks, but when possible it's better if we change the production code to deal with the error. Boris's suggestion for a FallibleCreate sounds like a good one. Do you want to address that with this bug or keep it to a FUZZING fix?

I think addressing this without a FUZZING ifdef is always preferable, so I think we should do that. Will you be working on this, or shall I try it?

Comment 12

[Boris Zbarsky [:bzbarsky]]

The attached testcase is the input to a fuzzing target

Ah. Then yeah, this is likely running into comment 8 item 2.

Comment 13

[Jens Stutte [:jstutte]]

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

(In reply to Haik Aftandilian [:haik] from comment #9)

I think it's OK to add these #ifdef FUZZING checks, but when possible it's better if we change the production code to deal with the error. Boris's suggestion for a FallibleCreate sounds like a good one. Do you want to address that with this bug or keep it to a FUZZING fix?

I think addressing this without a FUZZING ifdef is always preferable, so I think we should do that. Will you be working on this, or shall I try it?

:decoder, you are still the assignee here. Are you planning any steps forward here? Thanks!

Comment 14

[Jens Stutte [:jstutte]]

So the cases we see in crash-stats seem slightly different and no nullptr crashes. It looks on Win11 as if we have a bogus RIP that points 1 byte before NullPrincipal::Create actually starts. Not sure how actionable those are, fortunately they are very rare.

:decoder told me that fuzzers are not tripping over this anymore, though he is not aware of any relevant changes. Let's keep this open for another while for observation.

Comment 15

[Tom Ritter [:tjr]]

So this is a startup crash, of the 111 reports in 6 months they come from 48 people (most people crash more than once) and it seems like it's stopping them from starting Firefox, at which point presumably they give up. 70% x64, 94% Windows.

The majority of them go through nsContentUtils::Init | nsLayoutStatics::Initialize | nsLayoutModuleInitialize and the overwhelming majority go through nsLayoutStatics::Initialize | nsLayoutModuleInitialize.

17% are the breakpoint release assert from the original issue. Given the relatively rare occurrance and how it's no longer related to the original issue, I'm going to close this out.