Cache Storage: restrict access to secure origins (Blink-side)

Cache Storage: restrict access to secure origins (Blink-side)

Per spec discussions[1], the Cache Storage API (i.e. window.caches)
should be restricted to secure origins. Reject access attempts with
SecurityError if made from non-secure origins

After this lands, https://codereview.chromium.org/1192003006/ will
go in to enforce things on the browser side.

[1] https://github.com/slightlyoff/ServiceWorker/issues/709

BUG=501380
R=falken@chromium.org,mkwst@chromium.org,asvitkine@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200191

[jsbell, 2015-06-18 19:06:59]

falken@, mkwst@ - please take a look?

I just cribbed this from what Service Worker is doing. There's a follow-up
Chromium CL that will terminate the renderer process if messages slip through.

[jsbell, 2015-06-18 19:11:08]

jsbell@chromium.org changed reviewers:
+ asvitkine@chromium.org

[jsbell, 2015-06-18 19:12:31]

This should land after the blink side CL:
https://codereview.chromium.org/1181973004

please take a look?

Note that I only added a single histogram entry rather than one per method,
given that the check/behavior is identical for each one. Is that okay?

[jsbell, 2015-06-18 19:13:48]

Whoops, sorry, this email was intended to be on the Chromium-side CL. Please
disregard.

On 2015/06/18 19:12:31, jsbell wrote:
> This should land after the blink side CL:
> https://codereview.chromium.org/1181973004
> 
> please take a look?
> 
> Note that I only added a single histogram entry rather than one per method,
> given that the check/behavior is identical for each one. Is that okay?

[jsbell, 2015-06-18 19:14:01]

jsbell@chromium.org changed reviewers:
- asvitkine@chromium.org

[jsbell, 2015-06-18 19:14:25]

Patchset #2 (id:20001) has been deleted

[Mike West, 2015-06-18 19:54:57]

LGTM % nit.

https://codereview.chromium.org/1177983007/diff/1/Source/modules/cachestorage...
File Source/modules/cachestorage/CacheStorage.cpp (right):

https://codereview.chromium.org/1177983007/diff/1/Source/modules/cachestorage...
Source/modules/cachestorage/CacheStorage.cpp:292: }
Nit: It might be reasonable to extract this repeated check out to something like
what's done in the "checkBoilerplate" bits of
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

[jsbell, 2015-06-18 21:31:37]

Hrm, inspector apparently will need tweaking as well. It's getting killed by the
chromium-side patch since it tries to excerise the API as well.

[jsbell, 2015-06-18 22:00:12]

On 2015/06/18 21:31:37, jsbell wrote:
> Hrm, inspector apparently will need tweaking as well. It's getting killed by
the
> chromium-side patch since it tries to excerise the API as well.

Interesting. Things like the New Tab Page pass the "isPrivilegedContext()" test
on the Blink side, but fail the "IsOriginSecure()" on the Chromium side. So
opening up the inspector on the NTP gets the renderer killed. mkwst@ - any
advice here would be helpful.

[jsbell, 2015-06-19 20:32:39]

jsbell@chromium.org changed reviewers:
+ palmer@chromium.org

[jsbell, 2015-06-19 20:32:40]

+palmer, for synchronizing Blink and Chromium-side checks

https://codereview.chromium.org/1177983007/diff/10003/Source/modules/cachesto...
File Source/modules/cachestorage/CacheStorage.cpp (right):

https://codereview.chromium.org/1177983007/diff/10003/Source/modules/cachesto...
Source/modules/cachestorage/CacheStorage.cpp:36: if
(!executionContext->isPrivilegedContext(errorMessage)) {
Per notes in https://codereview.chromium.org/1192003006/ should this further
call documentOrigin->isSecure() ?

https://codereview.chromium.org/1177983007/diff/10003/Source/modules/cachesto...
File Source/modules/cachestorage/InspectorCacheStorageAgent.cpp (right):

https://codereview.chromium.org/1177983007/diff/10003/Source/modules/cachesto...
Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:458: if
(!executionContext || !executionContext->isPrivilegedContext(*errorString))
This should call a public static on CacheStorage rather than duplicating the
permission check.

[palmer, 2015-06-26 21:13:17]

LGTM. Thanks!

https://codereview.chromium.org/1177983007/diff/10003/Source/modules/cachesto...
File Source/modules/cachestorage/CacheStorage.cpp (right):

https://codereview.chromium.org/1177983007/diff/10003/Source/modules/cachesto...
Source/modules/cachestorage/CacheStorage.cpp:36: if
(!executionContext->isPrivilegedContext(errorMessage)) {
> Per notes in https://codereview.chromium.org/1192003006/ should this further
> call documentOrigin->isSecure() ?

No, this is fine, and correct.

406 bool SecurityOrigin::isPotentiallyTrustworthy(String& errorMessage) const
407 {
408     ASSERT(m_protocol != "data");
409     if (SchemeRegistry::shouldTreatURLSchemeAsSecure(m_protocol) ||
isLocal() || isLocalhost())
410         return true;
411 
412     if (SecurityPolicy::isOriginWhiteListedTrustworthy(*this))
413         return true;
414 
415     errorMessage = "Only secure origins are allowed (see:
https://goo.gl/Y0ZkNV).";
416     return false;
417 }

[falken, 2015-06-29 03:27:18]

code lgtm, spec discussion looks stalled though

[jsbell, 2015-08-03 23:16:16]

jsbell@chromium.org changed reviewers:
+ pfeldman@chromium.org

[jsbell, 2015-08-03 23:16:16]

pfeldman@ - PTAL at the inspector changes?

[pfeldman, 2015-08-03 23:53:49]

https://codereview.chromium.org/1177983007/diff/90001/Source/modules/cachesto...
File Source/modules/cachestorage/InspectorCacheStorageAgent.cpp (right):

https://codereview.chromium.org/1177983007/diff/90001/Source/modules/cachesto...
Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:403: if
(!CacheStorage::canAccessCacheStorage(executionContext)) {
Inspector should have access to everything, but we might want to let user know
that the page does not have access to these.

https://codereview.chromium.org/1177983007/diff/90001/Source/modules/cachesto...
Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:456: for (Frame*
frame = m_page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
This approach is super-confusing, I was so happy it went away. Could you explain
what is happening here that we need to go via the blink core and execution
context? What are you trying to do here?

[jsbell, 2015-08-04 00:14:53]

https://codereview.chromium.org/1177983007/diff/90001/Source/modules/cachesto...
File Source/modules/cachestorage/InspectorCacheStorageAgent.cpp (right):

https://codereview.chromium.org/1177983007/diff/90001/Source/modules/cachesto...
Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:403: if
(!CacheStorage::canAccessCacheStorage(executionContext)) {
On 2015/08/03 23:53:49, pfeldman wrote:
> Inspector should have access to everything, but we might want to let user know
> that the page does not have access to these.

Implicitly, entries won't exist if the page doesn't have access since the page
can't create them. I'm not sure what you're thinking here.

https://codereview.chromium.org/1177983007/diff/90001/Source/modules/cachesto...
Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:456: for (Frame*
frame = m_page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
On 2015/08/03 23:53:48, pfeldman wrote:
> This approach is super-confusing, I was so happy it went away. Could you
explain
> what is happening here that we need to go via the blink core and execution
> context? What are you trying to do here?

Top level: don't try to and query CacheStorage if the origin isn't secure, else
the browser will nuke this process. (i.e. same thing the script-facing API does)

CacheStorage is calling ExecutionContext::isPrivilegedContext() which is (so far
as know) how we want to validate that the origin is permitted to use the API.
That does more than simply calling SecurityOrigin::isPotentiallyTrustworthy()
though - it walks the document ancestor chain.

But... given that we have a SecurityOrigin here anyway, perhaps we don't want to
try and mirror the exact check done by the document; calling
SecurityOrigin::isPotentiallyTrustworthy() should match the check done on the
Chromium side, which is sufficient to achieve the goal above.

palmer@?

(That would let us nuke most of the changes here, at the risk of having
different checks here vs. in CacheStorage, although the ones here would be a
strict subset.)

[palmer, 2015-08-04 00:41:00]

> > what is happening here that we need to go via the blink core and execution
> > context? What are you trying to do here?
> 
> Top level: don't try to and query CacheStorage if the origin isn't secure,
else
> the browser will nuke this process. (i.e. same thing the script-facing API
does)
> 
> CacheStorage is calling ExecutionContext::isPrivilegedContext() which is (so
far
> as know) how we want to validate that the origin is permitted to use the API.
> That does more than simply calling SecurityOrigin::isPotentiallyTrustworthy()
> though - it walks the document ancestor chain.
> 
> But... given that we have a SecurityOrigin here anyway, perhaps we don't want
to
> try and mirror the exact check done by the document; calling
> SecurityOrigin::isPotentiallyTrustworthy() should match the check done on the
> Chromium side, which is sufficient to achieve the goal above.

Walking the document means checking if the document is embedded in parent
documents (e.g. is an iframe)?

If so, it's still OK for an HTTPS iframe to use the cache even if the parent
document is HTTP, as far as I understand, because it can only use the cached
resources on its own behalf, and it only uses resources in its *own* cache,
right? It would never trust an HTTP parent document's cached, or other,
resources?

That would seem obviously true, but I ask to make sure I even know anything. :)

[jsbell, 2015-08-04 15:24:38]

On 2015/08/04 00:41:00, palmer wrote:
> Walking the document means checking if the document is embedded in parent
> documents (e.g. is an iframe)?

Code is:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

So far as I understand, it's checking to see that it's not (nor is an ancestor)
sandboxed.
 
> If so, it's still OK for an HTTPS iframe to use the cache even if the parent
> document is HTTP, as far as I understand, because it can only use the cached
> resources on its own behalf, and it only uses resources in its *own* cache,
> right?

It does look like that's being restricted. Perhaps that's *overly* restrictive?

> It would never trust an HTTP parent document's cached, or other,
> resources?

Correct.

[palmer, 2015-08-05 17:55:55]

> So far as I understand, it's checking to see that it's not (nor is an
ancestor)
> sandboxed.

OK, thanks.

> > If so, it's still OK for an HTTPS iframe to use the cache even if the parent
> > document is HTTP, as far as I understand, because it can only use the cached
> > resources on its own behalf, and it only uses resources in its *own* cache,
> > right?
> 
> It does look like that's being restricted. Perhaps that's *overly*
restrictive?

Perhaps. Let's leave it for now, and think about it.

> > It would never trust an HTTP parent document's cached, or other,
> > resources?
> 
> Correct.

SGTM, and still LGTM. Thanks!

[jsbell, 2015-08-07 18:08:12]

Apologies for the confusion; I should know better than to solicit feedback w/o
the code.

pfeldman@ - I think this (PS#6) is more what you'd like - no dependency on a
Page/ExecutionContext by the InspectorXXXAgent.

palmer@ - Ideally this (PS#6) is all we do from the inspector. Since the
inspector doesn't grant web content additional permissions, this change doesn't
weaken security for web content. It makes the inspector's renderer-side check
(before firing off requests) match the browser's check ("is this a potentially
trustworthy origin?"). 

Insofar as I understand the ExecutionContext::isPrivilegedContext() logic, the
worst case is that a sandboxed page in a trustworthy origin might (due to
sandboxing) not have permission to use the Cache Storage API, but DevTools would
still enumerate caches for that origin.

One more look, sorry & thanks folks!

[pfeldman, 2015-08-07 18:53:49]

devtools lgtm, thanks!

[palmer, 2015-08-07 19:01:59]

LGTM.

[jsbell, 2015-08-07 20:06:30]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2015-08-07 20:06:30]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1177983007/#ps110001
(title: "Simplify DevTools agent changes")

[commit-bot: I haz the power, 2015-08-07 20:07:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177983007/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1177983007/110001

[commit-bot: I haz the power, 2015-08-07 20:10:19]

Committed patchset #6 (id:110001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200191