Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 2454563003: Fix web accessible resource checks in ShouldAllowOpenURL (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months ago by alexmos
Modified:
7 months, 3 weeks ago
CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, site-isolation-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix web accessible resource checks in ShouldAllowOpenURL (This is joint work with nick@) When a web page navigates an extension main frame to another extension URL via a proxy, RenderFrameProxyHost::OnOpenURL calls NavigatorImpl::RequestTransferURL, which checks whether the destination URL points to a web-accessible resource in ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL. This CL fixes several problems with that check: 1. It doesn't work properly for blob/filesystem URL, since it references schemes on the destination GURL directly. This leads to a security problem where all blob/filesystem URLs are allowed. To fix this, use url::Origin to handle nested URL cases properly, similarly to other fixes in this area (e.g., issue 645028). 2. It ignores cases where the source SiteInstance's site URL isn't an HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or about:blank URL bypasses WAR checks. This artificial restriction apparently was added to make a test pass several years ago. This CL removes it, and fixes one test that was wrong and which relied on this logic. The test is ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to window.open a non-web-accessible resource while incorrectly assuming it was web-accessible. Since it did so from about:blank with a blank SiteInstance, this bug allowed the test to work. 3. The things that ShouldAllowOpenURL does block aren't blocked correctly for transfers. RequestTransferURL rewrites the blocked URL to about:blank, and later, NavigatorImpl::NavigateToEntry decides that this is a transfer to the same RFH (is_transfer_to_same == true), so it happily lets the old navigation continue and load the *old* (disallowed) URL. To fix this, we return early instead of rewriting dest_url to about:blank. 4. The SiteInstance plumbed through to ShouldAllowOpenURL when going via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current SiteInstance of the frame that's being navigated, rather than the SiteInstance in which the navigation on the proxy was started. RenderFrameProxyHost::OnOpenURL already passes the right SiteInstance as source_site_instance to RequestTransferURL, so it just needs to use it when it's available. Two new tests are added: NestedURLNavigationsViaProxyBlocked for case 1. WindowOpenInaccessibleResourceFromDataURL for case 2. The fixes to ShouldAllowOpenURL also apply to subframe navigations as well as main frame navigations (since it doesn't distinguish between the two). That is, subframe navigations that go through proxies, transfers, or OpenURL, will now also have the unsafe blob/filesystem URL blocking enforced, whereas before this was only done for main frames by other checks. The test exercising this for subframes, NestedURLNavigationsViaProxyBlocked, was updated accordingly. Why don't our other checks work? -------------------------------- 1. ExtensionNavigationThrottle also performs web-accessible resource checks checks, but that currently happens only for subframes. Thus, ShouldAllowOpenURL are the only checks that would work in this scenario for main frames. Fundamentally, ExtensionNavigationThrottle can't perform the WAR checks for main frames until we plumb through the initiator SiteInstance (i.e., who navigated the proxy, and not the StartingSiteInstance, which corresponds to the frame that's being navigated) to NavigationHandle. 2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to blob/filesystem URLs. It doesn't apply in this case, since the process where this navigation is happening is already an extension process. 3. url_request_util::AllowCrossRendererResourceLoad also allows this navigation, as it explicitly excludes all main frames from WAR enforcement: // Navigating the main frame to an extension URL is allowed, even if not // explicitly listed as web_accessible_resource. if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) { *allowed = true; return true; } BUG=656752, 645028, 652887 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4 Cr-Commit-Position: refs/heads/master@{#429532}

Patch Set 1 #

Patch Set 2 : Test fixes #

Patch Set 3 : Try to remove incorrect scheme checks #

Patch Set 4 : Fix ExtensionBrowserTest.WindowOpenNoPrivileges #

Patch Set 5 : Cleanup + test #

Patch Set 6 : More cleanup #

Patch Set 7 : Tighten check a bit more #

Total comments: 15

Patch Set 8 : Add metrics #

Patch Set 9 : Rebase #

Total comments: 31

Patch Set 10 : Charlie's comments #

Total comments: 2

Patch Set 11 : Devlin's and Alexei's comments #

Total comments: 4

Patch Set 12 : Round 2 of Devlin's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+307 lines, -85 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -3 lines 0 comments Download
M chrome/browser/extensions/chrome_content_browser_client_extensions_part.h View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +100 lines, -29 lines 0 comments Download
M chrome/browser/extensions/extension_resource_request_policy_apitest.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +16 lines, -35 lines 0 comments Download
M chrome/browser/extensions/process_manager_browsertest.cc View 1 2 3 4 5 6 7 8 9 5 chunks +106 lines, -13 lines 0 comments Download
M chrome/browser/extensions/window_open_apitest.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +43 lines, -0 lines 0 comments Download
M chrome/test/data/extensions/uitest/window_open/manifest.json View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/frame_host/navigator_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +14 lines, -3 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 2 chunks +24 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 83 (62 generated)
alexmos
Nick - please take a look. Charlie - please also take a look. (Since parts ...
8 months ago (2016-10-28 00:29:42 UTC) #37
ncarter (slow)
https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc#newcode575 chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:575: base::debug::DumpWithoutCrashing(); On 2016/10/28 00:29:41, alexmos wrote: > Should we ...
7 months, 4 weeks ago (2016-10-28 21:45:03 UTC) #39
alexmos
https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc#newcode575 chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:575: base::debug::DumpWithoutCrashing(); On 2016/10/28 21:45:03, ncarter wrote: > On 2016/10/28 ...
7 months, 3 weeks ago (2016-10-31 23:34:48 UTC) #41
ncarter (slow)
lgtm https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensions/window_open_apitest.cc File chrome/browser/extensions/window_open_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensions/window_open_apitest.cc#newcode319 chrome/browser/extensions/window_open_apitest.cc:319: newtab->GetMainFrame()->GetSiteInstance()->GetSiteURL().scheme()); On 2016/10/31 23:34:48, alexmos wrote: > On ...
7 months, 3 weeks ago (2016-10-31 23:42:10 UTC) #45
alexmos
Devlin, can you please review chrome/browser/extensions for OWNERS?
7 months, 3 weeks ago (2016-11-01 00:41:58 UTC) #49
Charlie Reis (slow)
Wow. That's an impressive list of issues to fix. I like where it's headed, and ...
7 months, 3 weeks ago (2016-11-01 18:12:18 UTC) #52
alexmos
Thanks for reviewing, Charlie! For the two questions about regressions, Nick and I tried a ...
7 months, 3 weeks ago (2016-11-01 22:04:22 UTC) #55
Charlie Reis (slow)
On 2016/11/01 22:04:22, alexmos wrote: > Thanks for reviewing, Charlie! > > For the two ...
7 months, 3 weeks ago (2016-11-01 23:16:28 UTC) #56
alexmos
Thanks! https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_host/navigator_impl.cc File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_host/navigator_impl.cc#newcode740 content/browser/frame_host/navigator_impl.cc:740: current_site_instance, url)) { On 2016/11/01 23:16:27, Charlie Reis ...
7 months, 3 weeks ago (2016-11-01 23:36:59 UTC) #57
alexmos
asvitkine@: can you please review histograms.xml?
7 months, 3 weeks ago (2016-11-01 23:40:15 UTC) #59
Devlin
https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc#newcode531 chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:531: ExtensionRegistry* registry = ExtensionRegistry::Get(profile); Not your code, but ExtensionRegistry ...
7 months, 3 weeks ago (2016-11-02 16:19:23 UTC) #62
Alexei Svitkine (very slow)
https://codereview.chromium.org/2454563003/diff/180001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/180001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc#newcode563 chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:563: FAILURE_BLOB_URL, FAILURE_LAST); Please make a helper function to log ...
7 months, 3 weeks ago (2016-11-02 16:35:47 UTC) #63
alexmos
Thanks, Devlin and Alexei. Comments addressed below. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc#newcode531 chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:531: ExtensionRegistry* registry ...
7 months, 3 weeks ago (2016-11-02 17:32:46 UTC) #66
Alexei Svitkine (very slow)
histograms lgtm
7 months, 3 weeks ago (2016-11-02 17:35:42 UTC) #67
Devlin
lgtm https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/extension_resource_request_policy_apitest.cc File chrome/browser/extensions/extension_resource_request_policy_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/extension_resource_request_policy_apitest.cc#newcode365 chrome/browser/extensions/extension_resource_request_policy_apitest.cc:365: EXPECT_NE("Private", content); On 2016/11/02 17:32:46, alexmos wrote: > ...
7 months, 3 weeks ago (2016-11-02 20:50:57 UTC) #70
alexmos
https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/extension_resource_request_policy_apitest.cc File chrome/browser/extensions/extension_resource_request_policy_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/extension_resource_request_policy_apitest.cc#newcode365 chrome/browser/extensions/extension_resource_request_policy_apitest.cc:365: EXPECT_NE("Private", content); On 2016/11/02 20:50:57, Devlin wrote: > On ...
7 months, 3 weeks ago (2016-11-03 00:43:26 UTC) #73
Devlin
https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/extension_resource_request_policy_apitest.cc File chrome/browser/extensions/extension_resource_request_policy_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/extension_resource_request_policy_apitest.cc#newcode365 chrome/browser/extensions/extension_resource_request_policy_apitest.cc:365: EXPECT_NE("Private", content); On 2016/11/03 00:43:26, alexmos wrote: > On ...
7 months, 3 weeks ago (2016-11-03 00:57:09 UTC) #74
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2454563003/220001
7 months, 3 weeks ago (2016-11-03 06:01:00 UTC) #79
commit-bot: I haz the power
Committed patchset #12 (id:220001)
7 months, 3 weeks ago (2016-11-03 06:06:42 UTC) #80
commit-bot: I haz the power
Patchset 12 (id:??) landed as https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4 Cr-Commit-Position: refs/heads/master@{#429532}
7 months, 3 weeks ago (2016-11-03 06:08:17 UTC) #82
findit-for-me
7 months, 3 weeks ago (2016-11-03 07:51:35 UTC) #83
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 429532 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318