Closed
Bug 783049
Opened 12 years ago
Closed 12 years ago
CSP : use existing/old parser for X-Content-Security-Policy header, new/CSP 1.0 spec compliant parser for Content-Security-Policy header
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: imelven, Assigned: imelven)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 11 obsolete files)
(deleted),
patch
|
imelven
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
imelven
:
review+
|
Details | Diff | Splinter Review |
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•12 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: csp-w3c-1.0
Assignee | ||
Updated•12 years ago
|
Comment 2•12 years ago
|
||
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•12 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.
Comment 4•12 years ago
|
||
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•12 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•12 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•12 years ago
|
Assignee: nobody → imelven
Assignee | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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.
Comment 11•12 years ago
|
||
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 13•12 years ago
|
||
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 | ||
Comment 15•12 years ago
|
||
The version I used to write patches for bug 746978
Attachment #656656 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 years ago
|
||
Version I used to write patches for bug 746978
Attachment #656658 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
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 | ||
Comment 18•12 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 ?
Comment 19•12 years ago
|
||
Both if possible, but for sure they should be deprecation warnings in the web console.
Assignee | ||
Comment 20•12 years ago
|
||
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•12 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 ?
Comment 22•12 years ago
|
||
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•12 years ago
|
||
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•12 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.
Comment 25•12 years ago
|
||
Can attackers disable CSP by injecting "Content-Security-Policy:" if the site still uses old headers?
Assignee | ||
Comment 26•12 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.
Comment 27•12 years ago
|
||
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•12 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•12 years ago
|
||
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•12 years ago
|
||
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 31•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
(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•12 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 38•12 years ago
|
||
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•12 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•12 years ago
|
Status: NEW → ASSIGNED
![]() |
||
Comment 40•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
Attachment #693083 -
Attachment is patch: true
![]() |
||
Comment 43•12 years ago
|
||
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•12 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 44•12 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•12 years ago
|
||
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•12 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•12 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
Comment 48•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment 49•12 years ago
|
||
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•12 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.
Updated•12 years ago
|
relnote-firefox:
--- → ?
Assignee | ||
Comment 51•12 years ago
|
||
The pref to enable this won't be turned on in Fx21 so I suspect this doesn't need a relnote yet ?
Updated•12 years ago
|
relnote-firefox:
? → ---
Updated•8 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•