CSP standard says policies can be declared via <meta http-equiv="X-Content-Security-Policy"> element.

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.

Apparently some sites are already serving CSP via the meta tag (Chrome has implemented support for this already, AIUI)

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.

(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.

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.

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.

Maybe the report-uri directive should be restricted to the header?

If we don't ignore report-uri in a <meta> policy altogether we should at the very least reinstate the same-origin restriction.

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.

TIL about qref. @geekboy: sorry about that. It is missing 4 lines. I will add tests in a separate patch

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).

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 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++)?

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 on attachment 637270 [details] [diff] [review] Add meta tag support to CSP Seems very reasonable to me.

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.

Mail dmandelin and ask?

What's the status on this? This is blocking getting CSP to protect chrome pages.

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 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).

Another todo: ignore frame-ancestors in policies specified via meta tag. The CSP 1.1 draft says to do this.

I believe this would help improve HTML email viewing in Gaia's email client, more details in bug 1102469 comment 1.

Johnny, you reviewed basically all of the CSP changed within DOM. Please feel free to delegate to other peers though.

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?

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 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.

(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!

> 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?

(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.

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

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?

(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!

Marking Bug 1048048 as blocking. We can add the new contentPolicy_types in that bug and than use them in this bug.

Is CSP meta tag support in Nightly? How can I test it?

(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 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.

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!

> 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.

(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

(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.

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....

(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?

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?

(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.

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.

Hmm. Would we end up using or not using the preload in that situation?

(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)

> 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?

(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.

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 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 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.

(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?

(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 :-(

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?

> 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?

[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.

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

[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).

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

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

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

[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 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?

(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 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.

(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.

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 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 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 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 on attachment 8683309 [details] [diff] [review] metacsp_part7_preload_validation.patch r=me

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 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 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 on attachment 8683314 [details] [diff] [review] metacsp_test4.patch Review of attachment 8683314 [details] [diff] [review]: ----------------------------------------------------------------- Same here

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 on attachment 8683316 [details] [diff] [review] metacsp_test6.patch Review of attachment 8683316 [details] [diff] [review]: ----------------------------------------------------------------- double-r=me

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 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 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

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/632197cb1589 https://hg.mozilla.org/integration/mozilla-inbound/rev/7939f4d62b78 https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1a64890d95 https://hg.mozilla.org/integration/mozilla-inbound/rev/08a0e6fdb3ff https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9a7333c030 https://hg.mozilla.org/integration/mozilla-inbound/rev/149cc9f89a2e https://hg.mozilla.org/integration/mozilla-inbound/rev/32b101a15f2f https://hg.mozilla.org/integration/mozilla-inbound/rev/24577613f42d https://hg.mozilla.org/integration/mozilla-inbound/rev/8021ae61935b https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d25972ed8b https://hg.mozilla.org/integration/mozilla-inbound/rev/7a9d4687ad06 https://hg.mozilla.org/integration/mozilla-inbound/rev/14756127759a https://hg.mozilla.org/integration/mozilla-inbound/rev/0f6a404c8d78

https://hg.mozilla.org/mozilla-central/rev/632197cb1589 https://hg.mozilla.org/mozilla-central/rev/7939f4d62b78 https://hg.mozilla.org/mozilla-central/rev/0f1a64890d95 https://hg.mozilla.org/mozilla-central/rev/08a0e6fdb3ff https://hg.mozilla.org/mozilla-central/rev/5c9a7333c030 https://hg.mozilla.org/mozilla-central/rev/149cc9f89a2e https://hg.mozilla.org/mozilla-central/rev/32b101a15f2f https://hg.mozilla.org/mozilla-central/rev/24577613f42d https://hg.mozilla.org/mozilla-central/rev/8021ae61935b https://hg.mozilla.org/mozilla-central/rev/c6d25972ed8b https://hg.mozilla.org/mozilla-central/rev/7a9d4687ad06 https://hg.mozilla.org/mozilla-central/rev/14756127759a https://hg.mozilla.org/mozilla-central/rev/0f6a404c8d78

Christoph, I wonder if we should not add this to the release notes for 45, what do you think?

(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?

In the tracking flags, select relnote-firefox, fill the form and that it is :)

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

Added with your wording (thanks btw) I will update the release notes when we have a dev doc

https://developer.mozilla.org/en-US/Firefox/Releases/45#Security https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy#Browser_compatibility