New issue
Advanced search Search tips

Issue 823864 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Make WebUI more robust to user gesture spoofing

Project Member Reported by dcheng@chromium.org, Mar 20 2018

Issue description

Today, this is enforced renderer-side: https://cs.chromium.org/chromium/src/content/renderer/web_ui_extension.cc?rcl=a0a5c4046775f09dfff103972baf6ab12e33acc5&l=101.

But unfortunately, the renderer can be compromised and then the signal will be unreliable. Instead, we should track it browser-side. While having per-frame browser-side tracking would be the ideal end state, for now, it should be sufficient to track it with WebContents granularity. alexmos points out https://cs.chromium.org/chromium/src/content/browser/web_contents/web_contents_impl.cc?l=5470&rcl=38751e3d1de591c99a49c2d535d427e940ec08e8, so we ought to be able to keep track of the last time we received a user gesture.

WebUI callbacks that require a user gesture should be able to check the last gesture time and determine if the action should be allowed or not.

Mojofied WebUI can do these checks as well, but it will have to be in each individual method handler, since there's Mojofied WebUI no longer has a central location where WebUI message are processed.
 

Comment 1 by dpa...@chromium.org, Mar 20 2018

This is unfortunately not as easy. It seems that the gesture detection code in Blink does not work well with some of the newer MD UI components. For example see [1] and [2] where a check had to be removed for chrome://extensions because it was not detected.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=764129#c9
[2] https://chromium-review.googlesource.com/c/chromium/src/+/734707

Comment 2 by mustaq@chromium.org, Mar 20 2018

I suspect stack-scope of UserGestureIndicator could be the culprit but I don't know WebUI code enough to confirm or verify.

dpapad@: If the bug you fixed through the CL in #c1 is easy to repro, could you please verify if enabling our new model (chrome://flags/#user-activation-v2) makes the CL unnecessary?

Comment 3 by dpa...@chromium.org, Mar 20 2018

Should be easy to check if that fix is still necessary. Will try that shortly and report back.

Comment 4 by dpa...@chromium.org, Mar 20 2018

Enabling chrome://flags/#user-activation-v2 seems to make the CL mentioned at #1 unnecessary. FWIW, here are the test steps.

1) Navigate to any extensions details page.
2) Find the "allow in incognito" toggle and drag it left/right.

Inspect DevTools console, there should be no errors (see attached 
user_gesture_fails.mp4
1.8 MB View Download
user_activation2_works.mp4
977 KB View Download

Comment 5 by dpa...@chromium.org, Mar 20 2018

Forgot to mention, that the repro steps only work if you revert https://chromium-review.googlesource.com/c/chromium/src/+/734707 locally first.

Comment 6 by creis@chromium.org, Mar 20 2018

Comments 2-5: Thanks for checking that!  mustaq@, do you know when chrome://flags/#user-activation-v2 will be enabled?

It would be great to make progress here, because this is important for issue 595841 (which we've wanted to fix for a long time).

Comment 7 by mustaq@chromium.org, Mar 21 2018

Great to know UAv2 solves this and other related bugs!

We expect to ship UAv2 around M68.  If our work around site isolation goes well, we plan to enable temporarily in M67beta.

Comment 8 by dcheng@chromium.org, Mar 21 2018

#1: just to be clear, this bug isn't about the Blink side at all, so the stack-based nature of user gesture indicator today isn't a problem. This proposal is purely about a short-term workaround for tracking user gesture state on a per-WebContents basis. This would address some low-hanging fruit: certain WebUI pages have privileged operations that are supposed to be gated on user gesture, but this enforcement is purely renderer-side at the moment.

By tracking this on a per-WebContents basis, we'd be able to verify that the privileged methods requiring user gesture could plausibly have been triggered by one, rather than just being called by an exploited renderer with a fake user gesture on the stack.

Comment 9 by dpa...@chromium.org, Mar 21 2018

IIUC, only a subset of privileged WebUI interactions with the browser process would be benefiting, right? Specifically user gesture checks could be added only for chrome.* private APIs (like developerPrivate, languagesPrivate) mutating methods. 

Would it be possible to also add such checks within some mutating chrome.send() C++ handlers?
Aren't those extension APIs? I guess we should check those too. The main entry point (for non-Mojofied JS) that I know of is https://cs.chromium.org/chromium/src/content/browser/webui/web_ui_impl.cc?rcl=c27ca05e015a4275604c48611ef752d1ff41930d&l=107

If there are others, maybe we should list them out so we make sure they're covered.

Comment 11 by kenrb@chromium.org, Mar 21 2018

I'm curious, is this deliberately set as P1? This would add a reasonable but minor barrier for an attacker who has exploited an extension or otherwise gotten into a WebUI process (you have to induce a click by the user), but that level of exploitation should be difficult these days since extensions are now process-isolated from web content.
Owner: dcheng@chromium.org
Status: Started (was: Available)
Given the number of times WebUI surfaces have been a component of sandbox escapes, I think P1 is appropriate. Granted, this is generally only the last step--but if we can break the last step, that would make exploitation even harder.
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 30 2018

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 30 2018

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

commit 01e8024e561d55e2a9e04d4dd1f5e86a2d29f1ed
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Fri Mar 30 07:23:34 2018

Revert "Enforce user gesture requirement on browser side for WebUI."

This reverts commit f6953215ca45988400eb3cb7ab406456b72f36de.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 547114 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2Y2OTUzMjE1Y2E0NTk4ODQwMGViM2NiN2FiNDA2NDU2YjcyZjM2ZGUM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-rel/6414

Sample Failed Step: browser_tests

Original change's description:
> Enforce user gesture requirement on browser side for WebUI.
> 
> Also, don't trust the renderer process to send the correct URL.
> 
> Bug:  823864 
> Change-Id: I1f5c1c7908d8157240b5a7d5fc2df944267cf537
> Reviewed-on: https://chromium-review.googlesource.com/979399
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547114}

Change-Id: I5f801f404bee365976a3adb85f26cd8a6fde196a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  823864 
Reviewed-on: https://chromium-review.googlesource.com/987693
Cr-Commit-Position: refs/heads/master@{#547126}
[modify] https://crrev.com/01e8024e561d55e2a9e04d4dd1f5e86a2d29f1ed/chrome/browser/ui/webui/webui_browsertest.cc
[modify] https://crrev.com/01e8024e561d55e2a9e04d4dd1f5e86a2d29f1ed/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/01e8024e561d55e2a9e04d4dd1f5e86a2d29f1ed/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/01e8024e561d55e2a9e04d4dd1f5e86a2d29f1ed/content/browser/webui/web_ui_impl.cc
[modify] https://crrev.com/01e8024e561d55e2a9e04d4dd1f5e86a2d29f1ed/content/browser/webui/web_ui_impl.h
[modify] https://crrev.com/01e8024e561d55e2a9e04d4dd1f5e86a2d29f1ed/content/common/frame_messages.h
[modify] https://crrev.com/01e8024e561d55e2a9e04d4dd1f5e86a2d29f1ed/content/renderer/web_ui_extension.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 2 2018

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

commit 3017d428e2673eb7d3a508eab532139b874e03fa
Author: Daniel Cheng <dcheng@chromium.org>
Date: Mon Apr 02 10:31:59 2018

Reland "Enforce user gesture requirement on browser side for WebUI."

This is a reland of f6953215ca45988400eb3cb7ab406456b72f36de

Original change's description:
> Enforce user gesture requirement on browser side for WebUI.
>
> Also, don't trust the renderer process to send the correct URL.
>
> Bug:  823864 
> Change-Id: I1f5c1c7908d8157240b5a7d5fc2df944267cf537
> Reviewed-on: https://chromium-review.googlesource.com/979399
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547114}

Bug:  823864 
Tbr: sky@chromium.org
Change-Id: I9751f0f63524aa52bc5af326cd00e99a84dc1009
Reviewed-on: https://chromium-review.googlesource.com/987694
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547425}
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/chrome/browser/chrome_site_per_process_browsertest.cc
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/chrome/browser/ui/webui/webui_browsertest.cc
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/browser/webui/web_ui_impl.cc
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/browser/webui/web_ui_impl.h
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/common/frame_messages.h
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/public/browser/web_contents.h
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/public/test/web_contents_tester.h
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/renderer/web_ui_extension.cc
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/test/test_web_contents.cc
[modify] https://crrev.com/3017d428e2673eb7d3a508eab532139b874e03fa/content/test/test_web_contents.h

Project Member

Comment 16 by bugdroid1@chromium.org, Apr 2 2018

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

commit bc2dffe905df86de7c2f8c1c494263375540ba59
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Mon Apr 02 12:14:29 2018

Revert "Reland "Enforce user gesture requirement on browser side for WebUI.""

This reverts commit 3017d428e2673eb7d3a508eab532139b874e03fa.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 547425 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzMwMTdkNDI4ZTI2NzNlYjdkM2E1MDhlYWI1MzIxMzliODc0ZTAzZmEM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/4999

Sample Failed Step: browser_tests

Original change's description:
> Reland "Enforce user gesture requirement on browser side for WebUI."
> 
> This is a reland of f6953215ca45988400eb3cb7ab406456b72f36de
> 
> Original change's description:
> > Enforce user gesture requirement on browser side for WebUI.
> >
> > Also, don't trust the renderer process to send the correct URL.
> >
> > Bug:  823864 
> > Change-Id: I1f5c1c7908d8157240b5a7d5fc2df944267cf537
> > Reviewed-on: https://chromium-review.googlesource.com/979399
> > Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> > Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> > Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#547114}
> 
> Bug:  823864 
> Tbr: sky@chromium.org
> Change-Id: I9751f0f63524aa52bc5af326cd00e99a84dc1009
> Reviewed-on: https://chromium-review.googlesource.com/987694
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547425}

Change-Id: I100d8b7bdcf95693a3ee8a7d70af76bfc0ea9654
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  823864 
Reviewed-on: https://chromium-review.googlesource.com/989992
Cr-Commit-Position: refs/heads/master@{#547429}
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/chrome/browser/chrome_site_per_process_browsertest.cc
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/chrome/browser/ui/webui/webui_browsertest.cc
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/browser/webui/web_ui_impl.cc
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/browser/webui/web_ui_impl.h
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/common/frame_messages.h
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/public/browser/web_contents.h
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/public/test/web_contents_tester.h
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/renderer/web_ui_extension.cc
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/test/test_web_contents.cc
[modify] https://crrev.com/bc2dffe905df86de7c2f8c1c494263375540ba59/content/test/test_web_contents.h

Project Member

Comment 17 by bugdroid1@chromium.org, Apr 24 2018

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

commit 8839d302691536f5cdb93c0931e0b6a8eeb2a9ab
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue Apr 24 05:03:28 2018

Enforce user gesture requirement on browser side for WebUI.

WebContentsImpl now tracks the last time it received an input event that
could be considered user interaction. When the browser process receives
a WebUI message that requires a user gesture, it checks the WebContents
hosting the WebUI to make sure that the user recently interacted with
it.

This also cleans up a few incidental bits of code:
- RenderWidgetHost no longer prefilters events before notifying the
  delegate. This exposed some broken event filtering, tracked at
   https://crbug.com/827659 .
- Since the delegate method no longer prefilters input events,
  RenderWidgetHostDelegate::OnUserInteraction() is now named
  RenderWidgetHostDelegate::DidReceiveInputEvent().
- The IPC message used to pass WebUI messages from the renderer to the
  browser sends the source URL. However, this requires trusting the
  untrustworthy renderer to not lie about the URL. Instead, just use
  GetLastCommittedURL().

Bug:  823864 
Change-Id: I687776c8c7f7f93f78c968dd432b1674d65629c0
Reviewed-on: https://chromium-review.googlesource.com/1023174
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553001}
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/chrome/browser/chrome_site_per_process_browsertest.cc
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/chrome/browser/ui/webui/webui_browsertest.cc
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/browser/webui/web_ui_impl.cc
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/browser/webui/web_ui_impl.h
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/common/frame_messages.h
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/public/browser/web_contents.h
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/public/test/web_contents_tester.h
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/renderer/web_ui_extension.cc
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/test/test_web_contents.cc
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/content/test/test_web_contents.h
[modify] https://crrev.com/8839d302691536f5cdb93c0931e0b6a8eeb2a9ab/tools/metrics/histograms/histograms.xml

Project Member

Comment 18 by bugdroid1@chromium.org, Apr 25 2018

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

commit 8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec
Author: Daniel Cheng <dcheng@chromium.org>
Date: Wed Apr 25 18:54:21 2018

Revert "Enforce user gesture requirement on browser side for WebUI."

This reverts commit 8839d302691536f5cdb93c0931e0b6a8eeb2a9ab.

Reason for revert: causing WebUI renderer kills

Original change's description:
> Enforce user gesture requirement on browser side for WebUI.
> 
> WebContentsImpl now tracks the last time it received an input event that
> could be considered user interaction. When the browser process receives
> a WebUI message that requires a user gesture, it checks the WebContents
> hosting the WebUI to make sure that the user recently interacted with
> it.
> 
> This also cleans up a few incidental bits of code:
> - RenderWidgetHost no longer prefilters events before notifying the
>   delegate. This exposed some broken event filtering, tracked at
>    https://crbug.com/827659 .
> - Since the delegate method no longer prefilters input events,
>   RenderWidgetHostDelegate::OnUserInteraction() is now named
>   RenderWidgetHostDelegate::DidReceiveInputEvent().
> - The IPC message used to pass WebUI messages from the renderer to the
>   browser sends the source URL. However, this requires trusting the
>   untrustworthy renderer to not lie about the URL. Instead, just use
>   GetLastCommittedURL().
> 
> Bug:  823864 
> Change-Id: I687776c8c7f7f93f78c968dd432b1674d65629c0
> Reviewed-on: https://chromium-review.googlesource.com/1023174
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Reviewed-by: Ilya Sherman <isherman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#553001}

TBR=dcheng@chromium.org,thestig@chromium.org,isherman@chromium.org,alexmos@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  823864 
Change-Id: I7e6266f5e0d87197a76f2f201faef13bce129734
Reviewed-on: https://chromium-review.googlesource.com/1028335
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553683}
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/chrome/browser/chrome_site_per_process_browsertest.cc
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/chrome/browser/ui/webui/webui_browsertest.cc
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/browser/webui/web_ui_impl.cc
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/browser/webui/web_ui_impl.h
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/common/frame_messages.h
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/public/browser/web_contents.h
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/public/test/web_contents_tester.h
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/renderer/web_ui_extension.cc
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/test/test_web_contents.cc
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/content/test/test_web_contents.h
[modify] https://crrev.com/8bdbebbfdf5c54cbbf03243f472d56c80f9da6ec/tools/metrics/histograms/histograms.xml

Project Member

Comment 19 by bugdroid1@chromium.org, Apr 25 2018

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

commit 90196c8d430d42cc63c4b3ed1205eb187152fd12
Author: Daniel Cheng <dcheng@chromium.org>
Date: Wed Apr 25 21:49:14 2018

Enforce user gesture requirement on browser side for WebUI.

WebContentsImpl now tracks the last time it received an input event that
could be considered user interaction. When the browser process receives
a WebUI message that requires a user gesture, it checks the WebContents
hosting the WebUI to make sure that the user recently interacted with
it.

This also cleans up a few incidental bits of code:
- RenderWidgetHost no longer prefilters events before notifying the
  delegate. This exposed some broken event filtering, tracked at
   https://crbug.com/827659 .
- Since the delegate method no longer prefilters input events,
  RenderWidgetHostDelegate::OnUserInteraction() is now named
  RenderWidgetHostDelegate::DidReceiveInputEvent().

TBR=isherman@chromium.org

Bug:  823864 
Change-Id: I9237052dbd325d8e578bfa904a8779533b63b2f9
Reviewed-on: https://chromium-review.googlesource.com/1028484
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553769}
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/chrome/browser/chrome_site_per_process_browsertest.cc
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/chrome/browser/ui/webui/webui_browsertest.cc
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/browser/webui/web_ui_impl.cc
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/browser/webui/web_ui_impl.h
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/public/browser/web_contents.h
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/public/test/web_contents_tester.h
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/renderer/web_ui_extension.cc
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/test/test_web_contents.cc
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/content/test/test_web_contents.h
[modify] https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12/tools/metrics/histograms/histograms.xml

Labels: Merge-Request-67
Status: Fixed (was: Started)
For now, the timeout delta is set at a fairly generous 5 seconds. Based on the Tabs.TimeSinceLastInteraction UMA measurement, we may tweak this downwards in the future.

(We may want to consider merging this to M-66 as well, if there's any chance of a respin)
Project Member

Comment 21 by sheriffbot@chromium.org, Apr 27 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Pls apply appropriate OSs label. Thank you.
Labels: OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
The commit I'd like to merge: https://crrev.com/90196c8d430d42cc63c4b3ed1205eb187152fd12
Labels: Restrict-View-SecurityTeam
Labels: -Type-Bug Type-Bug-Security
Privilege escalation by opening an exe from chrome://downloads has been used as the final stage of many full chain exploits (two in the last month). We should break that last step.
Cc: awhalley@chromium.org
Thank you dcheng@.

+awhalley@ (Security TPM) for merge review.
Labels: Security_Impact-Stable Security_Severity-High
Tagging as high as this allows sandbox escape from a renderer (albeit a privileged WebUI renderer).
govind - good for 67
Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 branch 3396 based on comment @29. Please merge ASAP. Thank you.
Project Member

Comment 31 by bugdroid1@chromium.org, Apr 27 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4599909a5033ec68fc116771cc382895605ca88

commit d4599909a5033ec68fc116771cc382895605ca88
Author: Daniel Cheng <dcheng@chromium.org>
Date: Fri Apr 27 22:36:09 2018

Enforce user gesture requirement on browser side for WebUI.

WebContentsImpl now tracks the last time it received an input event that
could be considered user interaction. When the browser process receives
a WebUI message that requires a user gesture, it checks the WebContents
hosting the WebUI to make sure that the user recently interacted with
it.

This also cleans up a few incidental bits of code:
- RenderWidgetHost no longer prefilters events before notifying the
  delegate. This exposed some broken event filtering, tracked at
   https://crbug.com/827659 .
- Since the delegate method no longer prefilters input events,
  RenderWidgetHostDelegate::OnUserInteraction() is now named
  RenderWidgetHostDelegate::DidReceiveInputEvent().

TBR=dcheng@chromium.org, isherman@chromium.org

(cherry picked from commit 90196c8d430d42cc63c4b3ed1205eb187152fd12)

Bug:  823864 
Change-Id: I9237052dbd325d8e578bfa904a8779533b63b2f9
Reviewed-on: https://chromium-review.googlesource.com/1028484
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#553769}
Reviewed-on: https://chromium-review.googlesource.com/1033964
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#364}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/chrome/browser/chrome_site_per_process_browsertest.cc
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/chrome/browser/ui/webui/webui_browsertest.cc
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/browser/webui/web_ui_impl.cc
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/browser/webui/web_ui_impl.h
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/public/browser/web_contents.h
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/public/test/web_contents_tester.h
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/renderer/web_ui_extension.cc
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/test/test_web_contents.cc
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/content/test/test_web_contents.h
[modify] https://crrev.com/d4599909a5033ec68fc116771cc382895605ca88/tools/metrics/histograms/histograms.xml

Project Member

Comment 32 by sheriffbot@chromium.org, Apr 28 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Release-0-M67
Project Member

Comment 34 by sheriffbot@chromium.org, Aug 4

Labels: -Restrict-View-SecurityNotify allpublic
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