Given any of these policies: default-src http://elsewhere.com; manifest-src http://example.org default-src 'self'; manifest-src http://test:80 Expected to be able to load content from either example.com or test:80. However, in both cases, CSP doesn't seem to allow overriding default-src.

Patch includes only the tests. I'll poke around the CSP parser tomorrow to see what is going on, but any suggestions on how to fix would be welcome.

(In reply to Marcos Caceres [:marcosc] from comment #0) > Given any of these policies: > > default-src http://elsewhere.com; manifest-src http://example.org > default-src 'self'; manifest-src http://test:80 > > Expected to be able to load content from either example.com or test:80. > However, in both cases, CSP doesn't seem to allow overriding default-src. I don't fully understand what you mean. Looking at the first policy: > default-src http://elsewhere.com; manifest-src http://example.org that should allow to load web manifests only from example.org Hey Marcos, what happens exactly? Loading a web-manifest from example.org gets blocked? Or loading a web manifest from something other than example.org is allowed? Once I know, I can provide more guidance and help. Thanks for looking into this problem. Also, always have a look what the Web Console spits out under the [Security] Panel, that already might give us a hint where the problem might be buried.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2) > (In reply to Marcos Caceres [:marcosc] from comment #0) > > Given any of these policies: > > > > default-src http://elsewhere.com; manifest-src http://example.org > > default-src 'self'; manifest-src http://test:80 > > > > Expected to be able to load content from either example.com or test:80. > > However, in both cases, CSP doesn't seem to allow overriding default-src. > > I don't fully understand what you mean. Looking at the first policy: > > default-src http://elsewhere.com; manifest-src http://example.org > that should allow to load web manifests only from example.org > > Hey Marcos, what happens exactly? > Loading a web-manifest from example.org gets blocked? Correct. > Or loading a web manifest from something other than > example.org is allowed? Once I know, I can provide more guidance and help. No. It seems at if "manifest-src" has no effect when "default-src" is declared. I.e., "manifest-src" should override "default-src", allowing manifest to be loaded from a given URL (in case 1, example.org; in case 2, test:80). > Thanks for looking into this problem. Also, always have a look what the Web > Console spits out under the [Security] Panel, that already might give us a > hint where the problem might be buried. I can't remember how to get `mach test` to not quit once tests are done (or pop up the js debugger). Any idea?

Hey Marcos, I applied your patch real quick and had a look at what's going on. It seems that CSP manifest-src is working correctly, but since you are using fetch, you also have to specify a connect-src directive [1], otherwise fetch will use the default-src directive and block the load, see important arguments at 'doContentSecurityCheck' underneath. Anyway, what you have to do is, add > connect-src http://example.org to your CSP to make the test work from a CSP perspective. Please note that the test is still failing at the moment, because of: > Expected promise rejection obtaining http://example.org ... but I suppose you know how to get that fixed. [1] https://w3c.github.io/webappsec-csp/#directive-connect-src doContentSecurityCheck { channelURI: http://example.org/tests/dom/security/test/csp/file_web_manifest.json loadingPrincipal: http://example.org/tests/dom/security/test/csp/file_testserver.sjs?cors=*&csp=default-src%20http://elsewhere.com;%20manifest-src%20http://example.org&file=/tests/dom/security/test/csp/file_web_manifest.html triggeringPrincipal: http://example.org/tests/dom/security/test/csp/file_testserver.sjs?cors=*&csp=default-src%20http://elsewhere.com;%20manifest-src%20http://example.org&file=/tests/dom/security/test/csp/file_web_manifest.html contentPolicyType: 20 securityMode: 16 initalSecurityChecksDone: no enforceSecurity: no }

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4) > Hey Marcos, I applied your patch real quick and had a look at what's going > on. It seems that CSP manifest-src is working correctly, but since you are > using fetch, you also have to specify a connect-src directive [1], otherwise > fetch will use the default-src directive and block the load, see important > arguments at 'doContentSecurityCheck' underneath. Anyway, what you have to > do is, add > > connect-src http://example.org > > to your CSP to make the test work from a CSP perspective. d'oh, ok. I need your advice here: the use of fetch() in ManifestObtainer.jsm is, um, "privileged"? (sorry, can't think of a better word). That is to say, it is doing the implementation work that is in the W3C Web Manifest spec to fetch that particular linked resource. As such, it needs to behave like any "<link>" element, and should not require the developer to put in a "connect-src" (i.e., why we added manifest-src). Does that make sense? Or am I thinking about this all wrong?

Marcos, the problem is within fetch/InternalRequest.cpp, where GetRequestConstructorCopy uses a hardcoded TYPE_FETCH instead of mContentPolicyType. Further, I think we don't need canLoadManifest() at all, because we call fetch() which internally calls asyncOpen2() [1] which internally performs security checks within the nsContentSecurityManager() which is called from within asyncOpen2(). I suppose ManifestObtainer.jsm would need a little update to make that work. What do you suggest on how we should fix that? Once we changed ManifestObtainer.jsm we then need to slightly modify the test. Other than that I think the patch is what we want. Note, if I keep canLoadManifest, then the test succeeds. But the whole point of using asyncOpen2() is to remove contentPoicy checks on the callsite. [1] http://mxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#369

Comment on attachment 8731537 [details] [diff] [review] bug_1250048_update_csp_manifest_src.patch Review of attachment 8731537 [details] [diff] [review]: ----------------------------------------------------------------- Oh, this is awesome! We just let fetch automatically reject then. I'll need to fix up some of the tests, but this makes it so much better.

Comment on attachment 8731537 [details] [diff] [review] bug_1250048_update_csp_manifest_src.patch Review of attachment 8731537 [details] [diff] [review]: ----------------------------------------------------------------- Oh, this is awesome! We just let fetch automatically reject then. I'll need to fix up some of the tests, but this makes it so much better.

It works! :D I've updated the tests and the ManifestObtainer.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf2b81ebf014

(In reply to Marcos Caceres [:marcosc] from comment #9) > Created attachment 8731560 [details] [diff] [review] > 0001-Bug-1250048-CSP-manifest-src-doesn-t-override-defaul.patch > > It works! :D I've updated the tests and the ManifestObtainer. Of course it does :-) AsyncOpen2() within fetch obviously also checks for mixed content, so I suppose you have to adapt that test as well: dom/security/test/csp/browser_test_web_manifest_mixed_content.js Once that is fixed, please flag me for review again and we have that landed in no time. Already looked at the patch. Looks all fine to me.

Comment on attachment 8731560 [details] [diff] [review] 0001-Bug-1250048-CSP-manifest-src-doesn-t-override-defaul.patch Clearing for now, see previous comment.

Note, this patch includes the changes from bug 1176824 - which should be landing in the next few hours (so the diff will look larger than it is). Will rebase it once it lands. Having said that, I did refactor the tests as they were some silly things in there. Also made them conform to JSCS (changes are mostly cosmetic).... code quality and all that.

Comment on attachment 8732014 [details] [diff] [review] 0001-Bug-1250048-CSP-manifest-src-doesn-t-override-defaul.patch Review of attachment 8732014 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, r=me.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=431401a093b4

Re-based. Carrying over r+...

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b8360b8b360f for various CSP failures like: https://treeherder.mozilla.org/logviewer.html#?job_id=24265261&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=24262801&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=24262869&repo=mozilla-inbound

Thanks! will investigate.

Trying again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96e3c780ae9f

The "ImportError: No module named pygtk" error seems to be unrelated, as it's some Python package AFAICT.

Applied small change from Bug 1258588, carrying over r+. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81acf423494d

Gah... I haven't been able to reproduce the fails from comment 18. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a8feb8500d8 Let's see how the try run from comment 22 works out.

I seem to be hitting: Assertion failure: false (contentPolicyType not supported yet), at nsContentSecurityManager.cpp:159 Christoph, any ideas?

(In reply to Marcos Caceres [:marcosc] from comment #24) > I seem to be hitting: > Assertion failure: false (contentPolicyType not supported yet), at > nsContentSecurityManager.cpp:159 > > Christoph, any ideas? Odd, what is the contentPolicyType that is not supported? Just do a fprintf("%d : %d", internalContentPolicyType, contentPolicyType); I don't have a build right now, I can check later.

See if I can get it to trigger that assertion.

*I will see

Argh, so I thought I had landed Bug 1257747... but I had forgot to check it in. I'm just going to bundle that as part of this one, as you've already reviewed and r+ it. All this now depends on that anyway.

Patch that includes the updated test server file, which should hopefully fix the issues.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c2c24de6fc3

Comment on attachment 8733673 [details] [diff] [review] 0001-Bug-1250048-CSP-manifest-src-doesn-t-override-defaul.patch Review of attachment 8733673 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/fetch/InternalRequest.cpp @@ +39,4 @@ > // The default referrer is already about:client. > copy->mReferrerPolicy = mReferrerPolicy; > > + copy->mContentPolicyType = mContentPolicyType; I'm not sure this is correct. Our nsContentPolicyType maps to a combination of RequestDestination and RequestType in the spec. The GetRequestConstructorCopy() method is implementing step 8 from the spec here: https://fetch.spec.whatwg.org/#dom-request I don't see anything there which copies Request.destination or Request.type from the original Request.

Actually, I think the spec may be wrong. Lets ask Anne: https://github.com/whatwg/fetch/issues/262

Anne tells me in IRC that the current spec is intentional and the destinion/type should not be copied. Therefore copying the nsContentPolicyType is incorrect in the code in comment 31.

Looking at the original issue here, I think you may need to use something other than vanilla fetch(). We can break DOM exposed fetch() security in order to enable a chrome-only use case. Can you use nsIChannel directly in your ManifestObtainer.jsm? Alternatively, we could create some kind of chrome-only interface on Request or fetch(), but I'm not sure what that would look like. I'm open to suggestion.

(In reply to Ben Kelly [:bkelly] from comment #34) > Looking at the original issue here, I think you may need to use something > other than vanilla fetch(). We can break DOM exposed fetch() security in > order to enable a chrome-only use case. We had already done this by allowing the context of the request to be set by chrome code. See Request.webidl, which includes: // Bug 1124638 - Allow chrome callers to set the context. [ChromeOnly] void setContentPolicyType(nsContentPolicyType context); > Can you use nsIChannel directly in your ManifestObtainer.jsm? Only if behaves exactly the same as fetch() in that it would need exhibit the same security properties, follow redirects, block on mixed content, etc. Otherwise, it would mean replicating some or all of the security assurances given by using fetch(), which would be more code we would need to keep secure. That's not great, IMO. > Alternatively, we could create some kind of chrome-only interface on Request > or fetch(), but I'm not sure what that would look like. I'm open to > suggestion. See above. Is that sufficient?

Ok, then I think you can have the chrome-only setContentPolicyType() method also set a flag. When that flag is set the GetRequestConstructorCopy() can copy the nsContentPolicy. In all other cases, though, the exist FETCH policy should be used. There should probably be some documentation on setContentPolicyType() indicating it changes fetch() behavior as well. We want to make sure internal code does not call that method when setting up a normal FetchEvent.request, for example. Any assert you can think of to ensure that would be great.

(In reply to Ben Kelly [:bkelly] from comment #36) > Ok, then I think you can have the chrome-only setContentPolicyType() method > also set a flag. When that flag is set the GetRequestConstructorCopy() can > copy the nsContentPolicy. In all other cases, though, the exist FETCH > policy should be used. My recommendation would then be that change the IDL to: void setContentPolicyType(nsContentPolicyType context, optional PolicyControlOptions options); And dictionary PolicyControlOptions { boolean copyContentPolicy = false; }; > There should probably be some documentation on setContentPolicyType() > indicating it changes fetch() behavior as well. We want to make sure > internal code does not call that method when setting up a normal > FetchEvent.request, for example. Any assert you can think of to ensure that > would be great. Although it looks like a simple change, I don't know enough C++ to implement the above (well, not enough that I won't lose a week trying to do it, then probably get it wrong... while it would probably take someone else 5 minutes). Can someone that knows C++ help me out?

I think the right behavior is to preserve the content policy type set through setContentPolicyType() across copies of the Request object. We should probably also rename that method to overrideContentPolicyType() to make it clearer what it does. I can't think of a case where we would want to overwrite the content policy type to TYPE_FETCH upon copying if it has been overridden by some code, so the extension suggested in comment 37 doesn't seem needed. Let me write a patch for that part.

Marcos, can you give this a shot, please? Note that you should revert your change to dom/fetch/InternalRequest.cpp before applying this.

Comment on attachment 8734470 [details] [diff] [review] Part 1: Pass the correct content policy type down to Necko if Request.overrideContentPolicyType() has been called; r=bkelly f=marcosc Review of attachment 8734470 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

(In reply to :Ehsan Akhgari from comment #39) > Marcos, can you give this a shot, please? Note that you should revert your > change to dom/fetch/InternalRequest.cpp before applying this. Will give it a shot on Tuesday, once I'm back at work.

Comment on attachment 8734470 [details] [diff] [review] Part 1: Pass the correct content policy type down to Necko if Request.overrideContentPolicyType() has been called; r=bkelly f=marcosc (In reply to Marcos Caceres [:marcosc] from comment #41) > (In reply to :Ehsan Akhgari from comment #39) > > Marcos, can you give this a shot, please? Note that you should revert your > > change to dom/fetch/InternalRequest.cpp before applying this. > > Will give it a shot on Tuesday, once I'm back at work. OK, so resetting the feedback flag again. Please + it if the patch works. :-)

Ok, I've applied Ehsan's changes to previous patch. All tests pass (yay!). Carrying over request for review from Ehsan re: c++ bits. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9205557a7e96

Comment on attachment 8736676 [details] [diff] [review] 0001-Bug-1250048-CSP-manifest-src-doesn-t-override-defaul.patch Review of attachment 8736676 [details] [diff] [review]: ----------------------------------------------------------------- r=me. I only reviewed the c++ parts related to fetch. I believe Christoph reviewed the other bits previously, right? ::: dom/fetch/InternalRequest.h @@ +492,5 @@ > bool mCreatedByFetchEvent = false; > + // This is only set when Request.overrideContentPolicyType() has been set. > + // It is illegal to pass such a Request object to a fetch() method unless > + // if the caller has chrome privileges. > + bool mContentPolicyTypeOverridden = false; Ugh, I hate this style since it further confuses where things are initialized. But ok.

Ehsan, I'm hitting the assertion check in try: Assertion failure: nsContentUtils::IsCallerChrome(), at /builds/slave/try-lx-d-000000000000000000000/build/src/dom/fetch/Request.cpp:289

Do you have a full stack for the assert?

Assertion failure: nsContentUtils::IsCallerChrome(), at /Users/marcos/gecko/dom/fetch/Request.cpp:289 #01: mozilla::dom::Request::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::RequestOrUSVString const&, mozilla::dom::RequestInit const&, mozilla::ErrorResult&) (Request.cpp:288, in XUL) #02: mozilla::dom::FetchRequest(nsIGlobalObject*, mozilla::dom::RequestOrUSVString const&, mozilla::dom::RequestInit const&, mozilla::ErrorResult&) (Fetch.cpp:164, in XUL) #03: nsGlobalWindow::Fetch(mozilla::dom::RequestOrUSVString const&, mozilla::dom::RequestInit const&, mozilla::ErrorResult&) (nsGlobalWindow.cpp:6587, in XUL) #04: mozilla::dom::WindowBinding::fetch(JSContext*, JS::Handle<JSObject*>, nsGlobalWindow*, JSJitMethodCallArgs const&) (WindowBinding.cpp:11648, in XUL) #05: mozilla::dom::WindowBinding::fetch_promiseWrapper(JSContext*, JS::Handle<JSObject*>, nsGlobalWindow*, JSJitMethodCallArgs const&) (WindowBinding.cpp:11666, in XUL) #06: mozilla::dom::WindowBinding::genericPromiseReturningMethod(JSContext*, unsigned int, JS::Value*) (WindowBinding.cpp:13805, in XUL) #07: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:235, in XUL) #08: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:476, in XUL) #09: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2807, in XUL) #10: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:426, in XUL) #11: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:494, in XUL) #12: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:528, in XUL) #13: js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const (DirectProxyHandler.cpp:77, in XUL) #14: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const (CrossCompartmentWrapper.cpp:291, in XUL) #15: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (Proxy.cpp:390, in XUL) #16: js::proxy_Call(JSContext*, unsigned int, JS::Value*) (Proxy.cpp:682, in XUL) #17: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:235, in XUL) #18: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:464, in XUL) #19: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (BaselineIC.cpp:6113, in XUL)

Found a small issue in ManifestObserver.jsm in the prev patch, but still hitting the same thing. I guess I'm constructing the Request incorrectly or something?

s/ManifestObserver/ManifestObtainer/

Marcos, is there a reason you have to use aWindow.fetch() instead of just fetch()? I think that switches the subject principal to the content script which triggers the assertion. We added the assertion to make sure that content script never accidentally gets hold of one of these overriden Requests and uses it directly.

Ah, I though you had to create it from the content to get the right security privileges. Will try without it.

Please reset the needinfo flag if comment 51 doesn't go anywhere. Thanks!

(In reply to Marcos Caceres [:marcosc] from comment #51) > Ah, I though you had to create it from the content to get the right security > privileges. > Will try without it. Ok, so, I tried a few different things without success. ## Attempt 1: Attempt to import fetch and Request ```JS Cu.importGlobalProperties(["fetch", "Request"]); ``` Fails because "Request" can't be imported into JSMs: [JavaScript Error: "Error: Unknown property name: Request" {file: "resource://gre/modules/ManifestObtainer.jsm" line: 33}] ## Attempt 2: Import just `fetch()` This seems wrong, because I still have to create the Request from the content object: ```JS const request = new window.Request(manifestURL, reqInit); request.overrideContentPolicyType(Ci.nsIContentPolicy.TYPE_WEB_MANIFEST); ``` This fails, because it doesn't respect the content's CSP policy (default-src 'none'), which should block fetching. I get: 4 INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/browser_test_web_manifest.js | default-src 'none' blocks fetching manifest. - Got [object Object], expected csp-on-violate-policy Where "[object Object]" is the manifest, as confirmed by inspecting the object. ## What I really want... I think? It seems to me what we actually need is importGlobalProperties(["Request"]) or some way to import Request() into ManifestObtainer.jsm. Then I need to be able to call aWindow.fetch() but with a Chrome constructed Request. Does that sound right? Otherwise, I need to somehow get the content's CSP policy and combine it with the globally imported fetch(). That feels wrong to me, but as always [1]. [1] https://img.buzzfeed.com/buzzfeed-static/static/2014-10/26/6/enhanced/webdr08/enhanced-14836-1414320930-8.jpg

Boris, can you help me figure out the right fix here? Marcos has added a chrome-only webidl method on the Request interface. He is calling this from his jsm like this: // where aWindow is a content window var request = new aWindow.Request(args); request.myChromeOnlyMethod(); This works fine, but we want to ensure that the resulting Request object is only ever used from chrome code. So we have an assertion like this in the fetch() path: MOZ_ASSERT(nsContentUtils::IsCallerChrome()) Which then blows up because it checks the subject principal when Marcos calls fetch() like this: aWindow.fetch(request); Is there some way we can check that its chrome code calling fetch() even though its using the window's subject principal? Is that even correct to do? Or should we just give up on this assertion? Thanks!

Um... The subject principal when doing: aWindow.fetch(request); should be chrome, if the caller there is chrome and aWindow is an Xray for a content-side window, no? > even though its using the window's subject principal? Why is it doing that? What does the C++ callstack to the failing assertion look like?

(In reply to Boris Zbarsky [:bz] from comment #55) > Why is it doing that? What does the C++ callstack to the failing assertion > look like? I'm not sure. See comment 47.

Maybe because we are calling into the Request::Constructor() from c++ inside the fetch() implementation?

> Maybe because we are calling into the Request::Constructor() from c++ inside the fetch() implementation? Ah, stack is in comment 47. The assert is inside Request::Constructor, not fetch(); that matters! Typically we enter the target compartment before calling Foo::Constructor, for various reasons. And the fetch code does that for its manual call as well: see the "jsapi.Init(aGlobal)" bit in FetchRequest, which makes aGlobal's compartment the current compartment. What do you want to happen if content code does |new Request(yourHackedUpRequest)|? Or can that not happen for some reason?

We're mainly interested in asserting in the fetch() case. I'll see if I can make a patch.

Marcos, does this fix the assertion problem for you? Note, I tried running your tests locally but couldn't get any of them to pass; before or after this patch. Do I need some other dependent bug applied for that to work?

Comment on attachment 8738837 [details] [diff] [review] bug1250048_assertfix.patch Review of attachment 8738837 [details] [diff] [review]: ----------------------------------------------------------------- Applied patch and works great. The reason the tests were failing seems to be that (maybe?) mochitests changed recently to require explicit symlinking of the support files (in browser.ini).

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a168ab8eb147

Ben, not sure if you wanted to get one more round of review or not. Let me know how to proceed.

(In reply to Marcos Caceres [:marcosc] from comment #63) > Ben, not sure if you wanted to get one more round of review or not. Let me > know how to proceed. Feel free just to fold my fix into the previously reviewed file. Thanks!

All folded in, so carrying over r+.