1273364 - Crash in AppendCSPFromHeader due to null csp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: nsSystemPrincipal::EnsureCSP() should fail rather than succeed

This should fix some crashes in AppendCSPFromHeader.

Comment 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Nicholas Nethercote [inactive]]

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

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Nicholas Nethercote [inactive]]

(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)
	[e-mail] 0.11.620 	
	[e-mail] 1.2.6
	[e-mail] 1.0
	[e-mail]

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.

Comment 7

[Nicholas Nethercote [inactive]]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Nicholas Nethercote [inactive]]

Ok. I don't know anything about CSPs. I just want to fix the crashes :)

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 14

[Nicholas Nethercote [inactive]]

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?

Comment 15

[Pulsebot]

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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ac6078ac2b4