1012789 - CSP in C++: Ignore XCSP instead of spec compliant CSP header

Status: RESOLVED INVALID

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

The X-CSP-header has been deprecated for some time now (IIRC Firefox 23). Currently, if the speccompliant flag is not set, we ignore the CSP header which is misleading in my opinion [1].

What we should do is:
If the |csp.newbackend = true| then we can completely ignore |specCompliant|. This change avoids cumbersome debugging by simply using the spec compliant header if available.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2713

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1012789.patch

Please keep in mind that we cannot simply remove the speccompliant flag right now, therefore the attached patch provides the perfect interim solution till we can flip csp.newbackend switch forever.

Also note, if one of the mochitests uses a spec-compliant policy but has not pushed the |specCompliant=true| in additon, when running the tests, then spec compliant policies are ignored at the moment. Exactly what we experienced with the 30*_redirect_tests.

Comment 2

[Garrett Robinson [:grobinson]]

Comment on attachment 8424986 [details] [diff] [review]
bug_1012789.patch

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

As discussed on IRC, the right way to solve the test_30*_redirect.html test failures, IMO, is to pushPrefEnv the speccompliant pref to true in the mochitest_changeover patch. That patch changes those tests (file_redirect_content.sjs) to send CSP headers instead of the X-CSP headers they were sending previously, but does not push the speccompliant pref. Since that pref is not on by default on B2G (it is on desktop), we get the failures. This is trivally fixable by pushing the pref for those 4 tests.

::: content/base/src/nsDocument.cpp
@@ +2710,5 @@
>      Preferences::GetBool("security.csp.speccompliant");
>  
> +  // If the new backend (C++ version of CSP) is enabled, we ignore
> +  // X-CSP header instead of spec compliant CSP headers. We do this
> +  // by ignoring the speccompliant flag.

Do not do this. This means we cannot reason about the speccompliant flag. The speccompliant flag should be independent of which backend(s) we are using. When we're ready to switch to only supporting CSP 1.x, it should be removed entirely.

Comment 3

[Garrett Robinson [:grobinson]]

On IRC, Christoph correctly pointed out that if we want both parsers to coexist, we need to change the logic here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2808

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Garrett Robinson [:grobinson] from comment #3)
> On IRC, Christoph correctly pointed out that if we want both parsers to
> coexist, we need to change the logic here:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.
> cpp#2808

If we want to parse CSP-headers with the new parser and XCSP headers with the old parser we have to update that code. Correct!

Either way, I think we should favor CSP headers instead of XCSP headers, in case both are present. And this decision should *not* depend on whether the speccompliant flag was set or not. We should go forward and rather truncate XCSP headers instead of CSP headers, when both are present. Why would we favour something that has been deprecated for quite some time? Doesn't make sense at all, please see:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2717

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

As discussed with Garrett and Sid in person, we are going to use the new C++ implementation when possible, and fall back to the old JS implementation for XSCSP headers. We perform that change in Bug 1013559 - Support both CSP parsers coexisting; which makes this bug invalid in the end.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8424986 [details] [diff] [review]
bug_1012789.patch

Also clearing the review flag for Sid!