Last Comment Bug 663570 - Implement Content Security Policy via <meta> tag
: Implement Content Security Policy via <meta> tag
Status: RESOLVED FIXED
[CSP 1.1]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Security (show other bugs)
: Trunk
: All All
: P1 normal with 11 votes (vote)
: mozilla45
Assigned To: Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
:
: Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
Mentors:
https://w3c.github.io/webappsec/specs...
: 1218773 (view as bug list)
Depends on: 1048048 1185019 1226437 1226444 1233098 1247807 1261634
Blocks: CSP 923902 csp-w3c-2 1102469 1168540 1185719 1017257 1184781 1224694 1225383
  Show dependency treegraph
 
Reported: 2011-06-10 17:02 PDT by Brandon Sterne (:bsterne)
Modified: 2016-11-24 04:28 PST (History)
35 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
45+


Attachments
patch to add meta tag support to csp (4.88 KB, patch)
2012-06-25 18:07 PDT, Devdatta Akhawe [:devd]
mozbugs: review-
Details | Diff | Splinter Review
patch to add meta tag support to csp (4.88 KB, patch)
2012-06-26 15:27 PDT, Devdatta Akhawe [:devd]
no flags Details | Diff | Splinter Review
Add meta tag support to CSP (4.97 KB, patch)
2012-06-27 14:48 PDT, Devdatta Akhawe [:devd]
jst: feedback+
Details | Diff | Splinter Review
csp_meta_element_csp_changes.patch (16.59 KB, patch)
2015-07-27 13:40 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
no flags Details | Diff | Splinter Review
csp_meta_element_dom_changes.patch (11.46 KB, patch)
2015-07-27 13:41 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
jst: review+
Details | Diff | Splinter Review
csp_meta_element_html_parser.patch (13.83 KB, patch)
2015-07-27 13:44 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
wchen: review-
Details | Diff | Splinter Review
csp_meta_element_test1.patch (5.93 KB, patch)
2015-07-27 13:44 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
no flags Details | Diff | Splinter Review
csp_meta_element_test2.patch (9.17 KB, patch)
2015-07-27 13:45 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
no flags Details | Diff | Splinter Review
csp_meta_element_test3.patch (8.96 KB, patch)
2015-07-27 13:45 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
no flags Details | Diff | Splinter Review
csp_meta_element_test4.patch (5.98 KB, patch)
2015-07-27 13:46 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
no flags Details | Diff | Splinter Review
csp_meta_part_1_csp_changes.patch (15.74 KB, patch)
2015-10-29 14:34 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
no flags Details | Diff | Splinter Review
csp_meta_part_2_dom_changes.patch (10.89 KB, patch)
2015-10-29 14:34 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
bzbarsky: review+
Details | Diff | Splinter Review
csp_meta_part_3_invalidate_preloads.patch (7.72 KB, patch)
2015-10-29 14:34 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
bzbarsky: review-
Details | Diff | Splinter Review
csp_meta_test_1.patch (5.88 KB, patch)
2015-10-29 14:35 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
no flags Details | Diff | Splinter Review
csp_meta_test_2.patch (9.26 KB, patch)
2015-10-29 14:35 PDT, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
no flags Details | Diff | Splinter Review
metacsp_part1_csp_parser_changes.patch (19.51 KB, patch)
2015-11-04 13:06 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
jonas: review-
Details | Diff | Splinter Review
metacsp_part2_principal_changes.patch (7.04 KB, patch)
2015-11-04 13:06 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
bzbarsky: review+
Details | Diff | Splinter Review
metacsp_part3_upgrade_insecure_requests_changes.patch (20.86 KB, patch)
2015-11-04 13:07 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
bzbarsky: review+
Details | Diff | Splinter Review
metacsp_part4_speculative_parser_changes.patch (8.29 KB, patch)
2015-11-04 13:08 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
bzbarsky: review+
Details | Diff | Splinter Review
metacsp_part5_htmlmetaelement_changes.patch (3.52 KB, patch)
2015-11-04 13:08 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
bzbarsky: review+
Details | Diff | Splinter Review
metacsp_part6_csp_preloads_changes.patch (11.41 KB, patch)
2015-11-04 13:08 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
jonas: review+
Details | Diff | Splinter Review
metacsp_part7_preload_validation.patch (1.35 KB, patch)
2015-11-04 13:09 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
bzbarsky: review+
Details | Diff | Splinter Review
metacsp_test1.patch (5.74 KB, patch)
2015-11-04 13:10 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
jonas: review+
Details | Diff | Splinter Review
metacsp_test2.patch (9.12 KB, patch)
2015-11-04 13:10 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
jonas: review+
Details | Diff | Splinter Review
metacsp_test3.patch (8.94 KB, patch)
2015-11-04 13:10 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
jonas: review+
Details | Diff | Splinter Review
metacsp_test4.patch (6.00 KB, patch)
2015-11-04 13:11 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
jonas: review+
Details | Diff | Splinter Review
metacsp_test5.patch (7.78 KB, patch)
2015-11-04 13:11 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
jonas: review+
Details | Diff | Splinter Review
metacsp_test6.patch (46.72 KB, patch)
2015-11-04 13:12 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
jonas: review+
Details | Diff | Splinter Review
metacsp_part1_csp_parser_changes.patch (16.36 KB, patch)
2015-11-12 10:45 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
jonas: review+
Details | Diff | Splinter Review
metacsp_part1_csp_parser_changes.patch (16.36 KB, patch)
2015-11-14 19:35 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_part2_principal_changes.patch (7.35 KB, patch)
2015-11-14 19:35 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_part3_upgrade_insecure_requests_changes.patch (20.79 KB, patch)
2015-11-14 19:35 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_part4_speculative_parser_changes.patch (8.42 KB, patch)
2015-11-14 19:36 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_part5_htmlmetaelement_changes.patch (3.51 KB, patch)
2015-11-14 19:36 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_part6_csp_preloads_changes.patch (13.29 KB, patch)
2015-11-14 19:37 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_part7_preload_validation.patch (1.35 KB, patch)
2015-11-14 19:37 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_test1.patch (5.58 KB, patch)
2015-11-14 19:37 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_test2.patch (9.03 KB, patch)
2015-11-14 19:38 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_test3.patch (8.98 KB, patch)
2015-11-14 19:38 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_test4.patch (6.00 KB, patch)
2015-11-14 19:38 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_test5.patch (7.70 KB, patch)
2015-11-14 19:39 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review
metacsp_test6.patch (44.93 KB, patch)
2015-11-14 19:39 PST, Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)]
ckerschb: review+
Details | Diff | Splinter Review

Description Brandon Sterne (:bsterne) 2011-06-10 17:02:57 PDT
CSP standard says policies can be declared via <meta http-equiv="X-Content-Security-Policy"> element.
Comment 1 Ian Melven :imelven 2012-03-14 11:11:52 PDT
There are some concerns about embedding the policy to protect the content actually IN the content it's supposed to be protecting.

dchan raised the issue of sites potentially taking user content and including it inside a meta tag.

it was also discussed making the policy the 'first thing' in the <meta> tag could mitigate some of these issues.
Comment 2 Ian Melven :imelven 2012-03-14 11:37:38 PDT
Apparently some sites are already serving CSP via the meta tag (Chrome has implemented support for this already, AIUI)
Comment 3 Adam Barth 2012-03-14 18:07:42 PDT
In trying to convince the folks who run some of Google's large, complex web apps to use CSP, they are often attracted to the <meta> tag approach because they can turn on CSP after they're done "booting up" the app.
Comment 4 Sid Stamm [:geekboy or :sstamm] 2012-03-15 10:10:03 PDT
(In reply to Adam Barth from comment #3)
> they are often attracted to the <meta> tag approach because
> they can turn on CSP after they're done "booting up" the app.

This is one of the things I don't like about putting a policy in a meta tag; it intermixes the policy with the code it's restricting.  One of the key benefits I saw with header-only CSP was that we use the protocol/app separation to separate the policy language from the code it protects, making it far less likely to be buggy in implementations and more future-proof as we add new stuff to HTML.

I'm not *completely* opposed to implementing meta tag functionality, it just really worries me that it may introduce more points of failure in user agent implementations.
Comment 5 Adam Barth 2012-03-15 14:03:38 PDT
My plan for these folks, by the way, is to get them to ship with the <meta> tag at the end of their boot process and then work with them to continue to move the policy declaration earlier and earlier in the boot process until they can supply the policy in a header.

The ability to control when the policy engages lets these folks implement support for CSP iteratively rather than requiring a complete conversion of their web app before they gain any benefit.
Comment 6 Sid Stamm [:geekboy or :sstamm] 2012-03-15 14:08:12 PDT
Can we provide a technological incentive for folks to move from using <meta> to a header?  For example, would it make sense to omit support for the XSS-related directives from the policies supported in the <meta> tag to encourage adoption of the header?  Otherwise, I worry that if there's no functional incentive, they'll implement <meta> tag policies and then stop.
Comment 7 Adam Barth 2012-03-15 23:16:39 PDT
Maybe the report-uri directive should be restricted to the header?
Comment 8 Daniel Veditz [:dveditz] 2012-04-28 00:24:59 PDT
If we don't ignore report-uri in a <meta> policy altogether we should at the very least reinstate the same-origin restriction.
Comment 9 Devdatta Akhawe [:devd] 2012-06-25 18:07:38 PDT
Created attachment 636564 [details] [diff] [review]
patch to add meta tag support to csp
Comment 10 Sid Stamm [:geekboy or :sstamm] 2012-06-26 15:17:18 PDT
Comment on attachment 636564 [details] [diff] [review]
patch to add meta tag support to csp

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

I don't see how this works... is this a partial implementation?  I don't see anywhere this reads the policy from the meta tag.  Please also write some tests to go with this patch.
Comment 11 Devdatta Akhawe [:devd] 2012-06-26 15:27:13 PDT
Created attachment 636903 [details] [diff] [review]
patch to add meta tag support to csp

TIL about qref. @geekboy: sorry about that. It is missing 4 lines.

I will add tests in a separate patch
Comment 12 Sid Stamm [:geekboy or :sstamm] 2012-06-27 14:04:36 PDT
Comment on attachment 636903 [details] [diff] [review]
patch to add meta tag support to csp

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

Looking forward to the tests for this patch ... especially the ones that test what to do when there's both a HTTP header and a http-equiv meta tag.

::: content/base/public/nsIDocument.h
@@ +593,5 @@
>    /**
> +   * Initialize CSP policy based on the CSP HTTP header. This is used by
> +   * nsContentSink to initialize CSP policy when it sees one in a meta tag.
> +   */
> +  virtual nsresult InitCSP() = 0;

I'm not sure I like putting this in the .h file but not in the .idl... and I'm not sure we should put it in the IDL either.  We should get someone from content to take a look at this and give thoughts about the right way to get the content sink to init CSP on the document.

::: content/base/src/nsDocument.cpp
@@ +2409,5 @@
>  
> +    nsCOMPtr<nsIContentSecurityPolicy> csp;
> +    nsIPrincipal* principal = GetPrincipal();
> +    if (principal) {
> +      //if CSP already exists, then don't do anything

If this is the correct behavior (ignore meta when there's a real HTTP header), please explain why in the comment (something about if InitCSP is called twice, the second time is probably from meta tag instead of header, and the spec requires ignoring the meta tag when there's a header).
Comment 13 Devdatta Akhawe [:devd] 2012-06-27 14:48:56 PDT
Created attachment 637270 [details] [diff] [review]
Add meta tag support to CSP

thanks, I have modified the comment.

Any ideas on who is the right person to contact re the right way to get the contentsink to call initcsp ?
Comment 14 Sid Stamm [:geekboy or :sstamm] 2012-06-27 14:50:10 PDT
Comment on attachment 637270 [details] [diff] [review]
Add meta tag support to CSP

jst: can you look at the patch and see if this approach is reasonable (particularly the exposure of InitCSP() via C++)?
Comment 15 Devdatta Akhawe [:devd] 2012-06-27 14:56:41 PDT
Also, the current patch explicitly does not try to support the sandbox directive via the meta tag. That should be a separate bug, I think.
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-16 19:00:20 PDT
Comment on attachment 637270 [details] [diff] [review]
Add meta tag support to CSP

Seems very reasonable to me.
Comment 17 Devdatta Akhawe [:devd] 2012-09-01 08:07:25 PDT
Is there someone we can contact from the JS team to add support for a call to revoke the "eval" allowed status? AFAIK, the status of unsafe-inline is cached by the JIT, meaning restrictions on inline scripts won't be enforced by this patch.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-09-04 21:24:45 PDT
Mail dmandelin and ask?
Comment 19 Frederik Braun [:freddyb] 2013-10-06 01:17:28 PDT
What's the status on this? This is blocking getting CSP to protect chrome pages.
Comment 20 Devdatta Akhawe [:devd] 2013-10-06 08:14:59 PDT
Unfortunately, I don't have the cycles to push this through.

IIRC, the patch should do what you want save for blocking eval,which is a much deeper change. This is still a win, and so you can consider creating a separate bug for eval and talk to the JS team about it. This needs tests and a review.
Comment 21 Sid Stamm [:geekboy or :sstamm] 2014-04-28 16:58:17 PDT
Comment on attachment 637270 [details] [diff] [review]
Add meta tag support to CSP

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

So I've been sitting on this for quite a while since we've had a C++ rewrite of CSP in the works, but it looks like this can probably be done independently of that.  Sorry for the lag.  The approach looks reasonable (I agree with jst), but does need tests for documents with the meta tag and with/without a header, for content loads and for eval/inline scripts.  We should also write a test that links in stylesheets and scripts and stuff in the <head> of the document just to verify the timing of the meta tag being processed.

Also need to support changing of eval at runtime (see bug 614137).
Comment 22 Sid Stamm [:geekboy or :sstamm] 2014-05-30 14:59:24 PDT
Another todo: ignore frame-ancestors in policies specified via meta tag.  The CSP 1.1 draft says to do this.
Comment 23 James Burke [:jrburke] 2014-11-27 13:13:40 PST
I believe this would help improve HTML email viewing in Gaia's email client, more details in bug 1102469 comment 1.
Comment 24 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-27 13:40:26 PDT
Created attachment 8639485 [details] [diff] [review]
csp_meta_element_csp_changes.patch
Comment 25 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-27 13:41:19 PDT
Created attachment 8639486 [details] [diff] [review]
csp_meta_element_dom_changes.patch

Johnny, you reviewed basically all of the CSP changed within DOM. Please feel free to delegate to other peers though.
Comment 26 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-27 13:44:09 PDT
Created attachment 8639489 [details] [diff] [review]
csp_meta_element_html_parser.patch

William, it seems that Henri is not accepting needinfo requests at the moment. Since you already provided so much guidance and help, are you willing to review the html5 parser bits?
Comment 27 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-27 13:44:41 PDT
Created attachment 8639490 [details] [diff] [review]
csp_meta_element_test1.patch
Comment 28 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-27 13:45:00 PDT
Created attachment 8639491 [details] [diff] [review]
csp_meta_element_test2.patch
Comment 29 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-27 13:45:21 PDT
Created attachment 8639492 [details] [diff] [review]
csp_meta_element_test3.patch
Comment 30 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-27 13:46:31 PDT
Created attachment 8639493 [details] [diff] [review]
csp_meta_element_test4.patch

Dan, since there are CSP bits in all of the patches it would be great if you could take a look at all of them (code and tests). I am happy to fill you in on all the needed tests here. Let's chat in person afterwards.
Comment 31 William Chen [:wchen] 2015-07-28 00:22:39 PDT
Comment on attachment 8639489 [details] [diff] [review]
csp_meta_element_html_parser.patch

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

::: parser/html/nsHtml5TreeBuilderCppSupplement.h
@@ +237,5 @@
> +              // (2) communicate that the meta CSP was alreaded appended during speculative
> +              //     parsing so that htmlMetaElement::bindToTree does not append the same
> +              //     CSP policy again.
> +              // Please note that the htmlMetaElement is not fully constructed when
> +              // InitMetaCSP(*csp) is executed, hence we do that within InitMetaCSP(content).

I don't think this works because the speculative load queue is processed at different times than the tree op queue. By my reading of the code, speculative loads may be processed at a point where the speculation may fail which is different than tree ops that gets executed at points where the speculation has succeeded.

This means that speculative load items shouldn't be setting the document CSP state. We may need to do something similar to the referrer policy and create policies per speculative load if there are relevant meta tags in the queue, although it's a bit more complicated because we'll need policies that append onto the most recent document policy rather than overriding it.
Comment 32 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-28 09:44:05 PDT
(In reply to William Chen [:wchen] from comment #31)
> Comment on attachment 8639489 [details] [diff] [review]
> ::: parser/html/nsHtml5TreeBuilderCppSupplement.h
> @@ +237,5 @@
> > +              // (2) communicate that the meta CSP was alreaded appended during speculative
> > +              //     parsing so that htmlMetaElement::bindToTree does not append the same
> > +              //     CSP policy again.
> > +              // Please note that the htmlMetaElement is not fully constructed when
> > +              // InitMetaCSP(*csp) is executed, hence we do that within InitMetaCSP(content).
> 
> I don't think this works because the speculative load queue is processed at
> different times than the tree op queue. By my reading of the code,
> speculative loads may be processed at a point where the speculation may fail
> which is different than tree ops that gets executed at points where the
> speculation has succeeded.

So, wait a second. I agree, the speculative load queue is processed at different times than then tree op queue, but always in the following order, no?

Here was my thinking:

(1) Append Meta CSP policy to the speculative load queue
[mSpeculativeLoadQueue.AppendElement()->InitMetaCSP(*csp);]
Parse and Append the CSP policy to the principal of the current document before any speculative loading  happens. This is is important because CSP might block specific resources from being loaded. Further of importance is, that the CSP spec suggests putting the Meta CSP element as the first element in the head. If the meta CSP is not the first element in the head, then all bets are off.

(2) Add flag on the HTMLMetaElement to indicate speculative parser processed the CSP
[mOpQueue.AppendElement()->InitMetaCSP(content);]
The reason this does not happen within the speculative load queue is, because the HTMLMetaElement is not fully constructed yet, hence we set that flag in the tree op queue.

(3) HTMLMetaElement::BindtoTree inspects flag set that was set in tree op queue.
[if (!mMetaCSPInitialized)]
HtmlMetaElement::BindToTree is called regardless whether the speculative parser already parsed and initalized the CSP on the document. *BUT* if the meta CSP was already appended within step (1) then the flag added in step (2) would be set to true and we know we don't have to init/parse the meta CSP string again, right?

Or am I missing something completely here? Happy to chat over vidyo if that makes things easier.

Thanks for your help William!
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2015-07-28 09:53:20 PDT
> If the meta CSP is not the first element in the head, then all bets are off.

In what sense?

I think William's worry is the following web page:

  <head>
    <script>document.write("<!--")</script>
    <meta csp-stuff-here>
  </head>
  <body>
    I am still a comment. --> But I am not.
    <img src="the-meta-csp-better-not-apply-here">
  </body>

In that testcase, per spec there should be no CSP applied to the image load when the actual image load happens, right?
Comment 34 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-28 10:10:31 PDT
(In reply to Boris Zbarsky [:bz] from comment #33)
> > If the meta CSP is not the first element in the head, then all bets are off.
> 
> In what sense?
> 
> I think William's worry is the following web page:
> 
>   <head>
>     <script>document.write("<!--")</script>
>     <meta csp-stuff-here>
>   </head>
>   <body>
>     I am still a comment. --> But I am not.
>     <img src="the-meta-csp-better-not-apply-here">
>   </body>
> 
> In that testcase, per spec there should be no CSP applied to the image load
> when the actual image load happens, right?

That is absolutely correct, thanks for clarification. That makes things a little harder in the end.
Comment 35 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-28 21:33:50 PDT
Boris, I had a nice discussion of how we could handle meta csp with William this afternoon. Before we move forward, I would like to discuss our approach with you. One thing that is worth mentioning up front is that CSP delivered via the header and CSP via the meta tag should be merged together into the page's CSP.

Our approach would require the following changes:

(1) During speculation phase of the html parser we would parse and generate and object representation of the CSP. Instead of calling ::SetCSP() on the principal we would extend the principal to also hold a speculative CSP. So we could call ::SetSpeculationCSP() on the principal and hence keep the actual CSP separated from the speculative CSP. One reason for keeping it separate is, that it might be hard to communicate what policy is speculative and might end up being removed in the end (basically the case of Comment 33).

(2) We would have to add new content policy types for preloads to [a] for at least the following cases: TYPE_IMAGE_PRELOAD, TYPE_SCRIPT_PRELOAD, TYPE_STYLESHEET_PRELOAD and use those whenever contentPolicies are consulted during the speculation phase. Being able to distinguish between preloads and actual loads when consulting CSP would also allow us to clean up that code [b] and further fix [c]. You already proposed adding different policy types within comment 6 of [c]. I think it's time to add those now.

(3) Whenever CSP is consulted for a preload, the CSP could not only apply the actual CSP but apply the speculative CSP in addition. In case it's not a preload, then only apply the actual CSP of the page.

(4) Within HTMlMetaElement::bindToTree() we would have to parse the CSP again, but this time appending the policy to the actual CSP of the principal and not the speculative one. From this point on, everything CSP related should just work the same way as it does now.


I agree, parsing the CSP twice (once speculatively and again within ::bindToTree) is not that crisp but it seems like the best tradeoff at the moment. We also parse the referrer policy twice, once during speculation phase and once when we actually set it on the document.

Would you be fine with that approach or are we missing something?


[a] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIContentPolicyBase.idl#70
[b] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#163
[c] https://bugzilla.mozilla.org/show_bug.cgi?id=1048048
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2015-07-29 09:59:03 PDT
That all sounds fine, as long as we also ensure that preload reuse only happens if the CSP at real load time matches the CSP at preload time.  You'll want to update our internal content policies and coordinate with addons that implement content policies, of course.  I don't think parsing the CSP twice is a huge deal; it's not _that_ expensive to parse, right?
Comment 37 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-29 10:06:49 PDT
(In reply to Boris Zbarsky [:bz] from comment #36)
> That all sounds fine, as long as we also ensure that preload reuse only
> happens if the CSP at real load time matches the CSP at preload time.

Indeed, but thanks for highlighting it again. I have to think about how to accomplish that in a nice way again.

> You'll want to update our internal content policies and coordinate with
> addons that implement content policies, of course.  I don't think parsing
> the CSP twice is a huge deal; it's not _that_ expensive to parse, right?

It's not that expensive to parse it twice. Usually CSP policies are not that long anyway. In addition we can hold off on logging CSP parsing-warnings/errors in the policy to the console for the speculative parsing. Thanks Boris!
Comment 38 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-07-29 10:08:15 PDT
Marking Bug 1048048 as blocking. We can add the new contentPolicy_types in that bug and than use them in this bug.
Comment 39 michael.oneill 2015-08-02 01:55:50 PDT
Is CSP meta tag support in Nightly? How can I test it?
Comment 40 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-08-02 10:34:05 PDT
(In reply to michael.oneill from comment #39)
> Is CSP meta tag support in Nightly? How can I test it?

It's not in Nightly yet; this bug is adding meta csp support to Firefox.
Comment 41 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-08-06 15:27:55 PDT
Comment on attachment 8639485 [details] [diff] [review]
csp_meta_element_csp_changes.patch

This Bug is blocked by Bug 1048048 and will make all of these patches obsolete. Unflagging for now till Bug 1048048 got resolved.
Comment 42 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-26 13:57:13 PDT
Boris, I have been thinking about implementing meta CSP, and in particular Comment 33, for some time by now. Is there any chance I could convince you so we can drop support the document.write() case? E.g. drop meta CSP if it's used after the first occuring <script> tag and log a warning to the console or similar? Supporting that case for meta CSP does not make sense from a security perspective and makes the implementation overly complicated for a rare edge case. For example, if someone really uses document.write("<meta CSP ...>") and an image preload occurs in the mean time, than an exfiltration attack would have already happenend for the preload anyway. No need into applying the CSP after that image load.

If we do need to support document.write(), then I would suggest the algorithm described in Comment 35 and to address Comment 36 I would suggest the following:
1) Create a hash of all the CSP policy strings, something like:
   cspPreloadHash = (H(sort(H(CSP1), H(CSP2, H(CSPN)))))
   and store that cspPreloadHash within the loadInfo of the channel. Storing the hash in the loadInfo allows easy propagation for e10s.
2) Before we can reuse the resource we would check if the cspPreloadHash matches the actual CSP hash applied to the document. I assume that ::bindToTree for all the metaCSP is called before would check if we can resue a preloaded resource.

I am happy to hop on vidyo for any kind of discussion. Thanks!
Comment 43 Boris Zbarsky [:bz] (still a bit busy) 2015-10-26 19:41:13 PDT
> Is there any chance I could convince you so we can drop support the document.write() case?

That's fine as far as I'm concerned, as long as you get the spec changed accordingly and get other browsers to agree.
Comment 44 :Ehsan Akhgari 2015-10-27 06:45:59 PDT
*** Bug 1218773 has been marked as a duplicate of this bug. ***
Comment 45 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-28 21:00:46 PDT
(In reply to Boris Zbarsky [:bz] from comment #43)
> > Is there any chance I could convince you so we can drop support the document.write() case?
> 
> That's fine as far as I'm concerned, as long as you get the spec changed
> accordingly and get other browsers to agree.

I filed an issue against the CSP spec:
https://github.com/w3c/webappsec-csp/issues/27
Comment 46 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 08:31:29 PDT
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #45)
> https://github.com/w3c/webappsec-csp/issues/27

There is some discussion going on within the github issue and I understand the need to support dynamically created meta CSPs (somehow). I propose that within this bug we are going to ignore Meta CSP if it appears after any <script> tag so we get basic meta CSP support out the door soonish. I will file a follow up to make Meta CSP work for speculative loads which obviously needs more discussion and seems like a lot of engineering work.
Comment 47 Boris Zbarsky [:bz] (still a bit busy) 2015-10-29 08:46:53 PDT
I commented on the github issues, but as far as shipping something that only works in some cases... is it better to do that (and have people maybe output non-working CSP stuff that they think is working) or to just tell them that it doesn't work yet and they should most definitely use a header?

Also, the only win from ignoring a meta CSP after <script> compared to treating it like normal is that it consistently makes it not be enforced no matter what our preload behavior, right?  I'm not sure that's better from a security perspective than at least enforcing it sometimes....
Comment 48 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 09:14:36 PDT
(In reply to Boris Zbarsky [:bz] from comment #47)
> I commented on the github issues, but as far as shipping something that only
> works in some cases... is it better to do that (and have people maybe output
> non-working CSP stuff that they think is working) or to just tell them that
> it doesn't work yet and they should most definitely use a header?
> 
> Also, the only win from ignoring a meta CSP after <script> compared to
> treating it like normal is that it consistently makes it not be enforced no
> matter what our preload behavior, right?  I'm not sure that's better from a
> security perspective than at least enforcing it sometimes....

To sum it up, we could re-use the patches I already put up a while ago, where we speculatively parse the CSP which is then enforced for the document even if we would figure it got commented out by doc.write() at a later point. On the upside, that would allow a script to actually perform a doc.write(<meta csp... >) which is then actually enforced for non preloaded resources. So, it's some kind of limbo state we are in, where it's really not transparent for a user what gets blocked and what not.

But the spec mentions that meta CSP is best effort and if used correctly (hopefully that is the case most of the time), where meta csp is the first element in the head, then everything would work just fine.

Can we agree on that?
Comment 49 Boris Zbarsky [:bz] (still a bit busy) 2015-10-29 09:31:26 PDT
I guess my question is what you mean by "ignoring a meta CSP after <script>".  Do you mean just in the preload scanner, or in general?

I think what we could do for now is just ignore <meta csp> in preload scanner completely, but make sure we apply the policy before actually running scripts or adding stylesheets to the page or whatever.  That would mean that we have stray fetches but nothing worse, right?
Comment 50 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 09:40:52 PDT
(In reply to Boris Zbarsky [:bz] from comment #49)
> I guess my question is what you mean by "ignoring a meta CSP after
> <script>".  Do you mean just in the preload scanner, or in general?
>
> I think what we could do for now is just ignore <meta csp> in preload
> scanner completely, but make sure we apply the policy before actually
> running scripts or adding stylesheets to the page or whatever.  That would
> mean that we have stray fetches but nothing worse, right?

I thought we should ignore meta csp in general if it appears after a <script> tag. If we only ignore it for the preload scanner then we still have to tag all preloaded resources so we don't reuse the preloaded result without consulting CSP first, right? I think there is no great solution, but after all your suggestion of ignoring meta CSP for the preload scanner alltogether sounds like the best option.
Comment 51 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 11:08:14 PDT
The only thing I really don't like is that if a meta-csp would use upgrade-insecure requests directive, then the preload wouldn't know about it, which is really unfortunate.
Comment 52 Boris Zbarsky [:bz] (still a bit busy) 2015-10-29 11:37:58 PDT
Hmm.  Would we end up using or not using the preload in that situation?
Comment 53 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 11:49:35 PDT
(In reply to Boris Zbarsky [:bz] from comment #52)
> Hmm.  Would we end up using or not using the preload in that situation?

Well, I think it's not about that. Isn't the promise made by upgrade insecure requests that all requests that hit the network are upgraded from http to https? Now we end up having:
* GET http://example.com/foo.jpg (preload)
* GET https://example.com.foo.jpg (actual load)
Comment 54 Boris Zbarsky [:bz] (still a bit busy) 2015-10-29 12:56:28 PDT
> Isn't the promise made by upgrade insecure requests that all requests that hit the
> network are upgraded from http to https?

I don't know.  Is the idea to avoid leaking info or to avoid using stuff you got unsafely?
Comment 55 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 13:04:08 PDT
(In reply to Boris Zbarsky [:bz] from comment #54)
> > Isn't the promise made by upgrade insecure requests that all requests that hit the
> > network are upgraded from http to https?
> 
> I don't know.  Is the idea to avoid leaking info or to avoid using stuff you
> got unsafely?

Generally, I was concerned about leaking information to the network with the preload. Since we can add checks to never re-use the preload if meta CSP is involved we can avoid the problem of using resources that we got insecurely.

Actually, the more I think about it it's not that big of a problem that we might leak info to the network, since mixed content blocking would kick in and would stop the preload (at least for mixed active content). I assume there is always a tradeoff.
Comment 56 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 14:34:10 PDT
Created attachment 8680882 [details] [diff] [review]
csp_meta_part_1_csp_changes.patch

Here is how we roll for meta CSP. As agreed, let's ignore meta CSP for speculative loads and then invalidate the request before we are going to reuse it.
Comment 57 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 14:34:34 PDT
Created attachment 8680883 [details] [diff] [review]
csp_meta_part_2_dom_changes.patch
Comment 58 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 14:34:54 PDT
Created attachment 8680885 [details] [diff] [review]
csp_meta_part_3_invalidate_preloads.patch
Comment 59 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 14:35:15 PDT
Created attachment 8680886 [details] [diff] [review]
csp_meta_test_1.patch
Comment 60 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-29 14:35:36 PDT
Created attachment 8680887 [details] [diff] [review]
csp_meta_test_2.patch
Comment 61 Boris Zbarsky [:bz] (still a bit busy) 2015-10-30 18:30:50 PDT
Comment on attachment 8680883 [details] [diff] [review]
csp_meta_part_2_dom_changes.patch

This needs to land in the same changeset as part 1 to not end up with broken bisections, right?

I don't understand the comment and code change in nsDocument::InitCSP.  That code is called before any <meta> tags have been seen, no?  How would there be an existing csp on the principal?

>+   * Meta CSP has no direct access to the document,

I'm not sure what "direct access" means in this case.  You're just saying these are protected/private members and we need a setter for them?  Why is this not an issue for non-meta CSP?  Because there the document pulls information from it in InitCSP?  Would it make more sense to have an "ApplyCSP" method that gets called from both InitCSP and HTMLMetaElement::BindToTree and avoid both the extra setters and the code duplication??

r=me with those fixed up.

The o
Comment 62 Boris Zbarsky [:bz] (still a bit busy) 2015-10-30 18:35:29 PDT
Comment on attachment 8680885 [details] [diff] [review]
csp_meta_part_3_invalidate_preloads.patch

So this seems like the worst of both worlds: a <meta> CSP means not only that we don't use the preloads but that we waste the time to get them anyway and _then_ don't use them?

That seem like a pretty steep hit for pages that use <meta> CSP.  I would much rather we just skipped the preload altogether instead (which requires the parser letting us know when there is a <meta> CSP... Though not sure what happens with a page using document.write to output a <meta> CSP then.
Comment 63 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-30 18:47:34 PDT
(In reply to Boris Zbarsky [:bz] from comment #62)
> Comment on attachment 8680885 [details] [diff] [review]
> csp_meta_part_3_invalidate_preloads.patch
> 
> So this seems like the worst of both worlds: a <meta> CSP means not only
> that we don't use the preloads but that we waste the time to get them anyway
> and _then_ don't use them?
> 
> That seem like a pretty steep hit for pages that use <meta> CSP.  I would
> much rather we just skipped the preload altogether instead (which requires
> the parser letting us know when there is a <meta> CSP... Though not sure
> what happens with a page using document.write to output a <meta> CSP then.

It seems there is some tradeoff anyway. What if we parse the Meta CSP in the preload parser and apply it to the document. We could store some flag on the HTMLMetaElement whether it was already parsed so we know we shouldn't parse it again within ::bindElementToTree, which would allow us to reuse preloads because the CSP would already have been applied. Aren't content policies called before reuse again anyway? Eg. for scripts see here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#565

For doc.write() we could apply the CSP to the document within bindToTree and from that point on that CSP would also be used. There is some tradeoff we have to make somewhere, but this sounds like the best option we have to me. What do you think?
Comment 64 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-30 19:00:27 PDT
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #63)
> (In reply to Boris Zbarsky [:bz] from comment #62)
> > Comment on attachment 8680885 [details] [diff] [review]
> > csp_meta_part_3_invalidate_preloads.patch
> > 
> > So this seems like the worst of both worlds: a <meta> CSP means not only
> > that we don't use the preloads but that we waste the time to get them anyway
> > and _then_ don't use them?
> > 
> > That seem like a pretty steep hit for pages that use <meta> CSP.  I would
> > much rather we just skipped the preload altogether instead (which requires
> > the parser letting us know when there is a <meta> CSP... Though not sure
> > what happens with a page using document.write to output a <meta> CSP then.
> 
> It seems there is some tradeoff anyway. What if we parse the Meta CSP in the
> preload parser and apply it to the document. We could store some flag on the
> HTMLMetaElement whether it was already parsed so we know we shouldn't parse
> it again within ::bindElementToTree, which would allow us to reuse preloads
> because the CSP would already have been applied. Aren't content policies
> called before reuse again anyway? Eg. for scripts see here:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#565
> 
> For doc.write() we could apply the CSP to the document within bindToTree and
> from that point on that CSP would also be used. There is some tradeoff we
> have to make somewhere, but this sounds like the best option we have to me.
> What do you think?

BUT, that doesn't account for the doc.write() case which comments out a CSP :-(
Comment 65 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-10-31 07:45:16 PDT
Actually, the more I think about, we could do the following which covers all cases:

1) Within the speculative loading phase we apply a speculativeCSP to the principal.
2) We use the speculative CSP from 1) and any CSPs delivered via the header for any preloads
3) Within ::bindToTree we parse meta CSP again and append it to the actual CSP.
4) Whenever we are about to reuse a preloaded resource we perform an additional CSP check (not a full blown content security check, but a CSP check using the 'actual' CSP on the principal.
5) That approach also covers doc.write(meta CSP) because we would see it in ::bindToTree() and also doc.write(<!-- comment out meta CSP -->) because it wouldn't appear in ::bindToTree.
6) I will provide testcases for commenting out a meta CSP and also writing a CSP into the document using doc.write()
7) One more thing, we could also use a member mSpeculativeUpgradeInsecureRequests set on the document which would allow us to handle upgrade-insecure-requests correctly.

Boris, any objections to that approach?
Comment 66 Boris Zbarsky [:bz] (still a bit busy) 2015-11-02 09:49:37 PST
> Actually, the more I think about, we could do the following which covers all cases:

Yes, I think it does.  I assume the additional CSP check won't be too slow, right?
Comment 67 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:06:29 PST
Created attachment 8683303 [details] [diff] [review]
metacsp_part1_csp_parser_changes.patch

[See comment 65 for the overall approach of implementing meta CSP.]
Please note that the sandbox directive (which should be ignored by meta CSP) has not landed yet. Note that test1 is going to fail once sandbox is implemented but not ignored for meta csp.
Comment 68 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:06:59 PST
Created attachment 8683304 [details] [diff] [review]
metacsp_part2_principal_changes.patch

[See comment 65 for the overall approach of implementing meta CSP.]
Comment 69 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:07:33 PST
Created attachment 8683305 [details] [diff] [review]
metacsp_part3_upgrade_insecure_requests_changes.patch

[See comment 65 for the overall approach of implementing meta CSP.]

Boris, I didn’t ignore your suggestion within comment 61. Given that nsDocument::InitCSP() uses quite so much special logic which does not apply to meta csp I would like to defer the effort of creating a method ‘ApplyCSP()’ to a follow up bug. However, I created a method ApplySettingsFromCSP() which allows to apply settings for referrer and upgrade-insecure-reuqests(preloads).
Comment 70 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:08:03 PST
Created attachment 8683306 [details] [diff] [review]
metacsp_part4_speculative_parser_changes.patch

[See comment 65 for the overall approach of implementing meta CSP.]
Comment 71 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:08:29 PST
Created attachment 8683307 [details] [diff] [review]
metacsp_part5_htmlmetaelement_changes.patch

[See comment 65 for the overall approach of implementing meta CSP.]
Comment 72 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:08:58 PST
Created attachment 8683308 [details] [diff] [review]
metacsp_part6_csp_preloads_changes.patch

[See comment 65 for the overall approach of implementing meta CSP.]
Comment 73 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:09:25 PST
Created attachment 8683309 [details] [diff] [review]
metacsp_part7_preload_validation.patch

[See comment 65 for the overall approach of implementing meta CSP.]

Boris, I added test5 which performs doc.write(meta csp) as well as doc.write(comment out meta csp). I manually verified that we do not log errors to the console for preloads and also don’t send out any reports. If a preload fails because of any security checks, then we do not store that preload to be reusable (see for example [1], but please note that so far only the scriptloader uses asyncOpen2, we still have to convert the style loader and image loader to use asyncOpen2()). In all 3 cases (styles, images, and scripts) we perform another content security check before we actually embed a resource into the document. So I suppose the only thing we have to do is cancel the preload request for scripts. Otherwise that assertion [2] gets triggered. 

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1704
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1606
Comment 74 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:10:05 PST
Created attachment 8683310 [details] [diff] [review]
metacsp_test1.patch
Comment 75 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:10:30 PST
Created attachment 8683311 [details] [diff] [review]
metacsp_test2.patch
Comment 76 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:10:50 PST
Created attachment 8683312 [details] [diff] [review]
metacsp_test3.patch
Comment 77 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:11:16 PST
Created attachment 8683314 [details] [diff] [review]
metacsp_test4.patch
Comment 78 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:11:57 PST
Created attachment 8683315 [details] [diff] [review]
metacsp_test5.patch
Comment 79 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-04 13:12:28 PST
Created attachment 8683316 [details] [diff] [review]
metacsp_test6.patch
Comment 80 Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-11-11 14:55:41 PST
Comment on attachment 8683303 [details] [diff] [review]
metacsp_part1_csp_parser_changes.patch

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

::: dom/security/nsCSPUtils.cpp
@@ +958,3 @@
>    : mUpgradeInsecDir(nullptr)
>    , mReportOnly(false)
> +  , mDeliveredViaMetaTag(aDeliveredViaMetaTag)

Why store this information? Why not just drop the offending rules in the parser?
Comment 81 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-11 15:07:27 PST
(In reply to Jonas Sicking (:sicking) from comment #80)
> Why store this information? Why not just drop the offending rules in the
> parser?

It's because of the serialization we are doing. Look at nsCSPContext::Write() and then the later nsCSPContext::Read(). The ::Read() needs to know whether a policy was delivered via the meta tag because we are going to feed it to the parser again, hence we have to store it. We can discuss if we really need that, but for consistency I thought we rather keep that information around.
Comment 82 Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-11-11 16:56:16 PST
Comment on attachment 8683306 [details] [diff] [review]
metacsp_part4_speculative_parser_changes.patch

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

::: parser/html/nsHtml5TreeOpExecutor.cpp
@@ +1017,5 @@
>  }
>  
>  void
> +nsHtml5TreeOpExecutor::SetSpeculationCSP(const nsAString& aCSP)
> +{

Can you assert that this runs on the main thread.
Comment 83 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-12 10:43:47 PST
(In reply to Jonas Sicking (:sicking) from comment #82)
> >  void
> > +nsHtml5TreeOpExecutor::SetSpeculationCSP(const nsAString& aCSP)
> > +{
> 
> Can you assert that this runs on the main thread.

As discussed over IRC, I added that assertion and also renamed the function to ::AddSpeculationCSP(). All tests still pass and the assertion does not fire. I think there is no need to upload a new patch for that right now.
Comment 84 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-12 10:45:00 PST
Created attachment 8686713 [details] [diff] [review]
metacsp_part1_csp_parser_changes.patch

Jonas, as agreed on IRC, let's eliminate all the unwanted directives in the parser so we can eliminate the 'mDeliveredViaMetaTag' from the policy itself. Thanks for pointing that out - nice catch.
Comment 85 Boris Zbarsky [:bz] (still a bit busy) 2015-11-12 11:47:40 PST
Comment on attachment 8683304 [details] [diff] [review]
metacsp_part2_principal_changes.patch

>+  // If aPreloadCSP was already set, it should not be destroyed!
>+  // Instead, the current preloadCSP should be queried and extended
>+  // by calling appendPolicy() on it.

This should be in the API documentation, no?  That is, the IDL comments should describe how to use this attribute.

Would it make sense to automate it and instead have the API offer a getter and a way to add policies, where adding the first policy would create the CSP object internally?  Or does the object start out non-empty?

>+++ b/caps/nsSystemPrincipal.cpp
>+  // CSP on a null principal makes no sense

I know you copy/pasted this from the SetCsp case, but this is not the null principal.  ;)

r=me
Comment 86 Boris Zbarsky [:bz] (still a bit busy) 2015-11-12 11:57:57 PST
Comment on attachment 8683305 [details] [diff] [review]
metacsp_part3_upgrade_insecure_requests_changes.patch

>+nsDocument::ApplySettingsFromCSP(bool aSpeculative)

There's a bunch of code duplication here, and it's not obvious why the preload case still sets mReferrerPolicy/mReferrerPolicySet.  This last should at least be documented.

It might be better to have this function take nsIContentSecurityPolicy* and references/pointers to the members to update.  Then callers could document why they're passing in the particular members they want to update, and we'd only need one copy of this code...  The public API could call this function with the right pointers/references.

>+%{ C++
>+  inline bool GetUpgradeInsecurePreloads()

Does marking the attribute [infallible] not auto-generate this for you?

r=me with the above.
Comment 87 Boris Zbarsky [:bz] (still a bit busy) 2015-11-12 12:02:51 PST
Comment on attachment 8683307 [details] [diff] [review]
metacsp_part5_htmlmetaelement_changes.patch

>+    rv = GetContent(content);

Can't this (and the declaration of "content") move inside the "is descendant of head" block?

r=me, though again the existing API on nsIPrincipal seems a bit clunky.  Might be nice, in a followup, to just have an "append policy" API on it, and let it handle CSP object creation if it's the first policy.
Comment 88 Boris Zbarsky [:bz] (still a bit busy) 2015-11-12 12:03:02 PST
Comment on attachment 8683309 [details] [diff] [review]
metacsp_part7_preload_validation.patch

r=me
Comment 89 Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-11-12 15:04:00 PST
So one scenario that I suspect that this patch does not handle, is:

Page contains

<script>document.write("<!--");</script>
<meta http-equiv="Content-Security-Policy" content="image-src a.com">
<script>document.write("-->");</script>
<img src="http://a.com/image.jpg">

and where http://a.com/image.jpg redirects to http://b.com/image.jpg


There's a couple of potential edge cases here:
1. Parser starts preload of img at a.com
2. Image attempts to redirect to b.com and so we flag it as an error
3. <img> is inserted into a document and so we go look to see if there are matching preloads.

Sounds like this scenario is handled if we throw out any attempted load failures from the preload system

Another potential order of events:
1. Parser starts preload of img at a.com
2. <img> is inserted into a document and so we grab the existing in-progress preload.
3. Image attempts to redirect to b.com

Since the loadinfo still indicates that the channel is a preload in step 3 we'll probably fail the load.


Again, I'll leave it up to you to decide if we want to care about any of these scenarios.

One potential solution would be that at the time when we go grab a preload, that check to see if there are any rules in the preload-csp-policy list that is *not* in the normal-csp-policy list. If that's the case, then we ignore the preloads and always create a new channel.

But if the normal-csp-policy list is a strict superset of the preload-csp-policy, then we use the preloads as normal.


Again, up to you.
Comment 90 Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-11-12 15:11:22 PST
Comment on attachment 8686713 [details] [diff] [review]
metacsp_part1_csp_parser_changes.patch

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

r=me other than that.

::: dom/security/nsCSPParser.cpp
@@ +996,5 @@
> +  // CSP delivered via meta tag should ignore the following directives:
> +  // report-uri, frame-ancestors, and sandbox, see:
> +  // http://www.w3.org/TR/CSP11/#delivery-html-meta-element
> +  if (mDeliveredViaMetaTag &&
> +       ((CSP_IsDirective(mCurDir[0], nsIContentSecurityPolicy::REPORT_URI_DIRECTIVE)) ||

Shouldn't this be 

((CSP_IsDirective(mCurToken, ...

?
Comment 91 Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-11-12 16:01:04 PST
Comment on attachment 8683311 [details] [diff] [review]
metacsp_test2.patch

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

Let me know if you really prefer to keep things as-is.

::: dom/security/test/csp/test_meta_header_dual.html
@@ +20,5 @@
> +
> +const TESTS = [
> +  {
> +    /* load image without any CSP */
> +    query: "test1",

FWIW, I prefer the style where you here indicate which parts the server should send. That way you can look in one place to see which parts are sent, and what the expected result is. I.e. do something like

query: ["HTML_HEAD", "META_CSP_BLOCK_IMG", "HTML_BODY"]

and then

document.getElementById("testframe").src = "file_meta_header_dual.sjs?" + escape(JSON.stringify(curTest.query)

and then in the .sjs file parse that back into an array and have

{ HTML_HEAD: "...."
  HTML_BODY: "...." }

That'll make it easy for the .sjs file to be able to be generic and put together any test.
Comment 92 Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-11-12 16:02:50 PST
Comment on attachment 8683314 [details] [diff] [review]
metacsp_test4.patch

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

Same here
Comment 93 Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-11-12 16:26:57 PST
Comment on attachment 8683315 [details] [diff] [review]
metacsp_test5.patch

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

::: dom/security/test/csp/file_doccomment_meta.html
@@ +3,5 @@
> +<head>
> +  <title>Bug 663570 - Test doc.write(meta csp)</title>
> +  <meta charset="utf-8">
> +
> +  <!-- Use doc.write() to apply meta csp -->

"to *un*apply meta csp"

right?

@@ +15,5 @@
> +  <!-- try to load a css on a page where meta CSP is commented out -->
> +  <link rel="stylesheet" type="text/css" href="file_docwrite_meta.css">
> +
> +  <!-- try to load a script on a page where meta CSP is commented out -->
> +  <script id="testscript" src="file_docwrite_meta.js"></script>

Note that I suspect that we're only getting this right in the most simple cases.

I.e. if you were to set "script-src a.com" in the commented-out CSP, and then have a script which loads from a.com but then redirects to b.com, you'd probably fail here.
Comment 94 Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-11-12 16:27:41 PST
Comment on attachment 8683316 [details] [diff] [review]
metacsp_test6.patch

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

double-r=me
Comment 95 Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-11-12 18:30:00 PST
Comment on attachment 8683308 [details] [diff] [review]
metacsp_part6_csp_preloads_changes.patch

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

r=me with that addressed

::: dom/security/nsCSPService.cpp
@@ +212,5 @@
>  
> +  // 1) Apply speculate CSP for preloads
> +  if (aContentType == nsIContentPolicy::TYPE_INTERNAL_SCRIPT_PRELOAD ||
> +      aContentType == nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD ||
> +      aContentType == nsIContentPolicy::TYPE_INTERNAL_IMAGE_PRELOAD) {

Please add a nsContentUtils method for this next to the function which converts internal->external types. Use that in all three places where you do this check.

@@ +336,4 @@
>  
> +  // 1) Apply speculative CSP for preloads
> +  if (isPreload) {
> +    nsCOMPtr<nsIContentSecurityPolicy> preloadCsp;

Please break this out to a helper function.

@@ +346,5 @@
> +                             newUri,         // nsIURI
> +                             nullptr,        // nsIURI
> +                             nullptr,        // nsISupports
> +                             EmptyCString(), // ACString - MIME guess
> +                             originalUri,    // aMimeTypeGuess

The "aMimeTypeGuess" comment is not correct.
Comment 96 Boris Zbarsky [:bz] (still a bit busy) 2015-11-13 11:04:49 PST
Comment on attachment 8683306 [details] [diff] [review]
metacsp_part4_speculative_parser_changes.patch

>+      aExecutor->SetSpeculationCSP(mMetaCSP);

mUrl/mReferrerPolicy/mMetaCSP are mutually exclusive, right?  Why can't we just use a single string (probably names mUrlOrMetaCSPOrReferrerPolicy or some such) for all of them instead of adding more bloat to speculative load objects?

r=me modulo that and my earlier comments about the nsIPrincipal API.  It should just handle creating the object internally so callers don't have to do this get-and-maybe-create dance.
Comment 97 Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-11-13 11:21:00 PST
Comment on attachment 8683311 [details] [diff] [review]
metacsp_test2.patch

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

This is not critical since it sounds like this is more urgent
Comment 98 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:35:09 PST
Created attachment 8687605 [details] [diff] [review]
metacsp_part1_csp_parser_changes.patch
Comment 99 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:35:29 PST
Created attachment 8687606 [details] [diff] [review]
metacsp_part2_principal_changes.patch
Comment 100 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:35:50 PST
Created attachment 8687607 [details] [diff] [review]
metacsp_part3_upgrade_insecure_requests_changes.patch
Comment 101 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:36:11 PST
Created attachment 8687608 [details] [diff] [review]
metacsp_part4_speculative_parser_changes.patch
Comment 102 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:36:41 PST
Created attachment 8687609 [details] [diff] [review]
metacsp_part5_htmlmetaelement_changes.patch
Comment 103 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:37:02 PST
Created attachment 8687610 [details] [diff] [review]
metacsp_part6_csp_preloads_changes.patch
Comment 104 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:37:23 PST
Created attachment 8687611 [details] [diff] [review]
metacsp_part7_preload_validation.patch
Comment 105 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:37:44 PST
Created attachment 8687612 [details] [diff] [review]
metacsp_test1.patch
Comment 106 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:38:06 PST
Created attachment 8687613 [details] [diff] [review]
metacsp_test2.patch
Comment 107 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:38:28 PST
Created attachment 8687614 [details] [diff] [review]
metacsp_test3.patch
Comment 108 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:38:48 PST
Created attachment 8687615 [details] [diff] [review]
metacsp_test4.patch
Comment 109 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:39:12 PST
Created attachment 8687616 [details] [diff] [review]
metacsp_test5.patch
Comment 110 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:39:37 PST
Created attachment 8687617 [details] [diff] [review]
metacsp_test6.patch
Comment 111 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2015-11-14 19:46:13 PST
Thanks to all the reviewiers, I incorporated all of your suggestions and nitpicks, but there are 2 things I haven't addressed:
1) The document::InitCSP() is rather complicated and needs refactoring by itself. I filed bug 1224694 to refactor all of the 'set and init' functions/methods for CSP, which includes a nicer API for CSP on the principal.
2) As suggested within Comment 96 I tried to unify mUrl/mReferrerPolicy/mMetaCSP into one string within the speculative parser. Turns out they are not mutually exclusive and are causing test failures (e.g test_img_picture_preload.html). Hence I added a separate string for metaCSP to the speculative parser. If we want to refactor that, we can do that in a separate bug.
Comment 114 Sylvestre Ledru [:sylvestre] 2016-01-19 02:21:49 PST
Christoph, I wonder if we should not add this to the release notes for 45, what do you think?
Comment 115 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2016-01-19 09:36:23 PST
(In reply to Sylvestre Ledru [:sylvestre] from comment #114)
> Christoph, I wonder if we should not add this to the release notes for 45,
> what do you think?

Yes, I think that makes a lot of sense. How does the process work? Anything you need from my side?
Comment 116 Sylvestre Ledru [:sylvestre] 2016-01-20 06:39:09 PST
In the tracking flags, select relnote-firefox, fill the form and that it is :)
Comment 117 Christoph Kerschbaumer [:ckerschb (back on Dec. 5th)] 2016-01-21 14:13:53 PST
Release Note Request (optional, but appreciated)
The only way for a webpage to deliver and apply a CSP to a webpage was (until now) to send the CSP through a header. With the fix of this bug CSP can be delivered through a meta tag within the html markup using Firefox.

[Why is this notable]:
Provides an easy way to apply and deliver a CSP with a webpage.

[Suggested wording]:
Supports delivery of a Content Security Policy (CSP) via a meta tag

[Links (documentation, blog post, etc)]:
(general CSP documentation) https://developer.mozilla.org/en-US/docs/Web/Security/CSP
(meta CSP specific) https://www.w3.org/TR/CSP2/#delivery-html-meta-element
Comment 118 Sylvestre Ledru [:sylvestre] 2016-01-25 07:18:29 PST
Added with your wording (thanks btw)
I will update the release notes when we have a dev doc

Note You need to log in before you can comment on or make changes to this bug.