New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
User never visited
Closed: Sep 2014
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Security

Blocking:
issue 410113



Sign in to add a comment
Security: Page can run arbitrary code in the context of a UserGestureIndicator
Project Member Reported by rbyers@chromium.org, Aug 28 2014 Back to list
VERSION
Chrome Version: 38.0.2125.3 + dev
Operating System: Android

VULNERABILITY DETAILS
We rely on UserGestureIndicator to restrict what page JavaScript can do outside the context of a user actually interacting with the page.  It appears to me that a recent change on Chrome Android means that pages could run arbitrary code on page load in the context of a UserGestureIndicator.  I'm not sure how bad this is, but I know it's a security concern.

In particular, as of https://chrome-internal-review.googlesource.com/#/c/171865/14/java/apps/chrome/src/com/google/android/apps/chrome/dom_distiller/ReaderModeManager.java we apparently execute the JS here on every page load: https://code.google.com/p/chromium/codesearch#chromium/src/components/dom_distiller/core/javascript/is_distillable_trigger.js.

But WebLocalFrameImpl::executeScriptAndReturnValue takes a fake UserGestureIndicator:  https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp&l=761

    // FIXME: This fake user gesture is required to make a bunch of pyauto
    // tests pass. If this isn't needed in non-test situations, we should
    // consider removing this code and changing the tests.
    // http://code.google.com/p/chromium/issues/detail?id=86397

I believe this means any page could simply add a hook into any of the methods we're calling here (eg. document.querySelectorAll) and circumvent the UserGestureIndicator system.

I think the right fix is probably to try removing this fake UserGestureIndicator.  If it's really only necessary for pyauto tests then we should be fine (since pyauto was deprecated long ago).  But we may find other tests that are indirectly relying on this.

Alternately we may want to consider reverting the reader change from clank which invokes JS in this way on page load.

We provbably also need a test cases (including one that runs on internal clank bots) which verifies that the simple act of loading a page doesn't cause a UserGestureIndicator to be taken.

Zeeshan is looking at this (as this is the root cause of his  issue 406336 ), but we're happy for someone with more security expertise to take this over if desired.

 
Comment 1 by rbyers@chromium.org, Aug 28 2014
Blocking: chromium:406336
Comment 2 by rbyers@chromium.org, Aug 28 2014
Cc: nyquist@chromium.org
Comment 3 by klo...@chromium.org, Aug 28 2014
Cc: cjhopman@chromium.org yfried...@chromium.org
I would like to see us to remove fake user gesture. On mobile, possible CrOS, we don't trigger certain action, e.g. bring up keyboard, url override, unless it is user gesture. 
Comment 4 by rbyers@chromium.org, Aug 28 2014
Cc: jdduke@chromium.org
Comment 5 by wfh@chromium.org, Aug 28 2014
Labels: Security_Severity-High Security_Impact-Stable
based on the comments above I'm assigning this high severity and impact stable.
I have a patch in the works to only set this for tests, should be up for review soon.
Project Member Comment 8 by bugdroid1@chromium.org, Aug 29 2014
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=181075

------------------------------------------------------------------
r181075 | zeeshanq@chromium.org | 2014-08-29T05:01:17.583656Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/WebLocalFrameImpl.cpp?r1=181075&r2=181074&pathrev=181075
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/WebFrame.cpp?r1=181075&r2=181074&pathrev=181075
   M http://src.chromium.org/viewvc/blink/trunk/public/web/WebFrame.h?r1=181075&r2=181074&pathrev=181075

A recent CL to add reader mode for clank started triggering this codepath on
load and was interfering with how the virtual keyboard handles programmatic
focus() calls. This rogue UGI seems to have been added to get pyauto tests to
pass, we should be able to only set this for tests.

This is part 1 of 3, will be followed by:
https://codereview.chromium.org/518583003/

BUG= 408426 , 407643 

Review URL: https://codereview.chromium.org/516753002
-----------------------------------------------------------------
Cc: jam...@chromium.org
Security issue aside, this code isn't going to work very well.  It's running javascript in the context of the page but any of the functions it calls could be redefined to do something different (and often are by common misguided frameworks).  If we care about this working, we should run the script in an isolated world.  It appears to only want to scrape the DOM and not touch any actual javascript objects in the page.

Running this script in a clean isolated world would also fix the security bug.
Running the script in isolation would be ideal, but I think the UGI issue would still persist if something else is added later to trigger a script execution. We should follow up on both.
Comment 12 by ti...@chromium.org, Aug 29 2014
Permit me a silly question maybe: if pyauto was "deprecated long ago", why cannot we just remove one line that sets the fake gesture?
There's about two dozen browser tests that rely on the fake user gesture being present. This would be the smallest fix to merge into M38 but then later we can clean up the tests.
This is very fragile.  Script executed in this fashion can do basically anything that the page wants, so any code in the browser that depends on this doing anything in particular is going to get the wrong answer sometimes.

Having a magic bit that you have to set or you have security holes is also fragile.

How about this instead:
1.) We add a setting (command line flag or something) that controls whether executing script in the main page triggers a fake UGI.  We set this in the only tests that need it.  We don't provide any way to set this in a production binary.
2.) Concurrently, we audit script like this to see if it needs to run in the main world.  I strongly suspect the vast majority does not and should run in an isolate.  This particular piece of script definitely should.

(1) will fix the UGI security hole and is mergable and is a lot safer than having a bool per call.
Option 1. sounds like a good path forward, but I feel it would be a lot more effort than the current patch to Merge into M38. I'm up for following up with this.

Otherwise, we can turn off / rollback the reader CL and work on this in trunk.
I don't think the patch in https://codereview.chromium.org/518583003/ is safe enough to consider landing anywhere.
Comment 17 by ti...@chromium.org, Aug 29 2014
I admit being a noogler so the following maybe quite naive, but it seems that the straightforward way would be:

1. Revert the Zeeshan gesture CL, not the Reader Mode CL. As I mentioned in https://code.google.com/p/chromium/issues/detail?id=407643#c19, the gesture CL does not determine the user intention correctly while we have a fake gesture.
2. Clean the tests
3. Remove the fake gesture
4. Reapply the Zheeshan CL
I don't think that would work, the focus() CL only looks at the UGI to bring up the keyboard. Even without that, the Reader would be executing script at load in an unsafe context.

Comment 19 by ti...@chromium.org, Aug 29 2014
I see, I missed the security issue altogether.
 Issue 408616  tracks changing this new dom distiller code to not rely on injecting javascript into the page.  Perhaps the priority of that should be raised (or if we want to still run JS but in an isolated world, then maybe that's OK too).

But regardless I don't see why we'd want to leave this dangerous code path around.  There may be a number of different ways (including, like this, in downstream clank) the browser can invoke JavaScript, and if we're taking a UserGestureIndicator whenever that happens then the UGI system is essentially useless.

James, are you saying that you feel https://codereview.chromium.org/518583003/ makes this situation worse?  There's much more I think should be done here, but I think the top priority is to ensure this fake UserGestureIndicator codepath stops getting hit in production scenarios (ideally eliminated entirely), and I think we should plan to do that in a constrained way we can safely merge back to M38.  Security folks, any input here - am I over-reacting?
The UGI issue clearly needs to be fixed. I'll address some of the other points:

Should we be running this script (and others like it) in the page's context? Well, on ios, afaik, we don't really have much choice and so we need to be able to handle the difficulties of that (like the fact that we can't trust the value returned). I'm less worried about the page breaking us than I am about us breaking the page (like if they redefined querySelectorAll to delete the page :/). We've had a long-standing goal of running this stuff (or rather our main script, not this one) in an isolated world where we can (on iOS, again, we can't). It would be nice if it were as easy to do the safe correct isolated world thing as it is to do the unsafe thing.

I believe that this particular short script we will be rewriting to native rather than javascript (at least for non-iOS platforms).
I'm saying we should stop setting the fake UGI for any script invocations at all unless there's a setting that is only set in the browser tests (or whatever tests) that need it.  Having parallel APIs, one which is safe and one which isn't, that differ only by a bool parameter at the end is a recipe for disaster.

I don't see what iOS has to do with any of this.  We aren't setting a UGI on iOS since we can't (we don't control the script running).  We don't really have a sandbox on iOS so it seems a bit silly to worry about this too much.
The bool parameter is only for the IPC though, the code interacts with the wrapper above the ipc and that doesn't expose it (ExecuteJavascript/ExecuteJavascriptForTests).
#22: I thought I was clear that I think the UGI issue needs to be fixed and everything in #21 after the first line was addressing the other points brought up.

I don't think that running this script in an isolated world is the correct way to fix the UGI issue, unless we are going to go through and audit all the other uses of this function (I'd be particularly worried about Android's accessibility support).

Basically, I agree with your #14's (1) that the UGI should be controlled by a command line flag or some such that is hard to get wrong.
OK, then let's replace the bool parameter with something more bulletproof and go forward with that.

Isolated worlds doesn't fix the UGI issue.  It does address the issue of getting the wrong answer in script that depends on getting the right answer, however, which IMO is also an important thing to consider.
I agree finding something more bulletproof is better.  Whatever we do in the browser (short of removing this need entirely - which I suspect could be done without too much pain in M39), we'll still need some IPC and method call.  Is using a flag to invoke the method that tells the renderer to enable this unsafe path any better than just having tests call an unsafe method (marked clearly somehow)?  Eg.  What if we split this "execute JavaScript" IPC into 2, one clearly marked as unsafe for testing.  Then we can eliminate the bool parameter entirely.  This seems a little less dangerous to me than adding a separate IPC for enabling an unsafe mode globally.
I think one IPC and a flag set by the test harness is safer.  Having two methods one which does something completely unsafe is very hard to get right from the callsite, especially when it's not obvious from the callsite what the implications of getting the wrong one is.
If we have one IPC and switch in chromium, we still need some way to tell blink, which presumably will boil down to choosing to call one of two methods (or changing a WebSetting).  As long as we need some way to do this, I don't see how we can avoid your concern of "having two methods ..." somewhere.

Are you just saying that you want to push that two method choice down as far as possible in chromium (i.e. only at the current WebLocalFrame::executeScriptAndReturnValueForTests API)?  We can certainly do that - although I don't personally see what it buys us really.  Perhaps we should make this method name scarrier, eg. WebLocalFrame::unsafeForTestingOnly_executeScriptAndReturnValueWithinUserGestureIndicator.

So, just to be clear, we'd add a content_switch (since it's RenderFrameImpl that needs to read this), and then set that switch from any test which relies on it, right?  Did you have some mechanism in mind to "ensure production code could never set this switch"?  I don't see how to do that without violating the content layer abstraction.

By the way, note that production code could always achieve this same effect (without using any scarily named methods) by first sending a fake mousedown event, then executing script.  So it's not like we can code our way out of code doing stupid things here.  It seems like production code calling a "unsafeForTestingOnly..." method is one of the least likely ways someone could screw this up again. 
Blocking: -chromium:406336
Blocking: chromium:410113
James, I've updated the CL to have separate IPCs for production/testing. Would this me a more acceptable approach to you?

We need to keep in mind that our long term goal should be to update the tests so that they don't rely on a fake UGI. So this is mostly throwaway work until we simulate input events in tests.

If we are to put a lot of effort in updating the tests (which we would have to when adding a flag) I feel it should be directed towards removing their dependence on the UGI.
Project Member Comment 32 by bugdroid1@chromium.org, Sep 4 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453

commit 3454e9cfacdab576d72257e47b0d14d9a7b95453
Author: zeeshanq <zeeshanq@chromium.org>
Date: Thu Sep 04 21:30:28 2014

Don't take a fake UGI every time we execute Javascript.

This change will make it so that a fake UserGestureIndicator is not set for
every javascript execution request triggered by the browser. This was
interfering with how the virtual keyboard handles programmatic focus() calls on
load.

This is part 2 of 3, depends on blink change:
https://codereview.chromium.org/516753002

BUG= 408426 ,  406336 

Review URL: https://codereview.chromium.org/518583003

Cr-Commit-Position: refs/heads/master@{#293339}

[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/chrome/browser/extensions/extension_messages_apitest.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/chrome/browser/extensions/webstore_installer_test.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/chrome/browser/prerender/prerender_browsertest.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/chrome/browser/ui/browser_browsertest.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/chrome/test/base/tracing_browsertest.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/content/browser/frame_host/render_frame_host_impl.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/content/common/frame_messages.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/content/public/browser/render_frame_host.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/content/public/test/browser_test_utils.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/content/renderer/render_frame_impl.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/3454e9cfacdab576d72257e47b0d14d9a7b95453/content/renderer/render_frame_impl.h

Labels: -M-38 M-39
Reader mode has been disabled in M38 so this is no longer blocking it, moving to M39.
Project Member Comment 35 by bugdroid1@chromium.org, Sep 9 2014
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=181660

------------------------------------------------------------------
r181660 | zeeshanq@chromium.org | 2014-09-09T17:41:17.561600Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/WebLocalFrameImpl.cpp?r1=181660&r2=181659&pathrev=181660

Remove fake UserGestureIndicator from WebLocalFrameImpl

This CL removes the old code needed to make the blink roll happen properly.

This is part 3 of 3, depends on:
https://codereview.chromium.org/518583003/

BUG= 408426 ,  406336 

Review URL: https://codereview.chromium.org/518633002
-----------------------------------------------------------------
Project Member Comment 36 by bugdroid1@chromium.org, Sep 10 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e55d685552542d6d6e323f778369367311277c0

commit 9e55d685552542d6d6e323f778369367311277c0
Author: zeeshanq <zeeshanq@chromium.org>
Date: Wed Sep 10 02:04:05 2014

Enable ExtensionApiTest.MessagingUserGesture test

This test was disabled temporarily while changing how the fake
UserGestureIndicator is set for tests.

Depends on blink patch:
https://codereview.chromium.org/518633002/

BUG= 408426 ,  406336 
NOTRY=true
TBR=jochen@chromium.org

Review URL: https://codereview.chromium.org/557153002

Cr-Commit-Position: refs/heads/master@{#294076}

[modify] https://chromium.googlesource.com/chromium/src.git/+/9e55d685552542d6d6e323f778369367311277c0/chrome/browser/extensions/extension_messages_apitest.cc

Status: Fixed
Project Member Comment 38 by ClusterFuzz, Sep 10 2014
Labels: -Restrict-View-SecurityTeam -M-39 Merge-Triage M-37 M-38 Restrict-View-SecurityNotify
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz
Cc: infe...@chromium.org matthewyuan@chromium.org
Labels: -M-37 -M-38 M-39
inferno@ - assuming that this sizable change can wait unti M-39 based on c#33. It's currently marked as a release blocker for stable.

If you want this in M38, please update labels and request a merge. 
Labels: -Merge-Triage Release-0-M39
Project Member Comment 41 by ClusterFuzz, Dec 17 2014
Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member Comment 42 by sheriffbot@chromium.org, Oct 1 2016
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
Project Member Comment 43 by sheriffbot@chromium.org, Oct 2 2016
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
Labels: allpublic
Sign in to add a comment