Security: Enforce blob/filesystem "local scheme" checks in FilterURL |
|||||||||||||
Issue description[Split off from bug 820838] [Related to sub- bug 821586 ] "blob:" and "filesystem:" schemes are not allowed to be used cross-origin; blink enforces this with a console error: > location.href = 'filesystem:http://www.example.com/temporary/hello'; >> (index):1 Not allowed to load local resource: filesystem:http://www.example.com/temporary/hello However, if these renderer checks are defeated (e.g. by --disable-web-security), the browser process does not block such navigations. We can improve this by hardening ChildProcessSecurityPolicyImpl::CanRequestURL so that it requires CanCommitURL-level privileges. This will supplant and generalize the enforcement already attempted (for "chrome-extension:" origins) in ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
,
Mar 13 2018
,
Mar 14 2018
Tentatively assigning Security_Impact-None as it sounds like this is more of a hardening measure than a security bug. (Not 100% sure how to triage these sub-bugs of a chain.)
,
Mar 14 2018
Agree that it's primarily a hardening measure. There was a version of the full chain that used this, but another that didn't. The important security boundary here (now that we have isolate-extensions) is to prevent one origin placing data in another origin via blob/filesystem URLs. Preventing it from being navigated to once it's there is a much weaker defense; but can be a backstop. Before isolate-extensions, the navigation suppression was a more important defense.
,
Mar 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a6102ee28b50372c8cf67ee3040c929e86b1eb07 commit a6102ee28b50372c8cf67ee3040c929e86b1eb07 Author: Nick Carter <nick@chromium.org> Date: Wed Mar 14 18:49:13 2018 Stricter blob/filesystem check in CanRequestURL. Blobs and filesystem URLs are considered "local" schemes by blink; they can only be requested from processes with permission to commit them. Adding this means that we block extension filesystem: URLs via FilterURL rather than ShouldAllowOpenURL, meaning that the WebAccessibleResource UMAs don't trigger. Update test expectations to reflect this. Add a test that exercises an interesting 'noopener' corner case. Change-Id: Ie0473a4d4034ceb03e6098d524f16ef8d215398e Bug: 821586 , 821596 Reviewed-on: https://chromium-review.googlesource.com/961126 Reviewed-by: Reilly Grant <reillyg@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Nick Carter <nick@chromium.org> Cr-Commit-Position: refs/heads/master@{#543142} [modify] https://crrev.com/a6102ee28b50372c8cf67ee3040c929e86b1eb07/chrome/browser/extensions/process_manager_browsertest.cc [modify] https://crrev.com/a6102ee28b50372c8cf67ee3040c929e86b1eb07/content/browser/child_process_security_policy_impl.cc
,
Mar 14 2018
Nick confirms this one should be fixed now.
,
Mar 15 2018
,
Mar 15 2018
Nick: Can you verify the fix in today's 67.0.3371.0 Canary and request merge if it looks good? (I'm guessing we want it in M66 but M65 might not be necessary?) Thanks!
,
Mar 15 2018
This won't make a dev or beta before the stable merge is needed, so it doesn't meet the normal bar for a stable merge. We'll pick it up in 66.
,
Mar 15 2018
Verified fixed on 67.0.3371.0 Will merge to 66.
,
Mar 15 2018
,
Mar 15 2018
Branch:3359 M66 approved
,
Mar 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fd30633d9051ff97eed229cab019f75eaaa16144 commit fd30633d9051ff97eed229cab019f75eaaa16144 Author: Nick Carter <nick@chromium.org> Date: Thu Mar 15 18:43:02 2018 Stricter blob/filesystem check in CanRequestURL. Blobs and filesystem URLs are considered "local" schemes by blink; they can only be requested from processes with permission to commit them. Adding this means that we block extension filesystem: URLs via FilterURL rather than ShouldAllowOpenURL, meaning that the WebAccessibleResource UMAs don't trigger. Update test expectations to reflect this. Add a test that exercises an interesting 'noopener' corner case. Change-Id: Ie0473a4d4034ceb03e6098d524f16ef8d215398e Bug: 821586 , 821596 Reviewed-on: https://chromium-review.googlesource.com/961126 Reviewed-by: Reilly Grant <reillyg@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Nick Carter <nick@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#543142}(cherry picked from commit a6102ee28b50372c8cf67ee3040c929e86b1eb07) Reviewed-on: https://chromium-review.googlesource.com/962975 Reviewed-by: Nick Carter <nick@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#273} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/fd30633d9051ff97eed229cab019f75eaaa16144/chrome/browser/extensions/process_manager_browsertest.cc [modify] https://crrev.com/fd30633d9051ff97eed229cab019f75eaaa16144/content/browser/child_process_security_policy_impl.cc
,
Mar 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ac31d43d24d2e855e73c588fb9705904276da91e commit ac31d43d24d2e855e73c588fb9705904276da91e Author: Nick Carter <nick@chromium.org> Date: Fri Mar 16 23:38:35 2018 Fix test flake in ProcessManagerBrowserTest.NestedURLNavigationsViaProxyBlocked Cause of flake was confirmed by adding a slow unload handler to |popup|. This fixes it. Bug: 821596 , 822635 Change-Id: I602c98a0fb37f9951cc85fb9d886616bb22e8b42 Reviewed-on: https://chromium-review.googlesource.com/967352 Reviewed-by: Reilly Grant <reillyg@chromium.org> Commit-Queue: Nick Carter <nick@chromium.org> Cr-Commit-Position: refs/heads/master@{#543870} [modify] https://crrev.com/ac31d43d24d2e855e73c588fb9705904276da91e/chrome/browser/extensions/process_manager_browsertest.cc
,
Mar 23 2018
,
Mar 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d2e4acff8e95d6ff7794dbda0ae74cb49bb2b180 commit d2e4acff8e95d6ff7794dbda0ae74cb49bb2b180 Author: Nick Carter <nick@chromium.org> Date: Fri Mar 30 22:13:29 2018 Fix bug 825111 : Restore ability to request blob URLs from extension content scripts. This is effectively a revert of the active ingredients of r543142, except that it also adds a test, and the tests added in r543142 still pass due to enforcement that occurs via ShouldAllowOpenURL (added in r547218). Bug: 825111 , 821596 Change-Id: I6e53bfc70cf3cf67221de9cd51dee00077153643 Reviewed-on: https://chromium-review.googlesource.com/982645 Commit-Queue: Nick Carter <nick@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#547283} [modify] https://crrev.com/d2e4acff8e95d6ff7794dbda0ae74cb49bb2b180/chrome/browser/extensions/content_script_apitest.cc [modify] https://crrev.com/d2e4acff8e95d6ff7794dbda0ae74cb49bb2b180/chrome/browser/extensions/process_manager_browsertest.cc [add] https://crrev.com/d2e4acff8e95d6ff7794dbda0ae74cb49bb2b180/chrome/test/data/extensions/api_test/content_scripts/blob_fetch/content_script.js [add] https://crrev.com/d2e4acff8e95d6ff7794dbda0ae74cb49bb2b180/chrome/test/data/extensions/api_test/content_scripts/blob_fetch/manifest.json [add] https://crrev.com/d2e4acff8e95d6ff7794dbda0ae74cb49bb2b180/chrome/test/data/extensions/api_test/content_scripts/blob_fetch/test.html [add] https://crrev.com/d2e4acff8e95d6ff7794dbda0ae74cb49bb2b180/chrome/test/data/extensions/api_test/content_scripts/blob_fetch/test.js [modify] https://crrev.com/d2e4acff8e95d6ff7794dbda0ae74cb49bb2b180/content/browser/child_process_security_policy_impl.cc
,
Mar 30 2018
Are CLs listed at #14 and #16 need a merge to M66? If yes, pls request a merge. Thank you.
,
Apr 2 2018
I've requested a merge for the above changes via bug 825111 .
,
Apr 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/acc0f30fa74a040b656b67450726efb866bbd218 commit acc0f30fa74a040b656b67450726efb866bbd218 Author: Nick Carter <nick@chromium.org> Date: Tue Apr 03 19:47:37 2018 Fix test flake in ProcessManagerBrowserTest.NestedURLNavigationsViaProxyBlocked Cause of flake was confirmed by adding a slow unload handler to |popup|. This fixes it. Bug: 821596 , 822635 Change-Id: I602c98a0fb37f9951cc85fb9d886616bb22e8b42 Reviewed-on: https://chromium-review.googlesource.com/967352 Reviewed-by: Reilly Grant <reillyg@chromium.org> Commit-Queue: Nick Carter <nick@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#543870}(cherry picked from commit ac31d43d24d2e855e73c588fb9705904276da91e) Reviewed-on: https://chromium-review.googlesource.com/990453 Reviewed-by: Nick Carter <nick@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#560} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/acc0f30fa74a040b656b67450726efb866bbd218/chrome/browser/extensions/process_manager_browsertest.cc
,
Apr 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d1f247f93d555705098223582ad2ef06419e0299 commit d1f247f93d555705098223582ad2ef06419e0299 Author: Nick Carter <nick@chromium.org> Date: Tue Apr 03 22:26:31 2018 Fix bug 825111 : Restore ability to request blob URLs from extension content scripts. This is effectively a revert of the active ingredients of r543142, except that it also adds a test, and the tests added in r543142 still pass due to enforcement that occurs via ShouldAllowOpenURL (added in r547218). Bug: 825111 , 821596 Change-Id: I6e53bfc70cf3cf67221de9cd51dee00077153643 Reviewed-on: https://chromium-review.googlesource.com/982645 Commit-Queue: Nick Carter <nick@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#547283}(cherry picked from commit d2e4acff8e95d6ff7794dbda0ae74cb49bb2b180) Reviewed-on: https://chromium-review.googlesource.com/994272 Reviewed-by: Nick Carter <nick@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#568} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/d1f247f93d555705098223582ad2ef06419e0299/chrome/browser/extensions/content_script_apitest.cc [modify] https://crrev.com/d1f247f93d555705098223582ad2ef06419e0299/chrome/browser/extensions/process_manager_browsertest.cc [add] https://crrev.com/d1f247f93d555705098223582ad2ef06419e0299/chrome/test/data/extensions/api_test/content_scripts/blob_fetch/content_script.js [add] https://crrev.com/d1f247f93d555705098223582ad2ef06419e0299/chrome/test/data/extensions/api_test/content_scripts/blob_fetch/manifest.json [add] https://crrev.com/d1f247f93d555705098223582ad2ef06419e0299/chrome/test/data/extensions/api_test/content_scripts/blob_fetch/test.html [add] https://crrev.com/d1f247f93d555705098223582ad2ef06419e0299/chrome/test/data/extensions/api_test/content_scripts/blob_fetch/test.js [modify] https://crrev.com/d1f247f93d555705098223582ad2ef06419e0299/content/browser/child_process_security_policy_impl.cc
,
May 3 2018
Setting some labels for posterity.
,
Jun 21 2018
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by creis@chromium.org
, Mar 13 2018