1358250 - Crash in ContentPrincipal::Init

Status: RESOLVED INCOMPLETE

(Core :: Security: CAPS, defect)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-54c19257-adb6-42e7-89ec-659b40170420.
=============================================================

Only two crashes in the last week, from a single installation, but this is a release assert in CAPS code so I figured it would be worth filing a bug.

MOZ_RELEASE_ASSERT(((bool)(!!(!NS_FAILED_impl(NS_URIChainHasFlags(aCodebase, nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT, &hasFlag))))) && !hasFlag)

Comment 1

[Andrew McCreight [:mccr8]]

Bobby, how alarmed should we be that we are hitting this assertion? ;)

Comment 2

[Kris Maglione [:kmag]]

This is happening during deserialization, so I guess the possibilities are a) the protocol has different flags in the parent and content process, b) something is being serialized incorrectly, and we're trying to create a codebase principal in the parent that was something else in the child, or c) someone managed to change the codebase URI after the principal was created, and that broke when we tried to clone it.

Would definitely be good to know one way or the other, though. Can someone get the crash URLs from the reports?

Comment 3

[Andrew McCreight [:mccr8]]

I have crash dump access, but the only URL is about:blank.

Comment 4

[Kris Maglione [:kmag]]

Ah. Well that may actually be relevant, since about:blank inherits security context. But it looks like the deserialization is happening in CSS code. For a XUL image frame. Which is odd.

And it looks like we're actually deserializing its loading principal, which I guess means it's coming from places: http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/browser/base/content/browser-places.js#1546

It seems like we should always have a system principal there.

It looks like Ehsan is responsible for that code, so I suppose he should know more.

Comment 5

[Bobby Holley (:bholley)]

The stack in comment 0 led me to [1], where we appear to be parsing a principal out of a string attribute on a DOM node, which seems incredibly unsafe. Why the heck are we doing that?

If the answer is "because these DOM nodes are system principal", we should:
(a) MOZ_RELEASE_ASSERT that fact, and
(b) Call CreateCodebasePrincipal rather than NS_DeserializeObject, since the former is robust against malformed strings.

Those two things should be done regardless of whatever else happens in this bug.

[1] http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/dom/base/nsContentUtils.cpp#10024

Comment 6

[Kris Maglione [:kmag]]

Ah. It looks like there's also a bunch of sessionrestore code that sets iconLoadingPrincipal, which gets mapped to loadingprincipal. So we're probably just trying to deserialize old principals that were stored before this check.

We should probably store those as origin strings rather than binary serializations, to avoid weird breakage if the format changes in the future...

Comment 7

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #6)
> Ah. It looks like there's also a bunch of sessionrestore code that sets
> iconLoadingPrincipal, which gets mapped to loadingprincipal. So we're
> probably just trying to deserialize old principals that were stored before
> this check.
> 
> We should probably store those as origin strings rather than binary
> serializations, to avoid weird breakage if the format changes in the
> future...

Makes a lot of sense to me.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

This bug was filed 3 years ago - in the meantime we landed a complete new serialization mechanism for principals based on JSON (see Bug 1508939) which renders this bug INCOMPLETE.