Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1192003006:
Cache Storage: restrict access to secure origins (Chromium-side) (Closed)
|
Created:
5 years, 6 months ago by jsbell Modified:
5 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jkarlin+watch_chromium.org, nhiroki, dmurph, pfeldman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCache Storage: restrict access to secure origins (Chromium-side)
Follow-up to https://codereview.chromium.org/1177983007/ to terminate
renderers that send bogus cache storage access requests.
BUG=501380
R=falken@chromium.org,mkwst@chromium.org,asvitkine@chromium.org,
Committed: https://crrev.com/4c4749245f912e6e87950508b34483f2d80deba8
Cr-Commit-Position: refs/heads/master@{#342657}
Patch Set 1 #Patch Set 2 : Updated histograms #
Total comments: 4
Patch Set 3 : Review feedback #Patch Set 4 : Actually try building #
Total comments: 2
Patch Set 5 : Add chrome-search scheme to secure scheme list #
Messages
Total messages: 34 (12 generated)
jsbell@chromium.org changed reviewers: + asvitkine@chromium.org
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?
LGTM, but I think you can drop the TODO comment. https://codereview.chromium.org/1192003006/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_dispatcher_host.cc (right): https://codereview.chromium.org/1192003006/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_dispatcher_host.cc:48: // TODO(jsbell): Further restrict to HTTPS, like Service Workers? Aren't service workers available to any "secure context"? localhost, 127.0.0.1, whatever's been whitelisted via command-line flag, etc?
https://codereview.chromium.org/1192003006/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_dispatcher_host.cc (right): https://codereview.chromium.org/1192003006/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_dispatcher_host.cc:48: // TODO(jsbell): Further restrict to HTTPS, like Service Workers? On 2015/06/18 19:57:14, Mike West wrote: > Aren't service workers available to any "secure context"? localhost, 127.0.0.1, > whatever's been whitelisted via command-line flag, etc? Whoops, the TODO is incorrect. I mean to refer to this line: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se... ... which additionally checks SchemeIsHTTPOrHTTPS(). That's an added restriction by SWs on top of the usual "secure features" check. ISTM that we don't want that here, i.e. just remove the TODO, but wanted to raise the issue for comment.
https://codereview.chromium.org/1192003006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1192003006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49964: + <int value="90" label="CSDH_INVALID_ORIGIN"/> Since this list is increasing, would you mind updating the UMA histogram in bad_message.c to be logged via UMA_HISTOGRAM_SPARSE_SLOWLY() rather than UMA_HISTOGRAM_ENUMERATION? The former is more efficient for sparse histograms (doesn't pre-allocate all the 91 buckets).
TODO dropped, and... https://codereview.chromium.org/1192003006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1192003006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49964: + <int value="90" label="CSDH_INVALID_ORIGIN"/> On 2015/06/18 20:31:34, Alexei Svitkine wrote: > Since this list is increasing, would you mind updating the UMA histogram in > bad_message.c to be logged via UMA_HISTOGRAM_SPARSE_SLOWLY() rather than > UMA_HISTOGRAM_ENUMERATION? The former is more efficient for sparse histograms > (doesn't pre-allocate all the 91 buckets). Done.
lgtm
jsbell@chromium.org changed reviewers: + palmer@chromium.org
+palmer for thoughts on aligning the Blink/Chromium-side checks. https://codereview.chromium.org/1192003006/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_dispatcher_host.cc (right): https://codereview.chromium.org/1192003006/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_dispatcher_host.cc:48: return IsOriginSecure(url); The Blink side uses: ExecutionContext::isPrivilegedContext() ... which (per https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-secure) lets through things like localhost, "file", and things like the New Tab Page - a broader net than what's allowed here. This results in checks that pass on the Blink side but fail here, causing the renderer to be terminated. Is there a better check to use here, or do we need to restrict the Blink side with documentOrigin->isSecure() as well? If so... are we overly restrictive relative to the spec?
ping palmer and/or mkwst for any suggestions here (see comment thread)
LGTM. Thanks! https://codereview.chromium.org/1192003006/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_dispatcher_host.cc (right): https://codereview.chromium.org/1192003006/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_dispatcher_host.cc:48: return IsOriginSecure(url); On 2015/06/19 20:29:12, jsbell wrote: > The Blink side uses: > > ExecutionContext::isPrivilegedContext() > > ... which (per > https://w3c.github.io/webappsec/specs/powerfulfeatures/#settings-secure) lets > through things like localhost, "file", and things like the New Tab Page - a > broader net than what's allowed here. This results in checks that pass on the > Blink side but fail here, causing the renderer to be terminated. > > Is there a better check to use here, or do we need to restrict the Blink side > with documentOrigin->isSecure() as well? If so... are we overly restrictive > relative to the spec? IsOriginSecure should/does also allow localhost, file, and so on. 40 bool IsOriginSecure(const GURL& url) { 41 if (url.SchemeIsCryptographic() || url.SchemeIsFile()) 42 return true; 43 44 if (url.SchemeIsFileSystem() && url.inner_url() && 45 IsOriginSecure(*url.inner_url())) { 46 return true; 47 } 48 49 std::string hostname = url.HostNoBrackets(); 50 if (net::IsLocalhost(hostname)) 51 return true; 52 53 if (ContainsKey(g_trustworthy_whitelist.Get().schemes(), url.scheme())) 54 return true; 55 56 if (ContainsKey(g_trustworthy_whitelist.Get().origins(), url.GetOrigin())) 57 return true; 58 59 return false; 60 } And see also its unit test. In any case, the Blink-side check is kind of like doing input validation in the client-side JavaScript: a performance optimization, possibly a usability aid, but not a security boundary. The security boundary is here, and I think it's correct.
lgtm
palmer@ and I tracked down the reason the NTP was failing - the chrome-search scheme wasn't being added to the secure protocols list on the chrome side. One pending issue on the blink side...
jsbell@chromium.org changed reviewers: + dmurph@chromium.org
jsbell@chromium.org changed required reviewers: + dmurph@chromium.org
Latest PS factors out the access check; despite being a trivial trampoline, I
think this makes the inspector agent clearer. Due to conflicting 'Cache' names
this does require explicit prefixing in a few places, alas.
One outstanding issue - maybe dmurph@ can help. With this CL in place, if I
visit an http: site and bring up the inspector's resources panel there is debug
spew:
[21229:21229:0728/153119:ERROR:CONSOLE(157)] "ServiceWorkerCacheAgent error
while loading caches: ", source:
chrome-devtools://devtools/bundled/sdk/ServiceWorkerCacheModel.js (157)
[21229:21229:0728/153119:ERROR:CONSOLE(52)] "Protocol Error: the message with
wrong id: {"error":{"code":-32000,"message":"Only secure origins are allowed
(see: https://goo.gl/Y0ZkNV)."},"id":300}", source:
chrome-devtools://devtools/bundled/sdk/InspectorBackend.js (52)
I don't know if we want to skip adding the Cache Storage tree node on insecure
origins, or just not treat this as an error. Thoughts?
dmurph@chromium.org changed reviewers: + pfeldman@chromium.org
Hm.... I imagine we get the error in C++. We can do either: * Tell the system to ignore these errors * Check to make sure we're on https before trying to have a cache storage tree item. I think pfeldman would have the best advice here, he was my reviewer for these patches.
On 2015/07/30 17:19:05, dmurph wrote: > Hm.... I imagine we get the error in C++. We can do either: > * Tell the system to ignore these errors > * Check to make sure we're on https before trying to have a cache storage tree > item. > > I think pfeldman would have the best advice here, he was my reviewer for these > patches. Either is fine. It is probably easiest to ignore this kind of errors. But hard-coding the notion of the secure origins into the front-end and not requesting caches is also fine.
jsbell@chromium.org changed reviewers: - dmurph@chromium.org, pfeldman@chromium.org
jsbell@chromium.org changed required reviewers: - dmurph@chromium.org
Thanks, all. And sorry for the confusion pfeldman@ and dmurph@ - I meant to ping you on the Blink-side CL. So far as I know this is now good to go; Blink side needs final review and can land, then this goes in.
The CQ bit was checked by jsbell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, falken@chromium.org, palmer@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1192003006/#ps80001 (title: "Add chrome-search scheme to secure scheme list")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1192003006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1192003006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jsbell@chromium.org changed reviewers: + jam@chromium.org
jam@ - can you review as / OWNER, specifically these two: chrome/common/chrome_content_client.cc (per palmer@) content/browser/bad_message.h (per asvitkine@)
lgtm
The CQ bit was checked by jsbell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1192003006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1192003006/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4c4749245f912e6e87950508b34483f2d80deba8 Cr-Commit-Position: refs/heads/master@{#342657} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
