New issue
Advanced search Search tips

Issue 661324 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Make subframe blocking in NestedURLNavigationsToExtensionBlocked work with PlzNavigate

Project Member Reported by alex...@chromium.org, Nov 1 2016

Issue description

This is a followup to https://codereview.chromium.org/2454563003/, which tightens web-accessible resource checks, and, among other things, starts enforcing the unsafe blob/filesystem URL blocking for subframes as well as main frames for navigations via proxies with --isolate-extensions.  This doesn't yet work for PlzNavigate, as that support needs to be added separately to ExtensionNavigationThrottle::WillStartRequest.  Once that's done, we'll need to enable the subframe part of the NestedURLNavigationsToExtensionBlocked test that was excluded for PlzNavigate here:  https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensions/process_manager_browsertest.cc#newcode809



 

Comment 1 by nick@chromium.org, Apr 27 2017

Owner: nick@chromium.org
Looks like I'm fixing this by accident.
Project Member

Comment 2 by bugdroid1@chromium.org, May 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c2f3f0b21b2e0eefe0e76e5f519a30970933202

commit 1c2f3f0b21b2e0eefe0e76e5f519a30970933202
Author: nick <nick@chromium.org>
Date: Wed May 17 03:35:17 2017

PlzNavigate: don't reuse current_frame_host() for error pages
if the navigation is browser-initiated. The "stay in current
process to prevent privilege escalation" strategy is only valid
when the navigation was initiated by that process.

(As an aside, it is worth pointing out that current_frame_host is
not necessarily the initiator process.)

This change prevents a CheckWebUIRendererDoesNotDisplayNormalURL browser
crash in the scenario where the current page is chrome://settings, and
the user types in an URL that happens to be blocked by a
NavigationThrottle. This scenario starts being possible once
ExtensionNavigationThrottle starts doing more aggressive blocking of
top-level navigations.

BUG=661324
TEST=ToolbarModelTest.ShouldDisplayURL
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2884123002
Cr-Commit-Position: refs/heads/master@{#472303}

[modify] https://crrev.com/1c2f3f0b21b2e0eefe0e76e5f519a30970933202/content/browser/frame_host/navigation_handle_impl_browsertest.cc
[modify] https://crrev.com/1c2f3f0b21b2e0eefe0e76e5f519a30970933202/content/browser/frame_host/navigation_request.cc

Project Member

Comment 3 by bugdroid1@chromium.org, May 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6cfe5c7f23e1447422a21ddc7b6aa597a0e7021a

commit 6cfe5c7f23e1447422a21ddc7b6aa597a0e7021a
Author: nick <nick@chromium.org>
Date: Mon May 22 22:00:42 2017

Refactor of ExtensionNavigationThrottle

Functional changes here are:
 - Treating an extension-origin "about:blank" iframe as extension,
   for the purposes of the web_accessible_resources ancestor
   check.
 - Treating nested URLs as non-web_accessible_resources for
   the purposes of subframe navigation (this fixes 661324)

These should both be minor, and are incidental.

Pure refactoring here is:
 - Preferring url::Origin() checks (which leads to the functional
   changes above).
 - Renaming of variables for clarity.
 - Hoisting the extension lookup to the top of the function, and
   eliminating !extension and !registry checks after dealing
   with the lookup result.
 - A more efficient lookup of the parent frame, in a way
   that shouldn't require an "this is safe" apology comment.
 - Reflow control logic in anticipation of adding new BLOCK
   cases for platform apps (in a follow-on CL).

BUG=661324
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2830893002
Cr-Commit-Position: refs/heads/master@{#473717}

[modify] https://crrev.com/6cfe5c7f23e1447422a21ddc7b6aa597a0e7021a/chrome/browser/extensions/extension_browsertest.cc
[modify] https://crrev.com/6cfe5c7f23e1447422a21ddc7b6aa597a0e7021a/chrome/browser/extensions/extension_navigation_throttle_unittest.cc
[modify] https://crrev.com/6cfe5c7f23e1447422a21ddc7b6aa597a0e7021a/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/6cfe5c7f23e1447422a21ddc7b6aa597a0e7021a/chrome/browser/extensions/window_open_apitest.cc
[modify] https://crrev.com/6cfe5c7f23e1447422a21ddc7b6aa597a0e7021a/chrome/browser/ui/toolbar/toolbar_model_unittest.cc
[modify] https://crrev.com/6cfe5c7f23e1447422a21ddc7b6aa597a0e7021a/extensions/browser/extension_navigation_throttle.cc

Project Member

Comment 4 by bugdroid1@chromium.org, May 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be2ecca29e9914527c5ee5deaceefd02646a3b72

commit be2ecca29e9914527c5ee5deaceefd02646a3b72
Author: nick <nick@chromium.org>
Date: Wed May 24 18:08:32 2017

PlzNavigateNavigationHandleImplBrowserTest.BlockedRequestAfterWebUI

Tests the case where a browser-initiated navigation to a normal webpage is
blocked (net::ERR_BLOCKED_BY_CLIENT) while departing from a privileged WebUI
page (chrome://gpu). It is a security risk for the error page to commit in
the privileged process.

BUG=661324
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2898763004
Cr-Commit-Position: refs/heads/master@{#474356}

[modify] https://crrev.com/be2ecca29e9914527c5ee5deaceefd02646a3b72/content/browser/frame_host/navigation_handle_impl_browsertest.cc

Owner: ----

Sign in to add a comment