1175138 - Cache API should reject in untrusted origins

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

It appears the spec is going to change to restrict Cache API to only trusted origins.

  https://github.com/slightlyoff/ServiceWorker/issues/709

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-secure

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I am planning to do a simple scheme check first and then we can add the more comprehensive settings-secure algorithm later.  This will let us prevent wholesale use on http sites while still shipping in the near future.  I think implementing settings-secure algorithm my be a bit of work for features running in a worker.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make the dom.caches.testing.enabled pref available in workers. r=ehsan

Expose our new testing preference to workers.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Enable dom.caches.testing.enabled in existing tests. r=ehsan

Add the preference to the necessary tests.  This turned out to be easy for the dom/cache tests because of the driver code.

For service workers, however, we have to add the preference everywhere since the sw script is stored in Cache API and we are using a lot of http service workers.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make CacheStorage reject on untrusted origins. r=ehsan

The simple solution for now which simply verifies URL scheme (if the testing pref is not set).

I still need to add a test that verifies that access is blocked on http, but I'm fairly certain that works based on figuring out where the pref needs to go.

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8626450 [details] [diff] [review]
P2 Enable dom.caches.testing.enabled in existing tests. r=ehsan

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

Oh, I also added dom.caches.enabled to all these tests since technically its necessary whenever a service worker script is saved now.  In practice this hasn't been a problem since SW is restricted to nightly where caches is also enabled.  Maybe this is a waste of time since I hope to remove dom.caches.enabled completely, but its done now.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8626451 [details] [diff] [review]
P3 Make CacheStorage reject on untrusted origins. r=ehsan

I realized I still need to add a localhost check as well.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8626450 [details] [diff] [review]
P2 Enable dom.caches.testing.enabled in existing tests. r=ehsan

Try shows there are more tests to fix.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I think I might just allow http if serviceWorker.testing is set as well.  Seems silly to force people to set two preferences.

Comment 10

[(no longer active)]

Comment on attachment 8626449 [details] [diff] [review]
P1 Make the dom.caches.testing.enabled pref available in workers. r=ehsan

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

::: dom/workers/RuntimeService.cpp
@@ +161,5 @@
>  #define PREF_WORKERS_LATEST_JS_VERSION "dom.workers.latestJSVersion"
>  #define PREF_INTL_ACCEPT_LANGUAGES     "intl.accept_languages"
>  #define PREF_SERVICEWORKERS_ENABLED    "dom.serviceWorkers.enabled"
>  #define PREF_INTERCEPTION_ENABLED      "dom.serviceWorkers.interception.enabled"
> +#define PREF_DOM_CACHES_TESTING_ENABLED "dom.caches.testing.enabled"

Nit: can you please put this next to PREF_DOM_CACHES_ENABLED?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1.5 Expose dom.serviceWorkers.testing.enabled to workers. r=ehsan

Expose dom.serviceWorkers.testing.enabled on workers as well.  This is easier than adding dom.caches.testing.enabled everywhere.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Enable dom.caches.testing.enabled in existing tests. r=ehsan

We shouldn't need all those new dom.cache.testing.enabled in dom/workers.

Note, we still need it in test_installation_simple.html because it sets and then unsets prefs which seems to confuse things.  I just added the cache pref to keep it working cleanly.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make CacheStorage reject on untrusted origins. r=ehsan

Updated to allow CacheStorage on localhost as well.

I still need to add a test case, but here's a try to verify the existing ones for now:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=82273e2e8a1e

Comment 14

[(no longer active)]

Comment on attachment 8626633 [details] [diff] [review]
P3 Make CacheStorage reject on untrusted origins. r=ehsan

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

r=me specifically with file:// URIs being handled.

::: dom/cache/CacheStorage.cpp
@@ +93,5 @@
> +  }
> +
> +  // Now parse the scheme of the principal's origin.  This is a short term
> +  // method for determining "trust".  In the long term we need to implement
> +  // the full algorithm here:

Can you please file a follow-up to fix this based on a logic similar to IsFromAuthenticatedOrigin() and include a FIXME pointing to the bug here?

@@ +115,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return false; }
> +
> +  nsAutoCString scheme(Substring(flatURL, schemePos, schemeLen));
> +  if (scheme.LowerCaseEqualsLiteral("https") ||
> +      scheme.LowerCaseEqualsLiteral("app")) {

You need to handle file here as well.

@@ +129,5 @@
> +                                 &hostPos, &hostLen,
> +                                 nullptr);                  // ignore port
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return false; }
> +
> +  nsCString hostname(url + authPos + hostPos, hostLen);

Nit: please use nsDependentCString which would be a tiny bit more efficient here.

@@ +133,5 @@
> +  nsCString hostname(url + authPos + hostPos, hostLen);
> +
> +  return hostname == NS_LITERAL_CSTRING("localhost") ||
> +         hostname == NS_LITERAL_CSTRING("127.0.0.1") ||
> +         hostname == NS_LITERAL_CSTRING("::1");

Nit: Please use .Equals() which would be a bit more efficient.

@@ +165,5 @@
> +  bool testingEnabled = false;
> +  Preferences::GetBool("dom.caches.testing.enabled", &testingEnabled);
> +  if (!testingEnabled) {
> +    Preferences::GetBool("dom.serviceWorkers.testing.enabled", &testingEnabled);
> +  }

Nit: Please rewrite similar to below:

  bool testingEnabled = Preferences::GetBool("dom.caches....") || Preferences::GetBool("dom.serviceWorkers....");

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Expose dom.serviceWorkers.testing.enabled to workers. r=ehsan

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Expose the devtools SW testing flag on workers. r=ehsan

For sanity CacheStorage should bypass the trusted origin checks when devtools ServiceWorker testing is enabled.  As a first step, expose this value on the worker private.

This is kind of busted on SharedWorker like everything else right now when there are multiple documents.  I added bug 1177935 to fix that.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Enable dom.caches.testing.enabled in existing tests. r=ehsan

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make CacheStorage reject on untrusted origins. r=ehsan

Update to check the devtools ServiceWorker testing flag as well.

This flag is only checked for the window and worker .caches global.  The ScriptLoader and ServiceWorkerScriptCache always bypass the trusted origin checks since ServiceWorker does its own checks at register.  This is my solution for the devtools flag not being available on the SW registration object.

I'm sorry this was not broken out as an interdiff, but I did a qref at the wrong time today.  I figured the origin patch was small, so hopefully re-reviewing should not be a problem.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Add a simple test to verify CacheStorage rejects in http origin. r=ehsan

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08d38df3c78d

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8555bee87b61
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2aaa299a7b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4a0a690041
https://hg.mozilla.org/integration/mozilla-inbound/rev/86b90d49810a
https://hg.mozilla.org/integration/mozilla-inbound/rev/08dee0e2fa45
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ca702e4ef1b

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8555bee87b61
https://hg.mozilla.org/mozilla-central/rev/c2aaa299a7b2
https://hg.mozilla.org/mozilla-central/rev/4e4a0a690041
https://hg.mozilla.org/mozilla-central/rev/86b90d49810a
https://hg.mozilla.org/mozilla-central/rev/08dee0e2fa45
https://hg.mozilla.org/mozilla-central/rev/8ca702e4ef1b

Comment 23

[Chris Mills [:cmills]]

I've added a note to cover this near the top of

https://developer.mozilla.org/en-US/docs/Web/API/CacheStorage

I am not sure if anything else is really needed here?

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Chris, do you think its worth mentioning the "Enable Service Workers over HTTP (when toolbox is open)" option in devtools allows CacheStorage to work on http?

Also, in the long term, "trusted" will be more complicated than just "https".  It will depend on the parent of the document being trusted as well.  I imagine we will want a page describing "trusted" when we fully implement that.

As always, thanks for the excellent docs!

Comment 25

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #24)
> Also, in the long term, "trusted" will be more complicated than just
> "https".  It will depend on the parent of the document being trusted as
> well.  I imagine we will want a page describing "trusted" when we fully
> implement that.

Yes, that would be wonderful to have!

Comment 26

[Chris Mills [:cmills]]

(In reply to Ben Kelly [:bkelly] from comment #24)
> Chris, do you think its worth mentioning the "Enable Service Workers over
> HTTP (when toolbox is open)" option in devtools allows CacheStorage to work
> on http?

Definitely! I've added to the note, to cover this. I also added a note about it at:

https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API/Using_Service_Workers#Dev_tools

Do we have any other devtools features worth mentioning for SWs now? This bit is a little out of date.

> 
> Also, in the long term, "trusted" will be more complicated than just
> "https".  It will depend on the parent of the document being trusted as
> well.  I imagine we will want a page describing "trusted" when we fully
> implement that.

Sounds good to me. Let's keep this in mind. I'll file a developer docs bug about it.
 
> As always, thanks for the excellent docs!

You're most welcome.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #26)
> Do we have any other devtools features worth mentioning for SWs now? This
> bit is a little out of date.

Its not in devtools itself, but I think this section could mention about:serviceworkers.  This lets you see what SWs are registered and then update/remove them.

Thanks again.

Comment 28

[Chris Mills [:cmills]]

(In reply to Ben Kelly [:bkelly] from comment #27)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #26)
> > Do we have any other devtools features worth mentioning for SWs now? This
> > bit is a little out of date.
> 
> Its not in devtools itself, but I think this section could mention
> about:serviceworkers.  This lets you see what SWs are registered and then
> update/remove them.

Great - added!