Crash in AppendCSPFromHeader due to null csp

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Security
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: njn, Assigned: njn)

Tracking

({crash})

Trunk
mozilla49
Unspecified
Windows 8
crash
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [domsecurity-active], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-59241869-a0cb-4be2-9596-584d82160510.
=============================================================

13 crashes with this signature in the past week: 10 in FF46 and 3 in FF49.

The cause is straightforward:

> 0 	xul.dll 	AppendCSPFromHeader 	dom/base/nsDocument.cpp:2652
> 1 	xul.dll 	nsDocument::InitCSP(nsIChannel*) 	dom/base/nsDocument.cpp:2923

In AppendCSPFromHeader, the passed-in |csp| is null. The only way I can see that happening is if the |principal| in InitCSP() is an nsSystemPrincipal, because nsSystemPrincipal::EnsureCSP() is trivial -- it does not set the |csp| but it *does* return NS_OK, indicating success.

Assuming that's right, the fix is easy: change nsSystemPrincipal to return NS_ERROR_FAILURE instead.
(Assignee)

Comment 1

2 years ago
Created attachment 8753178 [details] [diff] [review]
nsSystemPrincipal::EnsureCSP() should fail rather than succeed

This should fix some crashes in AppendCSPFromHeader.
Attachment #8753178 - Flags: review?(ckerschb)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
BTW, I checked all the calls to EnsureCSP(). There are three of them. In all three cases having EnsureCSP() causes an early return, which seems like the right thing to do.
(Assignee)

Comment 3

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #2)
>
> BTW, I checked all the calls to EnsureCSP(). There are three of them. In all
> three cases having EnsureCSP() causes an early return, which seems like the
> right thing to do.

The first of those call sites is in nsDocument::InitCSP(), which is the one that prompted me to file this bug.

The second of those three call sites is in mozilla::dom::HTMLMetaElement::BindToTree(), and I just discovered that we've had 10 crashes in that function in the past week, and 5 of those are due to a null |csp|. So this patch should fix those crashes too.

The third of those call sites is in AddonContentPolicy::ValidateAddonCSP(), but I don't see any crashes there.
Crash Signature: [@ AppendCSPFromHeader] → [@ AppendCSPFromHeader] [@ mozilla::dom::HTMLMetaElement::BindToTree]
Boris, is it OK to return NS_ERROR_FAILURE if we try to set a CSP on a SystemPrincipal or should we actually never even try to set a CSP on a SystemPrincipal?
When looking for callsites of EnsureCSP() I realized that we wouldn't call SetMetaReferrer() [1] because principal->EnsureCSP() on a systemPrincipal would return NS_ERROR_FAILURE and hence we would bail out of that function early. What do you think?


@ Nick, thanks for filing and providing the patch!

[1] http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLMetaElement.cpp#148
Flags: needinfo?(bzbarsky)
Whiteboard: [domsecurity-active]
Hmm.  So in nsDocument::InitCSP presumably we ended up with an actual HTTP channel with CSP headers, but our principal is system?  That seems ... very fishy.  But certainly something add-ons can trigger.  I don't really know what we want to do in that situation; possibly ignoring the CSP headers is the right thing, but the other option is to just refuse to load the document at all.  Are there any addon corelations in these crashes?

The SetMetaReferrer thing is basically a question about whether we want to allow setting a referrer policy via <meta> in a system document.  Seems like we should be able to do that.  Should system documents be able to do CSP via <meta>?

In AddonContentPolicy::ValidateAddonCSP we have just created the principal with BasePrincipal::CreateCodebasePrincipal, so it's not a system principal.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 6

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #5)
> Hmm.  So in nsDocument::InitCSP presumably we ended up with an actual HTTP
> channel with CSP headers, but our principal is system?  That seems ... very
> fishy.  But certainly something add-ons can trigger.  I don't really know
> what we want to do in that situation; possibly ignoring the CSP headers is
> the right thing, but the other option is to just refuse to load the document
> at all.  Are there any addon corelations in these crashes?

I can never get crash-stats' correlations thingy to work, but I looked through 18 crashes manually (14 from InitCSP() and 4 from BindToTree()) and the common set was all the ones we ship, e.g.:

 	{972ce4c6-7e08-4474-a285-3208198ce6fd} 46.0.1 (Firefox theme)
	shumway-dev@research.mozilla.org 0.11.620 	
	loop@mozilla.org 1.2.6
	e10srollout@mozilla.org 1.0
	firefox@getpocket.com

So, nothing obvious there.

> In AddonContentPolicy::ValidateAddonCSP we have just created the principal
> with BasePrincipal::CreateCodebasePrincipal, so it's not a system principal.

Which fits with the observation that AddonContentPolicy::ValidateAddonCSP() is the one call site to AppendCSPFromHeader in which we haven't seen a crash.

I also looked at the crashing URLs: some facebook.com, some about:newtab, some about:blank, some other random sites. No obvious pattern there, either.
(Assignee)

Comment 7

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Boris, is it OK to return NS_ERROR_FAILURE if we try to set a CSP on a
> SystemPrincipal or should we actually never even try to set a CSP on a
> SystemPrincipal?
> When looking for callsites of EnsureCSP() I realized that we wouldn't call
> SetMetaReferrer() [1] because principal->EnsureCSP() on a systemPrincipal
> would return NS_ERROR_FAILURE and hence we would bail out of that function
> early. What do you think?

One possibility is to do the SetMetaReferrer() call even if EnsureCSP() fails, just before doing an early return.

However, in BindToTree() there are already multiple ways to fail early, in which case SetMetaReferrer() would not be called. This patch just adds one more way (instead of crashing). So if this early fail is a problem, wouldn't all the other early fails be problems too?
The other failures in BindToTree are basically things like OOM.  In those cases, recovery is not really possible.

What we're talking about here, on the other hand, is what the logic flow is in conditions that are arguably very predictable and not that hard to produce.  The only question is how we want to handle them.
(Assignee)

Comment 9

2 years ago
Ok. I don't know anything about CSPs. I just want to fix the crashes :)
Comment on attachment 8753178 [details] [diff] [review]
nsSystemPrincipal::EnsureCSP() should fail rather than succeed

Review of attachment 8753178 [details] [diff] [review]:
-----------------------------------------------------------------

From a CSP point of view it doesn't make sense to attach a CSP to a SystemPrincipal. Also, given the function is called EnsureCSP() I would expect it to return an error if the CSP can't be attached the principal.

I think all the other issues we have been discussing are separate issues. Sure, we could call SetMetaReferrer() even if we can't attach the CSP to the systemPrincipal, but I would expect that in all the cases where a meta referrer policy is relevant we are dealing with a codeBasePrincipal and not a systemPrincipal.

I am fine with accepting the patch, but would also like to get bz's approval (but it seems he is not accepting review requests at the moment).
Attachment #8753178 - Flags: review?(ckerschb) → review+
> but I would expect that in all the cases where a meta referrer policy is relevant we are dealing with a
> codeBasePrincipal and not a systemPrincipal.

The fact that we clearly have system-principal HTTP(S)-url pages is a clear counterexample, no?  In those cases a meta referrer policy, if there were one, would obviously be relevant.
Or put another way, given that such documents exist, if any of them are using a meta referrer policy then by ignoring it we're probably introducing a privacy leak because they're part of the browsers UI (being system principal; yeah, this part is insane).

Now a privacy leak is better than a crash, which is what they get now, but once we fix the crash they can start using a meta referrer policy and never realize that it's not working....
Created attachment 8754803 [details] [diff] [review]
bug_1273364_ensurecsp_bug.patch

(In reply to Boris Zbarsky [:bz] from comment #12)
> Now a privacy leak is better than a crash, which is what they get now, but
> once we fix the crash they can start using a meta referrer policy and never
> realize that it's not working....

Agreed, how about this patch? We would call SetMetaReferrer even if we try to call EnsureCSP() on a systemPrincipal?
Attachment #8753178 - Attachment is obsolete: true
Attachment #8754803 - Flags: review?(n.nethercote)
(Assignee)

Comment 14

2 years ago
Comment on attachment 8754803 [details] [diff] [review]
bug_1273364_ensurecsp_bug.patch

Review of attachment 8754803 [details] [diff] [review]:
-----------------------------------------------------------------

Seems ok to me. Are you happy to land it?
Attachment #8754803 - Flags: review?(n.nethercote) → review+

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac6078ac2b4

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ac6078ac2b4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

a year ago
Duplicate of this bug: 1290905
You need to log in before you can comment on or make changes to this bug.