783049 - CSP : use existing/old parser for X-Content-Security-Policy header, new/CSP 1.0 spec compliant parser for Content-Security-Policy header

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ian Melven :imelven]

We want to implement the finalized CSP 1.0 spec - we also want to keep supporting (for some time) our existing implementation of CSP. There are semantic changes between our implementation and the spec.

Our proposal is to use the existing parser when a policy is served via the X-Content-Security-Policy header and the new parser that follows the 1.0 spec that we will create for bug 746978 when a policy is served via the officially spec'd Content-Security-Policy

I would suggest we also :
 * come up with a timeline for deprecating the old header - I don't think we should support both indefinitely
 * log a warning/deprecation message to the console when the old header is used, providing information about the deprecation time frame

Comment 1

[Ian Melven :imelven]

I'm wondering how this will interact with bug 663570 (csp from <meta>) - we have to do that for 1.0 support - should meta support both headers ??

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

I think <meta> should only support the new header name.

I also think that if there's a new header available, we *ignore* all the old ones (and log a warning).

Agree with the deprecation too -- we should come up with a timeline and phase it out.  Maybe something that lines up nicely across ESRs.

Comment 3

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #2)
> I think <meta> should only support the new header name.

makes sense to me.

> I also think that if there's a new header available, we *ignore* all the old
> ones (and log a warning).

yeah, that definitely sounds to me like the right thing to do.

> Agree with the deprecation too -- we should come up with a timeline and
> phase it out.  Maybe something that lines up nicely across ESRs.

lining up across ESR's sounds like a really good approach to me.

logging the warning could be done in another bug, but i think we definitely need to have the warning before this ships.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

I think it'll be trivial to tack the warning onto the patch that multiplexes parsing/semantics based on the header.  There'll be a big four-headed IF that checks the various headers' existences.  We can just dump a CSPWarning() about imminent deprecation and point to a wiki page where we can document the plan and timeline.

Comment 5

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #4)
> I think it'll be trivial to tack the warning onto the patch that multiplexes
> parsing/semantics based on the header.  There'll be a big four-headed IF
> that checks the various headers' existences.  We can just dump a
> CSPWarning() about imminent deprecation and point to a wiki page where we
> can document the plan and timeline.

oh right - i was thinking we had to do the work log to the console, but thanks to mgoodwin's work in Bug 712859 CSPWarning() does go to the console. very cool.

Comment 6

[Ian Melven :imelven]

A note that when discussing the XSS filter with jst, we talked about essentially a "content security service" that both the XSS filter and CSP would register with. You would then ask this service 'can i run this script?' and it would ask the XSS filter, check CSP etc. - one possible way to drive this work would be to implement which parser is used by registering a different CSP 'hook' with this service...

Comment 7

[Ian Melven :imelven]

Attachment: Part 1: WIP - C++ changes to read both CSP headers and call the right parser

The TODO's left for this part are to add the deprecation warning and the warning when both headers are sent. 

This bug also needs tests for behaving correctly when both headers are sent. The tests for the new parser itself should go in bug 746978 with the new parser code.

Comment 8

[Ian Melven :imelven]

Attachment: Part 2: WIP - IDL and JS changes for content security policy

Open question for this part : what to do about policy-uri ? It's not in the 1.0 spec. A couple of TODOs in this patch related to that.

Comment 9

[Ian Melven :imelven]

Attachment: Part 3: WIP - modify existing tests to add new arg to refinePolicy

Not much to say about this part, we will need to make versions of all the CSP tests that use the finalized directives etc in the spec, those might be better in bug 746978 though.

Comment 10

[Ian Melven :imelven]

A note that we plan to still support policy-uri and frame-ancestors directives when specified in a CSP 1.0 compliant Content-Security-Policy header - this require modifying the part 2 patch attached.

Comment 11

[Devdatta Akhawe [:devd]]

What was the spec decision on combining multiple policies? Did they go with intersection? I can't figure it out based on my reading of the spec. I recall there was some suggestion that CSP1.0 only support 1 policy, and no mechanism for combination.

Comment 12

[Devdatta Akhawe [:devd]]

(because, I noticed you are using refinepolicy calls in your patch)

Comment 13

[Tanvi Vyas[:tanvi]]

For CSP 1.0, if multiple headers are specified, we should go through each policy and block content as necessary.  A source will be blocked if it violates one more policies.  

Not sure if we decided to go through each policy one at a time and generate the report for that policy, or if we decided to intersect and then apply the intersected policy.  I believe it was the former, but dveditz would know.

Comment 15

[Ian Melven :imelven]

Attachment: Part 1 v2: WIP - C++ changes to read both CSP headers and call the right parser

The version I used to write patches for bug 746978

Comment 16

[Ian Melven :imelven]

Attachment: Part 2 v2: WIP - IDL and JS changes for content security policy

Version I used to write patches for bug 746978

Comment 17

[Ian Melven :imelven]

Attachment: Part 3 v2: WIP - modify existing tests to add new arg to refinePolicy

Version I used to write the patches for bug 746978

These need to be refactored on top of 790747, which I'm going to land shortly.

Comment 18

[Ian Melven :imelven]

Sid, Tanvi, Mark, do you think we want to make the messages for using the old headers or specifying the old headers go to the web console or error console ? I think probably the former ?

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

Both if possible, but for sure they should be deprecation warnings in the web console.

Comment 20

[Ian Melven :imelven]

Attachment: Part 1 v3 - C++ changes to read both CSP headers and call the right parser

Update the patch for bitrot. 

The next thing for this is to have the new header only checked for when a pref is set - then the mochitests for bug 746978 can set that pref to test the new parser/header and we can land this bug and bug 746978 but normal users won't get the new header/parser until we decide it's ready to flip on and remove the pref check.

Comment 21

[Ian Melven :imelven]

Axel, we would like to log warnings to the web console when the old, eventually to be deprecated CSP header is seen and when both the new header and the old header are both sent. Could you give me some tips/pointers on how to do this in the right way wrt localization please ?

Comment 22

[Axel Hecht [:Pike]]

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#3200 is the code path that content uses in general. Maybe you want use that utility code as such.

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 676811 [details] [diff] [review]
Part 1 v3 - C++ changes to read both CSP headers and call the right parser

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

::: content/base/src/nsDocument.cpp
@@ +2370,5 @@
>        NS_ASSERTION(appCSP, "App, but no default CSP in security.apps.certified.CSP.default");
>      }
>  
>      if (appCSP)
> +      csp->RefinePolicy(appCSP, chanURI, true);

This isn't gonna work unless there's changes to the IDL.  Should part 2 be applied before this patch?

Comment 24

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #23)
> Comment on attachment 676811 [details] [diff] [review]
> Part 1 v3 - C++ changes to read both CSP headers and call the right parser
> 
> Review of attachment 676811 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsDocument.cpp
> @@ +2370,5 @@
> >        NS_ASSERTION(appCSP, "App, but no default CSP in security.apps.certified.CSP.default");
> >      }
> >  
> >      if (appCSP)
> > +      csp->RefinePolicy(appCSP, chanURI, true);
> 
> This isn't gonna work unless there's changes to the IDL.  Should part 2 be
> applied before this patch?

oops - yeah, what's part 2 now should probably become part 1 and vice versa.

Comment 25

[Masatoshi Kimura [:emk]]

Can attackers disable CSP by injecting "Content-Security-Policy:" if the site still uses old headers?

Comment 26

[Ian Melven :imelven]

(In reply to Masatoshi Kimura [:emk] from comment #25)
> Can attackers disable CSP by injecting "Content-Security-Policy:" if the
> site still uses old headers?

yes, it seems like that would work, although if an attacker can inject headers, other attacks would also be possible.

Comment 27

[Sid Stamm [:geekboy or :sstamm]]

We want to move developers off the X- headers anyway, since they were more-or-less experimental and will be deprecated.  Injecting the new headers for disabling CSP should not be effective for long.

Comment 28

[Ian Melven :imelven]

(In reply to Masatoshi Kimura [:emk] from comment #25)
> Can attackers disable CSP by injecting "Content-Security-Policy:" if the
> site still uses old headers?

Actually, injecting an empty Content-Security-Policy header won't work the way the patches are written, but injecting something like "Content-Security-Policy: default-src *" would.

Comment 29

[Ian Melven :imelven]

Attachment: Part 1 v3 - IDL and JS changes for content security policy

I made the IDL and JS changes part 1, since the other parts depend on the IDL change

Comment 30

[Ian Melven :imelven]

Attachment: Part 2 v4 - C++ changes to read both CSP headers and call the right parser

Moved the C++ changes to part 2. I tried to use nsConsoleUtils::ReportToConsole in the same way it's now used elsewhere in the CSP code but it still only writes to the error console, not the web console.. It looks like the innerWindowID in nsContentUtils::ReportToConsoleNonLocalized is always 0 which may be the problem ?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 689025 [details] [diff] [review]
Part 1 v3 - IDL and JS changes for content security policy

>+CSPRep.fromStringSpecCompliant = function(aStr, aSpecCompliant, self, docRequest, csp) {

Drop the aSpecCompliant arg you're not passing in the caller, please.  r=me with that.

Comment 32

[Ian Melven :imelven]

Got some help from Mihai, going to dig into if there's some way I can get the window ID or maybe to defer the logging until later.

Comment 33

[Ian Melven :imelven]

Comment on attachment 689028 [details] [diff] [review]
Part 2 v4 - C++ changes to read both CSP headers and call the right parser

oops !

Comment 34

[Ian Melven :imelven]

Attachment: Part 3 v3 - tests

I added a mochitest that makes sure that the new header is used when both headers are sent.

Comment 35

[Ian Melven :imelven]

Comment on attachment 689028 [details] [diff] [review]
Part 2 v4 - C++ changes to read both CSP headers and call the right parser

After talking to Dan and Sid, we'd like to get the spec compliant parsing for CSP landed ASAP and will work on making the logging go to the web console instead of the error console in a followup rather than blocking on it. We still want to land the inline style blocking in bug 763879 with this and 746978 though, ie. we do want to block on that.

Comment 36

[Ian Melven :imelven]

Attachment: Part 1 v4 - IDL and JS changes for content security policy

(In reply to Boris Zbarsky (:bz) from comment #31)
> Comment on attachment 689025 [details] [diff] [review]
> Part 1 v3 - IDL and JS changes for content security policy
>
> Drop the aSpecCompliant arg you're not passing in the caller, please.  r=me
> with that.

Done ! Carrying over the r+, thanks Boris !

Comment 37

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #35)
>
> After talking to Dan and Sid, we'd like to get the spec compliant parsing
> for CSP landed ASAP and will work on making the logging go to the web
> console instead of the error console in a followup rather than blocking on
> it. 

This is now bug 821877

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 689028 [details] [diff] [review]
Part 2 v4 - C++ changes to read both CSP headers and call the right parser

>+++ b/content/base/src/nsDocument.cpp
>+    nsresult rv;
>+    nsCOMPtr<nsIPrefService> prefs =
>+      do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsIPrefBranch> prefBranch;
>+    rv = prefs->GetBranch(nullptr, getter_AddRefs(prefBranch));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    bool specCompliantEnabled = false;
>+    rv = prefBranch->GetBoolPref("security.csp.speccompliant",
>+                                 &specCompliantEnabled);

How about:

  bool specCompliantEnabled = 
    Preferences::GetBool("security.csp.speccompliant");

?

r=me with that.

Comment 39

[Ian Melven :imelven]

Comment on attachment 691610 [details] [diff] [review]
Part 3 v3 - tests

Boris, please feel free to suggest another reviewer if i'm sending too many r? your way :)

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 691610 [details] [diff] [review]
Part 3 v3 - tests

Does this need to land together with the API change?

I'm not sure I understand the test.  How does this:

+X-Content-Security-Policy: default-src 'self' ; img-src 'self' http://example.com

allow this load:

+<img src="http://example.org/nonexistent.jpg"></img>

?  Shouldn't the hostnames match?

Comment 41

[Ian Melven :imelven]

Attachment: Part 2 v5 - C++ changes to read both CSP headers and call the right parser

(In reply to Boris Zbarsky (:bz) from comment #38)
> Comment on attachment 689028 [details] [diff] [review]
> Part 2 v4 - C++ changes to read both CSP headers and call the right parser
> 
> How about:
> 
>   bool specCompliantEnabled = 
>     Preferences::GetBool("security.csp.speccompliant");
> 

Make this change as suggested by Boris, thanks ! Marking r+.

Comment 42

[Ian Melven :imelven]

Attachment: Part 3v4 - tests

(In reply to Boris Zbarsky (:bz) from comment #40)
> Comment on attachment 691610 [details] [diff] [review]
> Part 3 v3 - tests
> 
> Does this need to land together with the API change?

my tentative plan is to land these patches (after flattening into one patch in case we need to backout etc.) and the patches in bug 746978 (flattened into one patch for the same reason). We can then land bug 763879 when it's finished and remove the pref checking around using the new header - ie the pref checking is only there until inline style blocking is done and we are compliant with the CSP 1.0 spec, once that happens we want to start honoring the Content-Security-Policy header without any opt-in needed.

> I'm not sure I understand the test.  How does this:
> 
> +X-Content-Security-Policy: default-src 'self' ; img-src 'self'
> http://example.com
> 
> allow this load:
> 
> +<img src="http://example.org/nonexistent.jpg"></img>
> 
> ?  Shouldn't the hostnames match?

oops, thanks for catching that - i must have missed changing that one when i switched the tests to use example.org, fixed now, test still passes.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 693083 [details] [diff] [review]
Part 3v4 - tests

r=me assuming the checkin comment ends up being fixed up

Comment 44

[Ian Melven :imelven]

I pushed this, bug 746978 and bug 783049 to try (https://tbpl.mozilla.org/?tree=Try&rev=809af8ae403c) and found a problem: 

917 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/base/test/chrome/test_csp_bug773891.html | cross_origin : style should be blocked
919 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/base/test/chrome/test_csp_bug773891.html | cross_origin : script should be blocked
926 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/base/test/chrome/test_csp_bug773891.html | cross_origin : script should be blocked
933 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/base/test/chrome/test_csp_bug773891.html | cross_origin : script should be blocked

Comment 45

[Ian Melven :imelven]

Attachment: Part 2 v6 - C++ changes to read both CSP headers and call the right parser

Fix the failing test mentioned in comment 44 by moving the check for the old CSP header before the code that loads a CSP from a manifest for an app (hooray for tests!). Also clean up some whitespace that snuck in somehow. Carrying over the r+.

Did another try push of the CSP 1.0 patches: https://tbpl.mozilla.org/?tree=Try&rev=dbd35bff60c5

Comment 46

[Ian Melven :imelven]

Going to try to land this bug and bug 746978 - the CSP 1.0 parser will land pref'd off.

https://tbpl.mozilla.org/?tree=Try&rev=39176223dd44

Comment 47

[Ian Melven :imelven]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1569edaba5fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d85b53c015
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6907c8de98

Comment 48

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1569edaba5fc
https://hg.mozilla.org/mozilla-central/rev/d4d85b53c015
https://hg.mozilla.org/mozilla-central/rev/cb6907c8de98

Comment 49

[Francesco Lodolo [:flod]]

BothCSPHeadersPresent=This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-ContentSecurityPolicy/ReportOnly header(s) will be ignored.

Why did you switch from "X-Content-Security-Policy/Report-Only" to "X-ContentSecurityPolicy/ReportOnly"? I guess that's an error

Comment 50

[Ian Melven :imelven]

(In reply to Francesco Lodolo [:flod] from comment #49)
> BothCSPHeadersPresent=This site specified both an
> X-Content-Security-Policy/Report-Only header and a
> Content-Security-Policy/Report-Only header. The
> X-ContentSecurityPolicy/ReportOnly header(s) will be ignored.
> 
> Why did you switch from "X-Content-Security-Policy/Report-Only" to
> "X-ContentSecurityPolicy/ReportOnly"? I guess that's an error

oops, yeah, that's a typo - thanks for catching that - I'll take care of it.

Comment 51

[Ian Melven :imelven]

The pref to enable this won't be turned on in Fx21 so I suspect this doesn't need a relnote yet ?