New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 732260 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Feature

Blocking:
issue 671609



Sign in to add a comment

Have WebContents deletion work asynchronously for navigation in Clank

Project Member Reported by jinsuk...@chromium.org, Jun 12 2017

Issue description

Quoted from clamy@'s note:

"right now it's asynchronous on all platforms, except Clank. Because of that, we've had to make some specific code in the main path of navigation asynchronous just on Clank, because we were risking UAFs. It'd be better if we didn't risk UAFs when executing a method from inside content/."


 
Status: Assigned (was: Available)
https://bugs.chromium.org/p/chromium/issues/detail?id=128962#c11 has more explanation.

One possible UAF situation is reproducible with following steps:

0) Enabled PlzNavigate
1) Run IntercepNavigationThrottle's shouldIgnoreCallback in synchronous mode:

--- a/components/navigation_interception/intercept_navigation_delegate.cc
+++ b/components/navigation_interception/intercept_navigation_delegate.cc
@@ -89,7 +89,7 @@ std::unique_ptr<content::NavigationThrottle>
 InterceptNavigationDelegate::CreateThrottleFor(
     content::NavigationHandle* handle) {
   return base::MakeUnique<InterceptNavigationThrottle>(
-      handle, base::Bind(&CheckIfShouldIgnoreNavigationOnUIThread), false);
+      handle, base::Bind(&CheckIfShouldIgnoreNavigationOnUIThread), true);
 }

2) Open a html page with an anchor launching external app. For instance, use a link to phone app:

<a target="_blank" href="tel:5555555;ext=555">Tel app</a>

A click on the link will launch the phone app, and also open an empty tab that should be closed.

3) Observe crash - InterceptNavigationThrottle and its relevant objects are already destroyed at this point.

I think it can be handled by https://crrev.com/c/531144. It's still in progress but please take an initial look and see if this is going towards the right direction. Let me know if there are other UAF situations that should be taken into account.
Other cases that will close the current WebContents are:
- Launching app from Incognito tab
- Download files
- Local file access permission

(in InterceptNavigationDelegateImpl -> ExternalNavigationHandler -> ExternalNavigationDelegateImpl)

These are all safe from UAF concern because they all close the tab after user interaction (warning dialog about leaving incognito mode, download accept/cancel, etc), effectively doing it behind the UI message loop. So I think we're covered in all grounds.

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 17 2017

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

commit d5545cdaf40989792dd8cc2483304c613ac9f54c
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Sat Jun 17 15:55:52 2017

PlzNavigate: Run InterceptnavigationThrottle in synchronous mode

Puts InterceptNavigationThrottle's shouldIgnoreNavigation callback
in synchronous mode, and defer the destruction of WebContents
(and the associated tab) while navigation throttle is in progress
and finishes accessing the object. This makes the navigation throttle
flow simpler on Android.

BUG= 732260 

Change-Id: Ib151b5458a3adde927db22c825f708c5266f6372
Reviewed-on: https://chromium-review.googlesource.com/531144
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Selim Gurun <sgurun@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480304}
[modify] https://crrev.com/d5545cdaf40989792dd8cc2483304c613ac9f54c/chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java
[modify] https://crrev.com/d5545cdaf40989792dd8cc2483304c613ac9f54c/chrome/browser/apps/app_url_redirector.cc
[modify] https://crrev.com/d5545cdaf40989792dd8cc2483304c613ac9f54c/chrome/browser/plugins/flash_download_interception.cc
[modify] https://crrev.com/d5545cdaf40989792dd8cc2483304c613ac9f54c/components/navigation_interception/intercept_navigation_delegate.cc
[modify] https://crrev.com/d5545cdaf40989792dd8cc2483304c613ac9f54c/components/navigation_interception/intercept_navigation_throttle.cc
[modify] https://crrev.com/d5545cdaf40989792dd8cc2483304c613ac9f54c/components/navigation_interception/intercept_navigation_throttle.h
[modify] https://crrev.com/d5545cdaf40989792dd8cc2483304c613ac9f54c/components/navigation_interception/intercept_navigation_throttle_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 3 2017

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

commit c644aad97a1e9efa2c818099fd33cee6386a09bf
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Mon Jul 03 21:32:44 2017

Reland "PlzNavigate: Run InterceptnavigationThrottle in synchronous mode""

The reverted CL ran the tab closing task in InterceptNavigation-
Throttle in an asynchronous fashion (posting task onto UI thread
again) to avoid UAF. In addition to that, loading a new URL on
the current tab by the throttle should have been given the same
treat because it cancels the ongoing navigation as well, hence
can cause UAF error. This CL handles the missing case.

BUG= 732260 

This reverts commit 6d4d2da0bda314416b87c1c3dde87ac96e9634f4.

Change-Id: I423d09c30f01fcd1d1f6ddf806d0a0123609bf9a
Reviewed-on: https://chromium-review.googlesource.com/552143
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Selim Gurun <sgurun@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484030}
[modify] https://crrev.com/c644aad97a1e9efa2c818099fd33cee6386a09bf/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java
[modify] https://crrev.com/c644aad97a1e9efa2c818099fd33cee6386a09bf/chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java
[modify] https://crrev.com/c644aad97a1e9efa2c818099fd33cee6386a09bf/chrome/browser/apps/app_url_redirector.cc
[modify] https://crrev.com/c644aad97a1e9efa2c818099fd33cee6386a09bf/chrome/browser/plugins/flash_download_interception.cc
[modify] https://crrev.com/c644aad97a1e9efa2c818099fd33cee6386a09bf/components/navigation_interception/intercept_navigation_delegate.cc
[modify] https://crrev.com/c644aad97a1e9efa2c818099fd33cee6386a09bf/components/navigation_interception/intercept_navigation_throttle.cc
[modify] https://crrev.com/c644aad97a1e9efa2c818099fd33cee6386a09bf/components/navigation_interception/intercept_navigation_throttle.h
[modify] https://crrev.com/c644aad97a1e9efa2c818099fd33cee6386a09bf/components/navigation_interception/intercept_navigation_throttle_unittest.cc

Status: Fixed (was: Assigned)

Comment 6 by lizeb@chromium.org, Jul 21 2017

Blocking: 671609

Sign in to add a comment