Attaching a proof of concept patch that service worker registration does not consult CSP of a page. We are missing a call to NS_CheckContentLoadPolicy somewhere.

I wrote a spec issue, but this is actually already spec'd. The SW Update algorithm should be running the equivalent of Fetch when downloading the sw script. Fetch includes the CSP check.

I fixed up the tests so that they pass.

This passes the test, but doesn't properly handle redirects. Christoph and I discussed another approach that would do that (coming up).

This is incomplete because we don't have a ref to the document. Christoph is going to finish this part up early next week.

Jonas, Ben, I think it's the best solution if we just have service workers use asyncOpen2(). If so, then I think there is no need in having the additional check within > dom/workers/ServiceWorkerManager.cpp which would result in consulting CSP twice for the same load. Kate created CSP tests for service workers including redirects (see Bug 1045891) which work fine with that approach. If you are fine with that patch I can have dveditz review the bits within dom/security where we needed to add a fallback mechanism to use the handed in principal in case the loadingDocument is null (which is the case for service workers). Thoughts?

AsyncOpen2 is generally about network policies, so semantically it's not a great fit here. However, I can't see anything that would cause it not to work here, so it's up to you, as the CSP-owner, and the SW-owners.

Comment on attachment 8671586 [details] [diff] [review] bug_1208559_serviceworker_csp_checks.patch Review of attachment 8671586 [details] [diff] [review]: ----------------------------------------------------------------- r=me, although I think we should consider the case where ScriptLoader loads the service worker script instead of ServiceWorkerScriptCache. Also, I don't really understand Jonas's comment 7. These are doing network loads... ::: dom/security/nsContentSecurityManager.cpp @@ +124,5 @@ > break; > } > > + case nsIContentPolicy::TYPE_SCRIPT: { > + mimeTypeGuess = NS_LITERAL_CSTRING("application/javascript"); I'm not sure how this guess is used, but service workers are required to be one of: text/javascript application/x-javascript application/javascript We check for this in service worker code explicitly. ::: dom/workers/ServiceWorkerScriptCache.cpp @@ +111,5 @@ > } > > // Note that because there is no "serviceworker" RequestContext type, we can > // use the TYPE_INTERNAL_SCRIPT content policy types when loading a service > // worker. This comment seems wrong now. @@ +115,5 @@ > // worker. > rv = NS_NewChannel(getter_AddRefs(mChannel), > uri, aPrincipal, > + nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED, > + nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER, In theory we should ensure these values are set in ScriptLoader.cpp as well, right? The ScriptLoader will attempt a network download of the service worker script if the Cache does not have it. That can happen due to storage eviction, etc. See: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?case=true&from=ScriptLoader.cpp#1500 https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#863 https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#168 Is that case covered somehow? To be honest, it would probably be hard to trigger with a test. We could also just make this trigger a load error if the top level service worker script is not already in the Cache: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#1498 Its kind of corner casey, so I guess it could wait until ScriptLoader is converted to AsyncOpen2() as well.

Comment on attachment 8671586 [details] [diff] [review] bug_1208559_serviceworker_csp_checks.patch (In reply to Ben Kelly [:bkelly] from comment #8) > r=me, although I think we should consider the case where ScriptLoader loads > the service worker script instead of ServiceWorkerScriptCache. With ScriptLoader you mean dom/workers/ScriptLoader, right? Within dom/workers/ScriptLoader we perform a CSP check anyway, so that should be fine. > > + case nsIContentPolicy::TYPE_SCRIPT: { > > + mimeTypeGuess = NS_LITERAL_CSTRING("application/javascript"); > > I'm not sure how this guess is used, but service workers are required to be > one of: > > text/javascript > application/x-javascript > application/javascript > > We check for this in service worker code explicitly. I already had that discussion with Jonas a while ago. I think I remember that we think it's fine to just use "applicaton/javascript", but then again, I am also not completely sure how important the mime type guess is. We don't consume it internally, but addons rely on it in some way. Dan probably can tell us whether we can just use "application/javascript" for all kinds of TYPE_SCRIPT or not. He has to review the patch anyway, since we need a dom/security peer to review the CSP changes. > @@ +115,5 @@ > > // worker. > > rv = NS_NewChannel(getter_AddRefs(mChannel), > > uri, aPrincipal, > > + nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED, > > + nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER, > > In theory we should ensure these values are set in ScriptLoader.cpp as well, > right? Yes, you are right, we are going to do that within Bug 1206955, but that's fine for now. As long as we haven't converted the callsite to use asyncOpen2() we perform security checks on the callsite. But soon we should be able to convert that callsite to use AsyncOpen2() anyway. Definitely safe for now. > The ScriptLoader will attempt a network download of the service worker > script if the Cache does not have it. That can happen due to storage > eviction, etc. See: > ... > Is that case covered somehow? To be honest, it would probably be hard to > trigger with a test. So, that's another service worker specific question. When registering a service worker I assume it's fine to perform a CSP check only if the script code is fetched from the network. I think this patch should cover everything we need, but then again I could be convinced otherwise. Let's wait for Dan's input.

> Also, I don't really understand Jonas's comment 7. These are doing network loads... What I'm saying is that AsyncOpen2 generally protects against three things: * That website A can't read sensitive personal data from website B. (For example, read the users login credentials for website B) * That website A can't create requests which to website B which would cause B to take inappropriate actions with user data that website B holds. (For example transfer the user's money to an attacker-controlled account.) * That website A can't be tricked into sending data to an untrusted party or over an insecure connection. (Trick website A into sending the users credentials for website A to an attacker controlled server). Limiting the scope of a SW doesn't seem to fit into any of these categories.

Comment on attachment 8671586 [details] [diff] [review] bug_1208559_serviceworker_csp_checks.patch Review of attachment 8671586 [details] [diff] [review]: ----------------------------------------------------------------- r=dveditz

Jonas, did you mean to r+ Christoph's patch here? ;)

As discussed with bkelly, we should also land the CSP check within the service worker registration code. Taking bholly's patch but using TYPE_INTERNAL_SERVICE_WORKER as the contentType. Ben also filed: https://github.com/slightlyoff/ServiceWorker/issues/755#issuecomment-148142978

Comment on attachment 8673842 [details] [diff] [review] Bug-1208559---Do-a-CSP-Check-in-ServiceWorkerManag.patch Ben, as agreed, can you green light that patch?

Comment on attachment 8671586 [details] [diff] [review] bug_1208559_serviceworker_csp_checks.patch Chatted with Jonas over IRC; he is fine with other people reviewing those bits. Clearing his r? and ni? flags.

I don't think this needs to be hidden (or require sec-approval to land) because ServiceWorkers aren't enabled yet and it's already known we are incomplete in CSP wrt governing workers in general

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e497ed4a5c https://hg.mozilla.org/integration/mozilla-inbound/rev/dd458f2077e8 https://hg.mozilla.org/integration/mozilla-inbound/rev/eca8be0e5336

https://hg.mozilla.org/mozilla-central/rev/e2e497ed4a5c https://hg.mozilla.org/mozilla-central/rev/dd458f2077e8 https://hg.mozilla.org/mozilla-central/rev/eca8be0e5336

https://hg.mozilla.org/mozilla-central/rev/e2e497ed4a5c https://hg.mozilla.org/mozilla-central/rev/dd458f2077e8 https://hg.mozilla.org/mozilla-central/rev/eca8be0e5336

Comment on attachment 8671586 [details] [diff] [review] bug_1208559_serviceworker_csp_checks.patch Approval Request Comment [Feature/regressing bug #]: Similar to Bug 1198078, we have had this bug for a while. [User impact if declined]: ServiceWorker registration bypasses CSP of a page because ContentPolicy checks were missing. [Describe test coverage new/current, TreeHerder]: Has a test and has been on try server. [Risks and why]: Low, because only pages using service workers and shipping with a CSP are affected. [String/UUID change made/needed]: No.

Note, service workers are not enabled by default on beta. Also, we have decided not to ship service workers in 43. So not sure if this really needs to be uplifted.

Just rebased the patches for aurora.

Just rebased the patches for aurora.

Just rebased the patches for aurora.

Comment on attachment 8676335 [details] [diff] [review] Bug-1208559---Tests-rbholley.patch See comment 20 (once approved and landed on aurora I can rebase them for beta).

Comment on attachment 8676336 [details] [diff] [review] bug_1208559_serviceworker_csp_checks.patch See comment 20 (once approved and landed on aurora I can rebase them for beta).

Comment on attachment 8676337 [details] [diff] [review] Bug-1208559---Do-a-CSP-Check-in-ServiceWorkerManag.patch See comment 20 (once approved and landed on aurora I can rebase them for beta).

Christoph, do we really need this on aurora/beta? It is more or less too late for beta 42 at this point unless this is very critical. Since Service Workers isn't shipping in 43, do we need this to uplift to 43?

Comment on attachment 8676335 [details] [diff] [review] Bug-1208559---Tests-rbholley.patch Clearing uplift request as Service workers are not going to be shipped in FF43.

Comment on attachment 8676336 [details] [diff] [review] bug_1208559_serviceworker_csp_checks.patch Clearing uplift request as Service workers are not going to be shipped in FF43.

Comment on attachment 8676337 [details] [diff] [review] Bug-1208559---Do-a-CSP-Check-in-ServiceWorkerManag.patch Clearing uplift request as Service workers are not going to be shipped in FF43.