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

RESOLVED FIXED in mozilla21

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
6 months ago

People

(Reporter: imelven, Assigned: imelven)

Tracking

(Blocks: 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 11 obsolete attachments)

6.85 KB, patch
imelven
: review+
Details | Diff | Splinter Review
7.26 KB, patch
Details | Diff | Splinter Review
12.42 KB, patch
imelven
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
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 ??
Blocks: 663566
(Assignee)

Updated

5 years ago
Blocks: 746978, 763879, 764937
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.
(Assignee)

Comment 3

5 years ago
(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.
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.
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
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...
(Assignee)

Updated

5 years ago
Assignee: nobody → imelven
Blocks: 770176
(Assignee)

Comment 7

5 years ago
Created attachment 656656 [details] [diff] [review]
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.
(Assignee)

Comment 8

5 years ago
Created attachment 656658 [details] [diff] [review]
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.
(Assignee)

Comment 9

5 years ago
Created attachment 656663 [details] [diff] [review]
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.
(Assignee)

Comment 10

5 years ago
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.
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.
(because, I noticed you are using refinepolicy calls in your patch)
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.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 638320
(Assignee)

Comment 15

5 years ago
Created attachment 661345 [details] [diff] [review]
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
Attachment #656656 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Created attachment 661347 [details]
Part 2 v2: WIP - IDL and JS changes for content security policy

Version I used to write patches for bug 746978
Attachment #656658 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 661356 [details] [diff] [review]
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.
Attachment #656663 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
No longer blocks: 746978
(Assignee)

Comment 18

5 years ago
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 ?
Blocks: 792161
Both if possible, but for sure they should be deprecation warnings in the web console.
(Assignee)

Comment 20

5 years ago
Created attachment 676811 [details] [diff] [review]
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.
Attachment #661345 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
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 ?
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.
(Assignee)

Updated

5 years ago
Blocks: 746978
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?
(Assignee)

Comment 24

5 years ago
(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.
Can attackers disable CSP by injecting "Content-Security-Policy:" if the site still uses old headers?
(Assignee)

Comment 26

5 years ago
(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.
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.
(Assignee)

Comment 28

5 years ago
(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.
(Assignee)

Comment 29

5 years ago
Created attachment 689025 [details] [diff] [review]
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
Attachment #661347 - Attachment is obsolete: true
Attachment #689025 - Flags: review?(bzbarsky)
(Assignee)

Comment 30

5 years ago
Created attachment 689028 [details] [diff] [review]
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 ?
Attachment #676811 - Attachment is obsolete: true
Flags: needinfo?
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.
Attachment #689025 - Flags: review?(bzbarsky) → review+
Flags: needinfo?
(Assignee)

Comment 32

4 years ago
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.
(Assignee)

Comment 33

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

oops !
Attachment #689028 - Attachment description: Part 1 v4 - C++ changes to read both CSP headers and call the right parser → Part 2 v4 - C++ changes to read both CSP headers and call the right parser
(Assignee)

Comment 34

4 years ago
Created attachment 691610 [details] [diff] [review]
Part 3 v3 - tests

I added a mochitest that makes sure that the new header is used when both headers are sent.
Attachment #661356 - Attachment is obsolete: true
(Assignee)

Comment 35

4 years ago
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.
Attachment #689028 - Flags: review?(bzbarsky)
(Assignee)

Comment 36

4 years ago
Created attachment 692448 [details] [diff] [review]
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 !
Attachment #689025 - Attachment is obsolete: true
Attachment #692448 - Flags: review+
(Assignee)

Comment 37

4 years ago
(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
Blocks: 821877
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.
Attachment #689028 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 39

4 years ago
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 :)
Attachment #691610 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
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?
(Assignee)

Comment 41

4 years ago
Created attachment 693034 [details] [diff] [review]
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+.
Attachment #689028 - Attachment is obsolete: true
Attachment #693034 - Flags: review+
(Assignee)

Comment 42

4 years ago
Created attachment 693083 [details] [diff] [review]
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.
Attachment #691610 - Attachment is obsolete: true
Attachment #691610 - Flags: review?(bzbarsky)
Attachment #693083 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #693083 - Attachment is patch: true
Comment on attachment 693083 [details] [diff] [review]
Part 3v4 - tests

r=me assuming the checkin comment ends up being fixed up
Attachment #693083 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 44

4 years ago
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
(Assignee)

Comment 45

4 years ago
Created attachment 695512 [details] [diff] [review]
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
Attachment #693034 - Attachment is obsolete: true
Attachment #695512 - Flags: review+
(Assignee)

Comment 46

4 years ago
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
(Assignee)

Comment 47

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/1569edaba5fc
https://hg.mozilla.org/mozilla-central/rev/d4d85b53c015
https://hg.mozilla.org/mozilla-central/rev/cb6907c8de98
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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
(Assignee)

Comment 50

4 years ago
(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.
(Assignee)

Updated

4 years ago
Depends on: 829699
(Assignee)

Updated

4 years ago
Depends on: 831372
relnote-firefox: --- → ?
(Assignee)

Comment 51

4 years ago
The pref to enable this won't be turned on in Fx21 so I suspect this doesn't need a relnote yet ?
relnote-firefox: ? → ---
(Assignee)

Updated

4 years ago
Blocks: 842657
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.