925186 - X-CSP missing default-src/allow overrides the non-prefixed CSP header and defaults to 'none'

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Neil Matatall]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.69 Safari/537.36

Steps to reproduce:

Test case: http://fathomless-taiga-4659.herokuapp.com/

Applied 
    content-security-policy: script-src 'self'
    x-content-security-policy: script-src 'self'


Actual results:

Content Security Policy: 'allow' or 'default-src' directive required but not present.  Reverting to "default-src 'none'"

All content related to unspecified directives is blocked.


Expected results:

Content from unspecified directives should not be blocked. Apparently this is more "per the spec", but currently it _appears_ there is no spec text around this behavior. I sent a feeler out to the webappsec list for definition http://lists.w3.org/Archives/Public/public-webappsec/2013Oct/0052.html

Comment 1

[Garrett Robinson [:grobinson]]

(In reply to Neil Matatall from comment #0)
> User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5)
> AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.69 Safari/537.36
> 
> Steps to reproduce:
> 
> Test case: http://fathomless-taiga-4659.herokuapp.com/
> 
> Applied 
>     content-security-policy: script-src 'self'
>     x-content-security-policy: script-src 'self'
> 
> 
> Actual results:
> 
> Content Security Policy: 'allow' or 'default-src' directive required but not
> present.  Reverting to "default-src 'none'"
> 
> All content related to unspecified directives is blocked.

There are two issues here. The first is whether a "default-src" directive should be required. This was true for Firefox's pre-1.0 CSP, but it is not true for our current CSP 1.0 implementation. This position was clarified in the discussion on webappsec, and language has been added to the latest draft of the spec to clarify this.

The second is that, given both a prefixed header and a non-prefixed header, Firefox should prefer the non-prefixed header and *ignore* the prefixed header. This is a regression, likely caused by changes from Bug 836922.

Comment 2

[Ian Melven :imelven]

(In reply to Garrett Robinson [:grobinson] from comment #1)
>
> There are two issues here. The first is whether a "default-src" directive
> should be required. This was true for Firefox's pre-1.0 CSP, but it is not
> true for our current CSP 1.0 implementation. This position was clarified in
> the discussion on webappsec, and language has been added to the latest draft
> of the spec to clarify this.

right, it's not required in a 1.0/unprefixed policy.

> The second is that, given both a prefixed header and a non-prefixed header,
> Firefox should prefer the non-prefixed header and *ignore* the prefixed
> header. This is a regression, likely caused by changes from Bug 836922.

that was my guess as well. I am sad the tests for bug 783049 didn't catch this :(

Comment 3

[Garrett Robinson [:grobinson]]

(In reply to Ian Melven :imelven from comment #2)
> that was my guess as well. I am sad the tests for bug 783049 didn't catch this :(

Me too :( That test (test_bothCSPHeaders.html) tests whether a resource load is allowed or blocked, where the resource is allowed by the X-Content-Security-Policy header but blocked by the Content-Security-Policy header; that is, the test passes if the resource load is blocked. However, in this case when a) both headers are specified and b) the prefixed header is invalid, we fail closed 
(defaults to "default-src 'none'"), so the test passes (but should not).

This test should additionally have the Content-Security-Policy allow a load that isn't allowed by the X-Content-Security-Policy. That way, we can determine if the Content-Security-Policy ran (and won't be fooled by the fail-closed behavior), and can still determine that the X-Content-Security-Policy *wasn't* enforced.

Comment 4

[Ian Melven :imelven]

(In reply to Garrett Robinson [:grobinson] from comment #3)
>
> Me too :( That test (test_bothCSPHeaders.html) tests whether a resource load
> is allowed or blocked, where the resource is allowed by the
> X-Content-Security-Policy header but blocked by the Content-Security-Policy
> header; that is, the test passes if the resource load is blocked. However,
> in this case when a) both headers are specified and b) the prefixed header
> is invalid, we fail closed 
> (defaults to "default-src 'none'"), so the test passes (but should not).
> 
> This test should additionally have the Content-Security-Policy allow a load
> that isn't allowed by the X-Content-Security-Policy. That way, we can
> determine if the Content-Security-Policy ran (and won't be fooled by the
> fail-closed behavior), and can still determine that the
> X-Content-Security-Policy *wasn't* enforced.

I agree. Good analysis and explanation !

Comment 5

[Garrett Robinson [:grobinson]]

Attachment: 1-test.patch

Updates test_bothCSPHeaders.html (from Bug 783049) to catch the case where an invalid prefixed header fails closed.

Comment 6

[Garrett Robinson [:grobinson]]

Attachment: 1.patch

Fix regression caused by Bug 836922 in handling the simultaneous use of the prefixed and unprefixed headers.

The switch to this new behavior in the patch for 836922 was quite deliberate (accompanied by comment changes). However, I do not believe this is the right approach, especially given cases like this one (if either one of the headers is incorrect, everything breaks). The point is to allow websites to serve both the old and new headers, so browsers that only understand the prefixed header still get CSP protection, but browsers that understand the unprefixed header will use it instead. I'm sorry I didn't flag this in my review.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 815578 [details] [diff] [review]
1-test.patch

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

I'm concerned that removing waitForExplicitFinish() will introduce a random orange.  Fix so we still have waitForExplicitFinish(), and explicitly catch the success or failures we expect.

With that change and the nits below, r=me.

::: content/base/test/csp/test_bothCSPheaders.html
@@ +15,5 @@
>  <iframe style="width:200px;height:200px;" id='cspframe'></iframe>
>  <script class="testbody" type="text/javascript">
>  
> +var prefixedHeaderImgURL = "http://example.org/prefixed.jpg";
> +var unprefixedHeaderImgURL = "http://mochi.test:8888/unprefixed.jpg"

I'm a little leary of hardcoding mochi.test:8888 here, but I think it's fine.

@@ +37,5 @@
> +      if (asciiSpec == prefixedHeaderImgURL) {
> +        ok(false, "Load was allowed - the Content-Security Policy header does not allow the load, the X-Content-Security header should be ignored");
> +      } else if (asciiSpec == unprefixedHeaderImgURL) {
> +        ok(true, "Load was allowed - the Content Security Policy header allows the load");
> +      }

Update the comment above that says this is a fail.

@@ +48,5 @@
> +      if (asciiSpec == prefixedHeaderImgURL) {
> +        ok(true, "Load was blocked - the Content-Security-Policy header doesn't allow the load, the X-Content-Security-Header does but should have been ignored");
> +      } else if (asciiSpec == unprefixedHeaderImgURL) {
> +        ok(false, "Load was blocked - the Content-Security-Policy header allows the load, but it was blocked.");
> +      }

This, and the if block above both feel a little awkward to me.
It took me a while to understand why we have ok(true and ok(false.
What do you think about something like this pseudocode:

if(asciiSpec in [prefixedHeaderImgURL, unprefixedHeaderImgURL]) {
  ok(asciiSpec == prefixedHeaderURL, ...);
}

This way it's easy to see that
1. We're only examining things in that list for this topic
2. The only acceptable thing from that list is the one in the ok argument.

Alternatively, you could add a comment that says something like "we only care about two URLs, one is success, one is fail, all otherURLs don't matter."

@@ +62,5 @@
>  }
>  
>  window.examiner = new examiner();
>  
> +//SimpleTest.waitForExplicitFinish();

Why is this commented out?  The observer notifications are async.

@@ +73,5 @@
> +    cspframe.addEventListener('load', function cleanup() {
> +      // All of the observers will have fired once the document is loaded, so
> +      // we can safely remove the examiner at this point (which we need to do
> +      // so we don't interfere with other tests)
> +      window.examiner.remove();

Are you absolutely sure all the observers will have fired?  What if we make some loads pipelined or async?

I'd rather have us update a counter and catch two explict fails or explicit successes.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 815580 [details] [diff] [review]
1.patch

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

Yeah, look at me, I changed the semantics of our code at a whim and broke stuff.  Looks like the right error messages are dumped into the web console when both headers are present, but I just messed up ignoring the X- headers when I fixed bug 836922.

Looks good to me, thanks for fixing.

Comment 9

[Garrett Robinson [:grobinson]]

Attachment: 2-test.patch

r=sstamm with changes from previous comment. Confirmed that this fails without 1.patch and passes with it.

Weirdly,

  if (asciiSpec in [x, y])

does not work (no error, but does not evaluate to true), but

  if (asciiSpec == x || asciiSpec == y)

does. This might be something to do with QI magic. Is it a bug (follow-up)?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4e4133d75aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7072b16be9f

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d4e4133d75aa
https://hg.mozilla.org/mozilla-central/rev/a7072b16be9f

Comment 12

[Garrett Robinson [:grobinson]]

Comment on attachment 815580 [details] [diff] [review]
1.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 836922
User impact if declined: Sites that serve both the prefixed and unprefixed CSP headers will have unpredictable behavior, and will likely be broken
Testing completed (on m-c, etc.): CSP test suite passes on m-c
Risk to taking this patch (and alternatives if risky): Minimal; only affects CSP
String or IDL/UUID changes made by this patch: None

Comment 13

[Garrett Robinson [:grobinson]]

Comment on attachment 816171 [details] [diff] [review]
2-test.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 836922
User impact if declined: None (test)
Testing completed (on m-c, etc.): CSP tests pass on m-c
Risk to taking this patch (and alternatives if risky): None (improves test coverage in CSP test suite)
String or IDL/UUID changes made by this patch: None

Comment 14

[Garrett Robinson [:grobinson]]

Hey Sid, can you uplift this whenever is convenient?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7b483e309add
https://hg.mozilla.org/releases/mozilla-aurora/rev/45a0303e9b50

Comment 16

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on latest Aurora (buildID: 20131024004004) and latest Nightly (buildID: 20131024030204).