User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 Build ID: 20180122220231 Steps to reproduce: The URLs located in `resource://devtools-client-jsonview/.*` bypass CSP. ( maybe intentionally ) A `require.js` file is in the this directory and it can be used for CSP strict-dynamic bypass against any websites. Steps to reproduce: 1. Go to attached PoC( csp_bypass_require.html ). This page has strict CSP rules: <meta http-equiv="Content-Security-Policy" content="default-src 'none';script-src 'nonce-random' 'strict-dynamic'"> 2. However, you can see `alert(1)`. It is executed from the following code: <script data-main='data:,alert(1)'></script> <script src="resource://devtools-client-jsonview/lib/require.js"></script> Actual results: CSP strict-dynamic is bypassed via browser resources. Expected results: CSP strict-dynamic should not be bypassed via browser resources.

FYI, the possibility of strict-dynamic bypass by `require.js` is already known. You can find some CSP bypass variations including it from this research by Google: https://github.com/google/security-research-pocs/tree/master/script-gadgets

Honza/Christoph, do you have ideas about where this should live / where/how we can address this? Thanks!

FWIW, this whitelisting of resource URIs is happening here: https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#88-97 I think the bug should live in DOM:Security since it's a CSP bug, but I don't know what the best way of addressing this problem is. Ultimately browser internals should not be affected by a page's CSP. This case makes a unified behavior tricky however. I am happy to implement the changes here, but I don't know what's the best strategy to accomplish.

I don't have enough experience with CSP to help here. Honza

I understand why we're loading the resource: url despite the CSP. Now that we don't support legacy extensions maybe we can stop doing that. But I don't understand how react.js executes the data-main if CSP is preventing inline script and eval. Does react know about strict-dynamic and dig up the nonce to create passable scripts?

Not sure we should be exposing react to every web page. Can this resource be made non-accessible? I don't know if resource: supports that like chrome: and moz-extension:

Sorry, "require.js", not "react". Thinking of the wrong library.

(In reply to Daniel Veditz [:dveditz] from comment #5) >I don't understand how react.js executes the data-main if CSP is preventing inline script and eval. Basically require.js executes the following code using the data-main attribute: script=document.createElement('script'); script.src='data:,alert(1)'; document.head.appendChild(script); You can confirm this behavior by putting a break point here: https://dxr.mozilla.org/mozilla-beta/source/devtools/client/jsonview/lib/require.js#1903 A strict-dynamic does not block the script execution via non-"parser-inserted" script elements: https://www.w3.org/TR/CSP3/#strict-dynamic-usage >Script requests which are triggered by non-"parser-inserted" script elements are allowed. Due to this, it works.

(In reply to Daniel Veditz [:dveditz] from comment #6) > Can this resource be > made non-accessible? I don't know if resource: supports that like chrome: > and moz-extension: resource: in principle supports this (bug 863246). But the JSON view executes in a content-privileged sandbox, and so I don't know if it would be possible to restrict its own script sources in this way. I would be somewhat surprised if so. (In reply to Christoph Kerschbaumer [:ckerschb] from comment #3) > FWIW, this whitelisting of resource URIs is happening here: > > https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService. > cpp#88-97 Like Dan, I don't think I understand why this is necessary. Why can't we just make resource: URIs obey CSP? > Ultimately browser internals should not be affected by a page's CSP. I agree, but certainly for the JSON viewer, the viewer has control over the relevant CSP headers because it munges the request, AIUI. Presumably it could set different headers, ie here: https://dxr.mozilla.org/mozilla-central/rev/c4ebc8c28a33b785dfbfa533810517cc707d1ad0/devtools/client/jsonview/converter-child.js#79 ? Christoph, would that work? Why do we specialcase resource: at all?

(In reply to :Gijs (lower availability until Jan 27) from comment #9) > Like Dan, I don't think I understand why this is necessary. Why can't we > just make resource: URIs obey CSP? That decision predates my time at Mozilla. I vaguely remember when we rewrote CSP in C++ (formerly our implementation was based on JS) we already whitelisted resource URIs, I guess mostly because of extensions. Unfortunately I couldn't dig out the exact bug, only the one that introduces subjectToCSP(), but even that bug didn't change that exclusion of resource: loads for CSP [1]. Anyway, I agree with both of you, probably we don't have to specialcase resource URIs now that we don't support legacy extensions anymore. Let's see how TRY likes that and we can take it from there: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ef53f8052af82b2abc53de2ddcbe59969e92c4b [1] https://hg.mozilla.org/integration/mozilla-inbound/rev/80b2348e3ab6#l1.109

Given the TRY run and our reasoning I think we can change subjectToCSP() to include resource URIs. It seems the only test affected is browser_jsonview_csp_json.js within devtools - I am not sure if this change of making resource URIs subject to CSP will affect more things within devtools. Honza, any insights? Also we would have to fix browser_jsonview_csp_json.js, any suggestions how to accomplish that? Would it make sense to land the change but also add a pref around it, in case we have to revert the change for any reason?

Honza, please have a look at comment 11 - thanks!

And here is the automated test!

Just chatted with Patrick on IRC and it seems that most likely that change of having CSP apply to resource: URIs will affect devtools a whole lot. Also given browser_jsonview_csp_json.js. Exactly what we are trying to prevent, devtools code relies on. E.g. the error on the console for browser_jsonview_csp_json.js is: Content Security Policy: The page’s settings blocked the loading of a resource at resource://devtools-client-jsonview/css/main.css (“default-src”). Content Security Policy: The page’s settings blocked the loading of a resource at resource://devtools-client-jsonview/lib/require.js (“default-src”)

Comment on attachment 8945065 [details] [diff] [review] bug_1432358_apply_csp_to_resource_uri.patch Review of attachment 8945065 [details] [diff] [review]: ----------------------------------------------------------------- So, luckily, as far as I can tell this require.js file isn't accessible via chrome:// , for one because it's not packaged / in the same location as the chrome:// stuff, and also because chrome://devtools isn't contentaccessible . But that seems a fragile thing to rely on. Why do we need to have any exceptions here? Does more stuff break if chrome:// is subject to CSP? I guess I kind of assume that things that run in content frame scripts and so on have chrome privileges and already don't encounter CSP, irrespective of the URIs of their respective resources, and I can think of no cases where we directly intend to inject chrome:// URI stuff into web content (that is subject to CSP). ::: dom/security/nsCSPService.cpp @@ +57,2 @@ > // protocol flag (URI_IS_LOCAL_RESOURCE) with other protocols, like > + // chrome: and moz-icon:, but those protocols are subject to CSP, I don't really know why we shouldn't make chrome and moz-icon subject to CSP, too... @@ +89,5 @@ > } > > // Other protocols are not subject to CSP and can be whitelisted: > // * URI_IS_LOCAL_RESOURCE > + // e.g. chrome:, data:, blob:, moz-icon: Looks like data: and blob: also want removing from this list.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14) > Just chatted with Patrick on IRC and it seems that most likely that change > of having CSP apply to resource: URIs will affect devtools a whole lot. Also > given browser_jsonview_csp_json.js. Exactly what we are trying to prevent, > devtools code relies on. E.g. the error on the console for > browser_jsonview_csp_json.js is: > > Content Security Policy: The page’s settings blocked the loading of a > resource at resource://devtools-client-jsonview/css/main.css > (“default-src”). > Content Security Policy: The page’s settings blocked the loading of a > resource at resource://devtools-client-jsonview/lib/require.js > (“default-src”) Yes. My suggestion (poorly expressed) in comment #9, is that we alter the CSP header / data when invoking the JSON viewer, and make specific exceptions for the sources we need in addition to whatever the contents of the CSP are at that point.

Comment on attachment 8945066 [details] [diff] [review] bug_1432358_test_resource_uri.patch Review of attachment 8945066 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/test/csp/file_resource_url.html @@ +12,5 @@ > +<script>document.getElementById("regScriptDiv").textContent="allowed";</script> > + > +<!-- resource URL test --> > +<script data-main='data:,document.getElementById("resourceScriptDiv").textContent="allowed;"'></script> > +<script src="resource://devtools-client-jsonview/lib/require.js"></script> I think we shouldn't make this dom/ test rely on the contents of a devtools bit of code. We should make the test create its own resource: mapping dynamically (at runtime) using setSubstitutionWithFlags on the protocol handler (making sure to make the relevant resource: thing contentaccessible) and then attempt to access that from the file we load.

(In reply to :Gijs (lower availability until Jan 27) from comment #15) > Why do we need to have any > exceptions here? Does more stuff break if chrome:// is subject to CSP? I > guess I kind of assume that things that run in content frame scripts and so > on have chrome privileges and already don't encounter CSP, irrespective of > the URIs of their respective resources, and I can think of no cases where we > directly intend to inject chrome:// URI stuff into web content (that is > subject to CSP). That seems like a fair assumption. Nothing that executes with system privileges is subject to CSP, so I think it makes sense what you are suggesting and we can also make chrome: and moz-icon subject to CSP.

(In reply to :Gijs (lower availability until Jan 27) from comment #15) >So, luckily, as far as I can tell this require.js file isn't accessible via chrome:// I noticed require.js can be fetched via chrome:// URL also. We can access from: chrome://browser/content//chrome/devtools/modules/devtools/client/jsonview/lib/require.js Please take notice to the two slashes of "content//chrome". I think this is an another issue which allows path traversal.

(In reply to Masato Kinugawa from comment #19) > Created attachment 8945081 [details] > csp_bypass_require_chromeurl.html > > (In reply to :Gijs (lower availability until Jan 27) from comment #15) > >So, luckily, as far as I can tell this require.js file isn't accessible via chrome:// > I noticed require.js can be fetched via chrome:// URL also. > We can access from: > chrome://browser/content//chrome/devtools/modules/devtools/client/jsonview/ > lib/require.js > Please take notice to the two slashes of "content//chrome". I think this is > an another issue which allows path traversal. Please file a separate bug for this.

(In reply to :Gijs (lower availability until Jan 27) from comment #20) > Please file a separate bug for this. Filed as bug 1432870.

Gijs, as agreed we should also have subjectToCSP apply to chrome: URIs in addition to resource URIs. Try seems happy overall [1], there are only two issues: * test_ext_contentscript_triggeringPrincipal.js * browser_jsonview_csp_json.js Once you are fine with the patch here I will follow up with devtools folks to get those issues resolved.

Oh, I forgot, here is the TRY run for comment 22: [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2182293c22c9ea9634ff9b0c9c376fe8a940161&selectedJob=158207724

And here is the automated test where we register a resource URI. Intially I wanted to capture the precise bypass, hence the initial test was based on require.js. Anyway, this test is self contained now and registers it's own chrome URI.

Bit busy tonight, but from a quick look at the patch, why can't we just return true for all URIs with URI_IS_LOCAL_RESOURCE, rather than enumerating all the sub-cases here? Does that not cover chrome: or some other URI schemes? That is, prior to the patch, the state was: // some local URI schemes bad: if (scheme == "foo") { return true; } if (scheme == "bar") { return true; } // all other local schemes good: if (uri.hasFlag(local)) { return false; } Which URI schemes do we actually still need to treat as "good" here? Can't we just flip the default to "all local URI schemes participate in CSP", and only make exceptions for schemes with this flag that really really don't (maybe file: which I don't currently see mentioned but presumably has this flag?) ? [Though I also don't know why file: doesn't participate in CSP, but that's a question for another bug...)

(In reply to :Gijs (lower availability until Jan 27) from comment #25) > Bit busy tonight, but from a quick look at the patch, why can't we just > return true for all URIs with URI_IS_LOCAL_RESOURCE, rather than enumerating > all the sub-cases here? Potentially we can, but I am not sure if we should do that within this bug (which most likely will be uplifted) or rather file a follow up to make sure that assumption is not breaking anything else. I think the important part here is that resource and chrome become subject to CSP, right? One of my question is, can anyone still register it's own ProtocolHandler besides Firefox itself these days given that legacy extensions are gone? If not, then the follow up will become a little easier. What do you think?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #26) > One of my question is, can anyone still register it's own ProtocolHandler > besides Firefox itself these days given that legacy extensions are gone? No. Only web-based registerProtocolHandler, which shouldn't ever get URI_IS_LOCAL_RESOURCE. > If not, then the follow up will become a little easier. What do you think? Yep, but it's also OK to do this in a separate bug as you suggested, if that's easier.

Comment on attachment 8945177 [details] [diff] [review] bug_1432358_apply_csp_to_resource_uri.patch Review of attachment 8945177 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/nsCSPService.cpp @@ +61,2 @@ > // http://www.w3.org/TR/CSP2/#source-list-guid-matching > + Nit: whitespace @@ +101,5 @@ > + } > + rv = aURI->SchemeIs("moz-icon", &match); > + if (NS_SUCCEEDED(rv) && match) { > + return true; > + } I wonder if, given the number of scheme comparisons here, this would be better written by just getting the scheme and using EqualsLiteral. Anyway, this is certainly OK for fixing this bug if you prefer not rewriting this method that drastically.

Comment on attachment 8945178 [details] [diff] [review] bug_1432358_test_subjecttocsp.patch Review of attachment 8945178 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/test/csp/test_resource_uri.html @@ +33,5 @@ > + // html file does apply meta CSP of: > + // default-src 'none'; script-src 'nonce-random' 'strict-dynamic' > + file: "file_resource_uri.html", > + result: "blocked", > + description: "resource: URI should be blocked by CSP", Should we have a third test that we can allow the resource again if the full URI is included in the list of script-src's in the CSP? @@ +42,5 @@ > + resourceHandler.setSubstitution("csptest", null); > + SimpleTest.finish(); > +} > + > +function ckeckResult() { Should be 'checkResult', I think.

[Carrying over r+ from Gijs] (In reply to :Gijs (lower availability until Jan 27) from comment #28) > I wonder if, given the number of scheme comparisons here, this would be > better written by just getting the scheme and using EqualsLiteral. Anyway, > this is certainly OK for fixing this bug if you prefer not rewriting this > method that drastically. I think that makes sense and I have considered that as well. As agreed we can clean that entire function up in a follow up bug.

[Carrying over r+ from Gijs] (In reply to :Gijs (lower availability until Jan 27) from comment #29) > Should we have a third test that we can allow the resource again if the full > URI is included in the list of script-src's in the CSP? I like that suggestion, thanks. I have incorporated such a test within this patch.

Hey Kris, within this bug we are changing the behavior of CSP and make CSP apply to chrome: and resource: URIs as well. In turn this change causes test test_ext_contentscript_triggeringPrincipal.js. The test applies default-src 'none' which with the patches from this bug applied causes the file chrome://global/content/bindings/scrollbar.xml to be blocked within your test. I don't know if this change might have bigger implications than just fixing the test, so I am relying on your input. Anyway, the *.xml file loads uses the internal load type of TYPE_XBL which is governed by default-src within CSP. Whitelisting chrome: within default-src fixes the test. Am I missing something or is it fine to just update the test?

Honza, as discussed on IRC; let's whitelist chrome: and resource: for that particular test for now with the potential problem that the code changes within this bug might affect devtools on a broader scale. We will have to do some thorough testing to make sure devtools still work in combination with CSP. FWIW, I am whitelisting chrome and resource within that test because of: * resource://devtools-client-jsonview/css/main.css * chrome://global/content/bindings/scrollbar.xml

Comment on attachment 8945483 [details] [diff] [review] bug_1432358_update_test_jsonview.patch Review of attachment 8945483 [details] [diff] [review]: ----------------------------------------------------------------- As we discussed on Slack, whitelisting the resource: protocol looks ok. Thanks for the patch! Honza

Comment on attachment 8945444 [details] [diff] [review] bug_1432358_update_test_triggeringPrincipal.patch Review of attachment 8945444 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this affects just this test. This test doesn't particularly rely on the scrollbar.xml binding. That binding just happens to get loaded into most content pages, and this page has a (perfectly ordinary) CSP which blocks it. If these patches are causing this test to fail because the CSP blocks scrollbar.xml from loading, it probably also breaks scrollbars on any site with a similarly strict CSP.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #35) > I don't think this affects just this test. This test doesn't particularly > rely on the scrollbar.xml binding. That binding just happens to get loaded > into most content pages, and this page has a (perfectly ordinary) CSP which > blocks it. If these patches are causing this test to fail because the CSP > blocks scrollbar.xml from loading, it probably also breaks scrollbars on any > site with a similarly strict CSP. Gijs, I guess that puts us back to the start, right? Any suggestions on how to proceed?

Depending on the principal of the stylesheet that applies those bindings, it might be enough to make content injected by system principals immune to CSP here: https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/caps/BasePrincipal.h#136-154 That would leave chrome: and resource: URLs subject to CSP in the general case, but allow them when they're injected by chrome.

Re: resource:/chrome: generally: We should try Kris' suggestion from comment #37. Otherwise we can limit the new behaviour to resource: and fix chrome: separately. (In reply to Jan Honza Odvarko [:Honza] from comment #34) > Comment on attachment 8945483 [details] [diff] [review] > bug_1432358_update_test_jsonview.patch > > Review of attachment 8945483 [details] [diff] [review]: > ----------------------------------------------------------------- > > As we discussed on Slack, whitelisting the resource: protocol looks ok. I have to admit this patch confuses me. My understanding (though I haven't tested to verify that...) is that this patch will break the json viewer on any JSON that has strict CSP, unless we hack up the jsonview content transformer/handler (that actually includes all the resource: based script etc. ) to force a change to the CSP to always allow resource: for content viewed in the JSON viewer. But the patch only fixes 1 test file, not the jsonview behaviour...

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #37) > Depending on the principal of the stylesheet that applies those bindings, it > might be enough to make content injected by system principals immune to CSP > here: > > https://searchfox.org/mozilla-central/rev/ > 062e1cf551f5bf3f0af33671b818f75a55ac497b/caps/BasePrincipal.h#136-154 > > That would leave chrome: and resource: URLs subject to CSP in the general > case, but allow them when they're injected by chrome. Kris, thanks for the hint, I've tried that, but it seems content is *not* injected by the systemPrincipal in that case, at least not for the test test_ext_contentscript_triggeringPrincipal.js. Any other suggestions that I could try to incorporate?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #39) > Kris, thanks for the hint, I've tried that, but it seems content is *not* > injected by the systemPrincipal in that case, at least not for the test > test_ext_contentscript_triggeringPrincipal.js. Any other suggestions that I > could try to incorporate? Ah, I see why. So your suggestion only applies to loads of TYPE_STYLESHEET (see [1]). But we are loading chrome://global/content/bindings/scrollbar.xml with TYPE_XBL which then presumably uses a different code path. Probably we can do a similar whitelisting as you suggested for the *.xml. Let me see if I can find the right place in the code for that. I think that should be doable though. [1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleUtil.cpp#887

Kris, thanks for your suggestion. It all makes sense now. Loads triggered by the system should bypass the CSP of the page. I don't know why that didn't work when I first tried this morning, but having overrideCSP() return true when called on a SystemPrincipal allows loading an XBL if triggered by the system here [1]. That whitelisting in turn allows test_ext_contentscript_triggeringPrincipal.js to work properly again. Do you feel comfortable accepting that change or do we need someone else to sign off on it as well? [1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#142

Comment on attachment 8946410 [details] [diff] [review] bug_1432358_update_overridescsp.patch Review of attachment 8946410 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.h @@ +135,5 @@ > > /** > + * Returns true if this principal's CSP should override a document's > + * CSP for loads that it triggers. Currently true for system > + * principals and for expanded principals which subsume the document Nit: s/ and for/,/ Also, should probably be "the system principal" rather than "system principals", since there's only ever one.

(In reply to :Gijs from comment #38) > I have to admit this patch confuses me. My understanding (though I haven't > tested to verify that...) is that this patch will break the json viewer on > any JSON that has strict CSP, unless we hack up the jsonview content > transformer/handler (that actually includes all the resource: based script > etc. ) to force a change to the CSP to always allow resource: for content > viewed in the JSON viewer. But the patch only fixes 1 test file, not the > jsonview behaviour... Gijs, I think you are right. I tried to debug that part again and it seems we are loading require.js using a resource:// URI within the JSON converter here [1]. I agree that this patch most likely will only fix the test browser_jsonview_csp_json.js but most likely will cause more problems around the JSON converter in the wild (whenever a page uses a strong CSP). Currently the triggeringPrincipal for loading resource://devtools-client-jsonview/lib/require.js is a NullPrincipal so the previous patch to overrule a pages CSP has no effect on the JSON converter code. Do you have any suggestions how to update the JSON converter code [1] so it's not subject to a page's CSP? [1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/converter-child.js#252

Talked about this with Christoph out-of-band. We can either do this such that CSPs don't apply to the jsonviewer, or overwrite the CSP with our own thing that only allows our own resources and nothing else. We'll do whatever is expedient given the immediate nature of the issue.

Olli, within this patch we want to make chrome: and resource: URIs to become subject to CSP. In turn, this change will break the json viewer, because it loads resource: URIs, e.g. here [1] which might get blocked by a document's CSP now. Within our own testsuite this change causes browser_jsonview_csp_json.js to fail. I thought of a few things to make this work. Ultimately, I think the best approach is to allow dropping the CSP in that specific case. I tried to minimize the risk by making the function a no-op for all principals other than a NullPrincipal. FWIW, the load uses a NullPrincipal because we explicitly reset to do so here [2]. I know it's a little hacky, but I think it's the best option we have to make this work again. What do you think? [1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/converter-child.js#252 [2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/converter-child.js#95

Honza, could you explain the jsonviewer setup? How does it even end up using csp from the json response?

Comment on attachment 8947044 [details] [diff] [review] bug_1432358_allow_dropping_csp.patch Can't really review this atm. It smells like we shouldn't inherit the CSP from the beginning, if we end up wanting to drop it anyhow. So, should something somewhere create a null principal without csp? But I don't understand jsonviewer setup.

I don't know why it's using the same CSP, but the initial setup happens in onStartRequest (converter-child.js). This method converts plain JSON into an application that is rendered instead (in the same tab/window). The HTML app is generated by initialHTML() method (in the same file) The method also explicitly sets request.loadInfo.resetPrincipalToInheritToNullPrincipal(); (as mentioned in the previous comment) If we want to create new (and correct) principal it feels like it should be done at this place... Honza

Olli, initially I wanted to not even apply (or actually even parse) a CSP in that case. I thought, maybe we can detect the loading of a *.json file in nsDocument.cpp where we apply the CSP to the document (somewhere around here [1]). The problem is that we can't reliably detect JSON loading. In the specific case of browser_jsonview_csp_json.js for example we are loading the content type "text/html". Solely not applying a CSP based on the file ending is a no go anyway. So that option is out, or am I missing something? The next thing I tried is to overwrite the principal (that holds the CSP) with yet another freshly created NullPrincipal. But that's not doable. Let me explain: This [2] resetting of the principal to inherit on the loadInfo caused the new document's nodePrincipal to be that NullPrincipal. Resetting the LoadingPrincipal on the loadInfo after the NodePrincipal has been assigned on the document has no effect in terms of querying the CSP within the CSP-code anymore, because the CSP code queries the CSP from the loadingContext (see [3]) and not from the loadInfo. Since there is no document->SetNodePrincipal() we can not overwrite that principal anymore. The only option we have is, to get a pointer to the principal and drop the CSP, which is what my patch was doing. I am open to better suggestions, but I can't think of anything else. Again, ideally I would like to not even parse the CSP in case of loading a *.json, but I think that's not reliably doable. Are you willing to reconsider my patch for an r+? [1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2915 [2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/converter-child.js#95 [3] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#145

I'm worried about having a principal around which first has csp and then it suddenly loses it. What guarantees no code between those states try to access csp? Can we mark loadinfo or channel or something as don't-apply-csp? Do that before we end up setting up csp in nsDocument

(In reply to Olli Pettay [:smaug] from comment #50) > Can we mark loadinfo or channel or something as don't-apply-csp? Do that > before we end up setting up csp in nsDocument In fact I like that idea a lot - here we go. What do you think?

Comment on attachment 8947380 [details] [diff] [review] bug_1432358_channel_csp_agnostic.patch > NS_IMETHODIMP >+LoadInfo::SetAllowDocumentToBeAgnosticToCSP(bool aAllowDocumentToBeAgnosticToCSP) >+{ >+ if (mInternalContentPolicyType != nsIContentPolicy::TYPE_DOCUMENT) { >+ MOZ_ASSERT(false, "not available for on loads other than TYPE_DOCUMENT"); I think "not available for loads other than..." Yeah, I think this approach is rather clear.

Incorporated nits; carrying over r+ from kmag.

Incorporated nits; carrying over r+ from smaug.

Comment on attachment 8945420 [details] [diff] [review] bug_1432358_apply_csp_to_resource.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Not that easily, because an attacker would have to know that Firefox's file require.js looks for a script tag of 'data-main' which makes the Universal CSP bypass possible at all. See the following example: <script data-main='data:,alert(1)'></script> <script src="resource://devtools-client-jsonview/lib/require.js"></script> The automatic testcase here registers it's own resource file so we don't pinpount to the weakness for require.js explicitly. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. The patch including comments does not reveal the actual problem. See explanation above. Which older supported branches are affected by this flaw? I don't know offhand, most likely all supported branches are affected. Resource URIs have never been subject to CSP since the introduction of our CSP code. I guess the question is, when did require.js land, but I imagine that file has also been around for a very long time. If not all supported branches, which bug introduced the flaw? see above. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I don't have backports. I would imagine that the attached patches apply more or less cleanly to other affected branches. How likely is this patch to cause regressions; how much testing does it need? Not very likely, because less than %5 of webpages ship a CSP. Out of those only a very small percentage load resource:// URIs.

Comment on attachment 8945421 [details] [diff] [review] bug_1432358_test_subjecttocsp.patch [Security approval request comment] See comment 55.

Comment on attachment 8947427 [details] [diff] [review] bug_1432358_update_overridescsp.patch [Security approval request comment] See comment 55.

Comment on attachment 8947428 [details] [diff] [review] bug_1432358_channel_csp_agnostic.patch [Security approval request comment] See comment 55.

Tests aren't going to get sec-approval on a sec-high rated issue until after we ship it in a release. I'll clear the sec-approval on that. You should set in-testsuite? to remember to check the tests in four weeks after we ship a release with the fix. I'll give sec-approval+ on the patches for trunk. You should nominate Beta and ESR52 patches for this as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/60852dec9e041887bea80313a70ad2a4cba745a6 https://hg.mozilla.org/integration/mozilla-inbound/rev/6840396ed6ec8cdc9e77e253c1c61258a24fb987 https://hg.mozilla.org/integration/mozilla-inbound/rev/27527f95cccca4927d4fee56b0dab9af11c5733f

Backed out for failing xpcshell's test_ext_contentscript_triggeringPrincipal.js on Windows debug: https://hg.mozilla.org/integration/mozilla-inbound/rev/a824791e0d28623e928f9b5314cb6d948b2218ef Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=35b81e0007ab42a5c0e7ec037ee2295d6708607e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=161405747&repo=mozilla-inbound 20:55:02 INFO - TEST-START | xpcshell-e10s.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js 21:00:02 WARNING - TEST-UNEXPECTED-TIMEOUT | xpcshell-e10s.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js | Test timed out

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #61) > Backed out for failing xpcshell's > test_ext_contentscript_triggeringPrincipal.js on Windows debug: Kris, even though the test runs quite long it works locally. I guess the problem is that we just exceed the allowed timeout of that xpcshell test. Can we somehow request a longer timeout? I know we have requestLongerTimeout() on mochitests, not sure if we have an equivalent for xcpshell tests. What do you think?

Use requesttimeoutfactor = number in the xpcshell.ini. Code: https://dxr.mozilla.org/mozilla-central/rev/2b7d42d527af582a465e49187fe387442d524a7c/testing/xpcshell/runxpcshelltests.py#667 Example: https://dxr.mozilla.org/mozilla-central/rev/2b7d42d527af582a465e49187fe387442d524a7c/netwerk/test/unit/xpcshell.ini#88-89

Aryx, are you comfortable accepting this? If so, could you please also reland those patches for me by any chance?

Comment on attachment 8950155 [details] [diff] [review] bug_1432358_fix_xpcshell_test.patch Setting sec-approval for this part as well since it's only a test timeout update.

Pushed this before lunch: https://hg.mozilla.org/integration/mozilla-inbound/rev/91c948c94506089d6f40dc59d13c75ab78ce914d https://hg.mozilla.org/integration/mozilla-inbound/rev/a67e95bd0ccf13faa26aaa6b56fd1d826a7608ec https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa11c525da3d0c7ca58a593241c1902f2849528 https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7b8eef07c1181cec03246e40807d6051f971c9

Backed out for failing xpcshell's test_ext_contentscript_triggeringPrincipal.js on Windows debug: https://hg.mozilla.org/integration/mozilla-inbound/rev/5d726f46260f0d96dba2ec23d5b01375f588c6e1 First push which ran failing jobs: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=50145ed826265cd5e228fb7d973b871359ca2213&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=161700807&repo=mozilla-inbound 16:20:22 INFO - "Got CSP report for forbidden URL http://localhost:51627/audio-source.webm?origin=page&source=pageScript-relative-url" 16:20:22 INFO - "Got CSP report for forbidden URL http://localhost:51627/video.webm?origin=page&source=pageScript-relative-url" 16:20:22 INFO - "Got CSP report for forbidden URL http://localhost:51627/video-source.webm?origin=page&source=pageScript-relative-url" 16:20:22 INFO - <<<<<<< 16:20:22 INFO - mozcrash kill_pid(): wait failed (-1) terminating pid 11692: error 5 16:20:22 INFO - xpcshell return code: None 16:20:22 INFO - xpcshell-e10s.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js | Process still running after test! 16:20:22 INFO - TEST-START | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js 16:30:22 WARNING - TEST-UNEXPECTED-TIMEOUT | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js | Test timed out 16:30:22 INFO - TEST-INFO took 600000ms 16:30:22 INFO - >>>>>>>

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #67) > Backed out for failing xpcshell's > test_ext_contentscript_triggeringPrincipal.js on Windows debug: No idea why. Kris, any idea what might be wrong? Would you be strictly against disabling the test on windows debug or is that an option?

As discussed, going ahead and disabling the test to get this sec-high bug landed. I filed Bug 1438796 to figure out the problem and re-enable the test.

Comment on attachment 8951547 [details] [diff] [review] bug_1432358_fix_xpcshell_test.patch Setting sec-approval on this one as well since it's only a tmp disable of a windows debug test.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc871960d805efc5b3c4ac9e0682312d7bb23bda https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b53e720ac7c35192eada9bf7c174df78105504 https://hg.mozilla.org/integration/mozilla-inbound/rev/93dcf59ff87e075acefb7fd7634c270519a857f8 https://hg.mozilla.org/integration/mozilla-inbound/rev/9f37220bc5360c91a80278119a1ce2a4940d9aec

Test annotation fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4aa33646f8bb62025644f240b71af17a854fcca

https://hg.mozilla.org/mozilla-central/rev/fc871960d805 https://hg.mozilla.org/mozilla-central/rev/a8b53e720ac7 https://hg.mozilla.org/mozilla-central/rev/93dcf59ff87e https://hg.mozilla.org/mozilla-central/rev/9f37220bc536 https://hg.mozilla.org/mozilla-central/rev/d4aa33646f8b

Comment on attachment 8945420 [details] [diff] [review] bug_1432358_apply_csp_to_resource.patch Approval Request Comment [Feature/Bug causing the regression]: That problem has been around for a long time. If I would have to pick a bug (even though it's not the root cause of the problem, but allowed us to reveal the problem) then I would choose Bug 1299483 - CSP: Implement 'strict-dynamic'. [User impact if declined]: Universal CSP bypass using browser internals. [Is this code covered by automated tests?]: Yes, automatet test is attached to this bug. [Has the fix been verified in Nightly?]: No, not yet. [Needs manual test from QE? If yes, steps to reproduce]: No, has automated test which is sufficient. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: There is low risk involved in a sense that a page having CSP might block a content exposed chrome: URL which gets loaded by a webpage. [Why is the change risky/not risky?]: see previous comment [String changes made/needed]: no

Comment on attachment 8945421 [details] [diff] [review] bug_1432358_test_subjecttocsp.patch Approval Request Comment See Comment 74

Comment on attachment 8947427 [details] [diff] [review] bug_1432358_update_overridescsp.patch Approval Request Comment See Comment 74

Comment on attachment 8947428 [details] [diff] [review] bug_1432358_channel_csp_agnostic.patch Approval Request Comment See Comment 74

Comment on attachment 8951547 [details] [diff] [review] bug_1432358_fix_xpcshell_test.patch Approval Request Comment See Comment 74

Comment on attachment 8945420 [details] [diff] [review] bug_1432358_apply_csp_to_resource.patch Just discussed things we Gijs again and it seems it's probably not worth uplifting that change, given the possibility that this might break some pages.

Updating status flags per the recent discussion in the bug and with Dan on IRC. That said, it isn't clear to me if ESR52 is wontfix at this point or if we'd still want to try to ship a fix in 52.8 after this has had more opportunity for testing.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #79) > Comment on attachment 8945420 [details] [diff] [review] When we've got 6 exceptions in that code to using the protocol flag we're clearly doing something wrong. Clearly the URI_IS_LOCAL_RESOURCE flag isn't as good a fit as we first thought. Maybe we need to add a special purpose URI_SOMETHING_CSP_SOMETHING flag and add that to the few places left. The specific PoC in this bug uses a copy of require.js that was not web-exposed in Firefox 52. Maybe we don't need to backport to ESR.

Christoph, what are your thoughts RE: the last 2 comments?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #82) > Christoph, what are your thoughts RE: the last 2 comments? I agree to both of the above 2 comments. First, I don't think we should backport this to ESR. Second, probably there is a better solution than relying on URI_IS_LOCAL_RESOURCE and we should revisit that decision eventually. For now, it seems like the right fix for this bug.

CSP bypass bugs should be capped to sec-moderate because (at this point, at least) it's a little-used mitigation that sites can opt in to. Even with the bypass the victim site must have a serious security bug itself.

(In reply to Daniel Veditz [:dveditz] from comment #84) > CSP bypass bugs should be capped to sec-moderate because (at this point, at > least) it's a little-used mitigation that sites can opt in to. Even with the > bypass the victim site must have a serious security bug itself. I'm not arguing against the rating, but there are scenarios around CSP sandbox, that don't involve the web page being vulnerable.

I have managed to reproduce this issue using Firefox 60.0a1(BuildId:20180122220231). This issue is verified fixed using Firefox 61.0a1(BuildId:20180503100146) and Firefox 60.0(BuildId:20180430165945) on Windows 10 64bit, macOS 10.13.3 and Ubuntu 16.04 64bit.