Pass user gesture bit when chrome handles an intent fired by itself

Pass user gesture bit when chrome handles an intent fired by itself

It is possible for user to choose chrome when url overriding results in an intent chooser.
In this case, we should let everything work as if nothing has happened.
However, intent handling will result in a new url request.
And that causes chrome to lose the user gesture bit, and introduces some side effects.
For example, when clicking a pdf link, the pdf will be auto opened by docs app.
However, if Adobe pdf reader is installed, an intent chooser will be shown when clicking the pdf link.
If user chooses chrome to handle the intent, the pdf will be downloaded, but not auto opened due to the missing user gesture

BUG=512633
Committed: https://crrev.com/976d4d9a2735bdc11e5a641c84b6382566d48f1d
Cr-Commit-Position: refs/heads/master@{#342388}

[qinmin, 2015-07-22 00:29:49]

qinmin@chromium.org changed reviewers:
+ jochen@chromium.org, mariakhomenko@chromium.org

[qinmin, 2015-07-22 00:29:50]

PTAL

[jochen (gone - plz use gerrit), 2015-07-22 09:45:41]

the bug id you reference in the CL description appears to be unrelated?

It looks like we'd set the user gesture bit on any intent. However, any other
android app can fire an intent at chrome, without any user interaction, so that
seems wrong.

[qinmin, 2015-07-22 13:52:35]

On 2015/07/22 09:45:41, jochen wrote:
> the bug id you reference in the CL description appears to be unrelated?
> 
> It looks like we'd set the user gesture bit on any intent. However, any other
> android app can fire an intent at chrome, without any user interaction, so
that
> seems wrong.

fixed the bug id. This CL only sets the user gesture if the intent is fired by
chrome itself.  On android we do that by comparing the external app id that
launches the intent with the current app id, the logic is in
ChromeLauncherActivity and ChromeTabbedActivity.

[qinmin, 2015-07-22 22:04:48]

qinmin@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Maria, 2015-07-22 22:18:32]

On 2015/07/22 22:04:49, qinmin wrote:

So I think this works correctly for existing cases, I am a bit worried about two
things:
1) Going forward someone could use EXTRA_APPLICATION_ID to send an intent back
to Chrome that did not come from a user-triggered action
2) Any app can actually specify the extra EXTRA_APPLICATION_ID to pretend they
are Chrome, so if that somehow can result in bypassing security, that might be
an issue.

For 1) we could just add an extra that would specify user gesture bit explicitly
in the cases it's needed.
For 2) if that's a concern (which I am not quite sure about), then we need to
sign/verify the intents with
IntentHandler.addTrustedIntentExtras()/IntentHandler.isIntentChromeOrFirstParty

[qinmin, 2015-07-23 21:58:16]

Talked with Maria offline and added an Extra for the user gesture. For
IntentHandler.addTrustedIntentExtras(), since we cannot expose the trusted
intent extra to another app, calling this method is unsafe in this scenario.
If an app trying to spoof the EXTRA_APPLICATION_ID, all it can do is to launch
chrome, download and auto open a pdf file. And apps can do that by themselves
rather than going through chrome. For example, an app can spoof the
EXTRA_APPLICATION_ID and let chrome open an phishing page, and when user hit
anywhere on the page, it uses that user gesture to do downloading. So there
doesn't seem to be any serious concerns.

[jochen (gone - plz use gerrit), 2015-07-24 11:34:56]

On 2015/07/23 at 21:58:16, qinmin wrote:
> Talked with Maria offline and added an Extra for the user gesture. For
IntentHandler.addTrustedIntentExtras(), since we cannot expose the trusted
intent extra to another app, calling this method is unsafe in this scenario.
> If an app trying to spoof the EXTRA_APPLICATION_ID, all it can do is to launch
chrome, download and auto open a pdf file. And apps can do that by themselves
rather than going through chrome. For example, an app can spoof the
EXTRA_APPLICATION_ID and let chrome open an phishing page, and when user hit
anywhere on the page, it uses that user gesture to do downloading. So there
doesn't seem to be any serious concerns.

that's not true, e.g., the external app wouldn't have access to the credentials
(cookies etc..) that chrome has, so it can't necessarily get the pdf iteslf.

also, the user gesture thing is used for other systems as well, they'd also be
affected by this.

[qinmin, 2015-07-24 23:06:23]

On 2015/07/24 11:34:56, jochen wrote:
> On 2015/07/23 at 21:58:16, qinmin wrote:
> > Talked with Maria offline and added an Extra for the user gesture. For
> IntentHandler.addTrustedIntentExtras(), since we cannot expose the trusted
> intent extra to another app, calling this method is unsafe in this scenario.
> > If an app trying to spoof the EXTRA_APPLICATION_ID, all it can do is to
launch
> chrome, download and auto open a pdf file. And apps can do that by themselves
> rather than going through chrome. For example, an app can spoof the
> EXTRA_APPLICATION_ID and let chrome open an phishing page, and when user hit
> anywhere on the page, it uses that user gesture to do downloading. So there
> doesn't seem to be any serious concerns.
> 
> that's not true, e.g., the external app wouldn't have access to the
credentials
> (cookies etc..) that chrome has, so it can't necessarily get the pdf iteslf.
> 
> also, the user gesture thing is used for other systems as well, they'd also be
> affected by this.

Hi, jochen, what about this:
1. When launching an intent, Chrome signs the URL and user gesture bit along
with some random numbers, and put that in the extra. Chrome also do some book
keeping to remember that there is a pending intent.
2. If user chooses chrome to open the intent, Chrome will verify the signature
with the URL/gesture bit in the intent, and proceed the request if it matches.
3. Whenever chrome handles an intent, it will clean up the book keeping, so that
another app cannot use the same intent over and over.
It is possible that another app can intercept the intent through intent chooser,
and relaunches chrome using the intent. 
However, chrome intent is publicly viewable (adb logcat will show it) and other
apps cannot modify the gesture/URL in this case. 
Additionally, an intent is functionally an user action so relaying an intent
through another app (intent chooser itself intercepts the intent) seems ok. Does
that sounds good to you?

[jochen (gone - plz use gerrit), 2015-07-27 09:01:37]

On 2015/07/24 at 23:06:23, qinmin wrote:
> On 2015/07/24 11:34:56, jochen wrote:
> > On 2015/07/23 at 21:58:16, qinmin wrote:
> > > Talked with Maria offline and added an Extra for the user gesture. For
> > IntentHandler.addTrustedIntentExtras(), since we cannot expose the trusted
> > intent extra to another app, calling this method is unsafe in this scenario.
> > > If an app trying to spoof the EXTRA_APPLICATION_ID, all it can do is to
launch
> > chrome, download and auto open a pdf file. And apps can do that by
themselves
> > rather than going through chrome. For example, an app can spoof the
> > EXTRA_APPLICATION_ID and let chrome open an phishing page, and when user hit
> > anywhere on the page, it uses that user gesture to do downloading. So there
> > doesn't seem to be any serious concerns.
> > 
> > that's not true, e.g., the external app wouldn't have access to the
credentials
> > (cookies etc..) that chrome has, so it can't necessarily get the pdf iteslf.
> > 
> > also, the user gesture thing is used for other systems as well, they'd also
be
> > affected by this.
> 
> Hi, jochen, what about this:
> 1. When launching an intent, Chrome signs the URL and user gesture bit along
with some random numbers, and put that in the extra. Chrome also do some book
keeping to remember that there is a pending intent.
> 2. If user chooses chrome to open the intent, Chrome will verify the signature
with the URL/gesture bit in the intent, and proceed the request if it matches.
> 3. Whenever chrome handles an intent, it will clean up the book keeping, so
that another app cannot use the same intent over and over.
> It is possible that another app can intercept the intent through intent
chooser, and relaunches chrome using the intent. 
> However, chrome intent is publicly viewable (adb logcat will show it) and
other apps cannot modify the gesture/URL in this case. 
> Additionally, an intent is functionally an user action so relaying an intent
through another app (intent chooser itself intercepts the intent) seems ok. Does
that sounds good to you?

can you please put this into a doc and run it by chrome-security for review?

[qinmin, 2015-08-04 21:21:46]

PTAL again after security review. 

Security review doc:
https://docs.google.com/a/google.com/document/d/1nSqfOhUxf3vBHKrkHZZz7EwRqeIP...

added a new class to generate token for intent launched with user gesture, and
will use the token to verify if a received intent has user gesture

[Maria, 2015-08-05 01:41:44]

https://codereview.chromium.org/1243253004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/IntentWithGesturesHandler.java
(right):

https://codereview.chromium.org/1243253004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/IntentWithGesturesHandler.java:18:
* that has user gesture. A previously launched inten with user gesture will be
inten->intent and remove "be"

https://codereview.chromium.org/1243253004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/IntentWithGesturesHandler.java:60:
mUri = intent.getData();
I would use IntentHandler.getUrlFromIntent() here and in the check below for
consistency with our other code.

https://codereview.chromium.org/1243253004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/IntentWithGesturesHandler.java:71:
byte[] bytes = intent.getByteArrayExtra(EXTRA_USER_GESTURE_TOKEN);
Add an accessor to IntentUtils.safeGet... collection. This protects Chrome from
external applications sending us bad intents

[qinmin, 2015-08-05 20:22:26]

https://codereview.chromium.org/1243253004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/IntentWithGesturesHandler.java
(right):

https://codereview.chromium.org/1243253004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/IntentWithGesturesHandler.java:18:
* that has user gesture. A previously launched inten with user gesture will be
On 2015/08/05 01:41:44, Maria wrote:
> inten->intent and remove "be"

Done.

https://codereview.chromium.org/1243253004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/IntentWithGesturesHandler.java:60:
mUri = intent.getData();
On 2015/08/05 01:41:44, Maria wrote:
> I would use IntentHandler.getUrlFromIntent() here and in the check below for
> consistency with our other code.

Done.

https://codereview.chromium.org/1243253004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/IntentWithGesturesHandler.java:71:
byte[] bytes = intent.getByteArrayExtra(EXTRA_USER_GESTURE_TOKEN);
On 2015/08/05 01:41:44, Maria wrote:
> Add an accessor to IntentUtils.safeGet... collection. This protects Chrome
from
> external applications sending us bad intents

Done.

[Maria, 2015-08-05 21:29:16]

Looks good, just one final question.

https://codereview.chromium.org/1243253004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/1243253004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:287:
IntentWithGesturesHandler.getInstance().clear();
Does this work in document mode? I would have thought we'd get window visibility
change on the window creating the intent right after it's sent and before it's
processed by ChromeLauncherActivity.

[qinmin, 2015-08-05 21:47:00]

https://codereview.chromium.org/1243253004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/1243253004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:287:
IntentWithGesturesHandler.getInstance().clear();
On 2015/08/05 21:29:16, Maria wrote:
> Does this work in document mode? I would have thought we'd get window
visibility
> change on the window creating the intent right after it's sent and before it's
> processed by ChromeLauncherActivity.

Yes, this works in document mode. Popping up the intent chooser won't change
window visibility, for both document and non-document mode.

[Maria, 2015-08-05 23:21:33]

lgtm

[jochen (gone - plz use gerrit), 2015-08-06 14:25:21]

lgtm

[qinmin, 2015-08-06 16:55:17]

qinmin@chromium.org changed reviewers:
+ tsepez@chromium.org

[qinmin, 2015-08-06 16:55:17]

+tsepez for content/common/ OWNER

[Tom Sepez, 2015-08-06 17:00:58]

On 2015/08/06 16:55:17, qinmin wrote:
> +tsepez for content/common/ OWNER

Messages LGTM.  There's probably not much cost in always marshalling a 0 / FALSE
pair for the two variables in the non-android case, the code might be cleaner if
the ifdefs were removed in most places in favor of a comment about this not
being meaningful other than on android -- or it might be much less clear to the
reader that way.  Your call.

[qinmin, 2015-08-07 04:30:16]

On 2015/08/06 17:00:58, Tom Sepez wrote:
> On 2015/08/06 16:55:17, qinmin wrote:
> > +tsepez for content/common/ OWNER
> 
> Messages LGTM.  There's probably not much cost in always marshalling a 0 /
FALSE
> pair for the two variables in the non-android case, the code might be cleaner
if
> the ifdefs were removed in most places in favor of a comment about this not
> being meaningful other than on android -- or it might be much less clear to
the
> reader that way.  Your call.

Ok, will submit the CL as it is, and do a clean up on ifdefs later on

[qinmin, 2015-08-07 04:30:22]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2015-08-07 04:30:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243253004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243253004/60001

[commit-bot: I haz the power, 2015-08-07 05:41:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 05:41:07]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[qinmin, 2015-08-07 16:36:23]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-08-07 16:36:23]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jochen@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/1243253004/#ps80001
(title: "fix findbugs warning")

[commit-bot: I haz the power, 2015-08-07 16:36:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243253004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243253004/80001

[commit-bot: I haz the power, 2015-08-07 16:52:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 16:52:03]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[qinmin, 2015-08-07 16:57:02]

Patchset #5 (id:80001) has been deleted

[qinmin, 2015-08-07 16:57:11]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-08-07 16:57:12]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jochen@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/1243253004/#ps100001
(title: "fix findbugs warning")

[commit-bot: I haz the power, 2015-08-07 16:57:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243253004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243253004/100001

[commit-bot: I haz the power, 2015-08-07 18:32:49]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-08-07 18:33:24]

Patchset 5 (id:??) landed as
https://crrev.com/976d4d9a2735bdc11e5a641c84b6382566d48f1d
Cr-Commit-Position: refs/heads/master@{#342388}