[Geolocation] Attach user gesture indicator to permission request call.

[Geolocation] Attach user gesture indicator to permission request call.

This enables permissions bubble policies that rely on user gesture signals for deciding when/how to show bubble UI for permission requests.

BUG=332115
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262963

[Greg Billock, 2014-03-05 20:01:59]

ready for a look

[jamesr, 2014-03-05 20:41:01]

This bool doesn't appear to be used by anything and there is no bug describing
the context.  What's the plan here?

[Greg Billock, 2014-03-05 20:59:48]

We're switching to bubbles for permissions requests (crbug.com/332115).
This is groundwork for policy to be defined there which will change the
willingness of the overall permissions policy as to whether and when to
show request bubbles. (user gesture will show them immediately, and when
they'd otherwise be suppressed).


On Wed, Mar 5, 2014 at 12:41 PM, <jamesr@chromium.org> wrote:

> This bool doesn't appear to be used by anything and there is no bug
> describing
> the context.  What's the plan here?
>
> https://codereview.chromium.org/185813004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2014-03-05 21:01:22]

Please put that in the description.

This patch by itself doesn't seem to do anything useful.

[Greg Billock, 2014-03-05 22:56:54]

On 2014/03/05 21:01:22, jamesr wrote:
> Please put that in the description.

done

> This patch by itself doesn't seem to do anything useful.

Correct. I've been splitting up the code required to service the permission
bubble API change to make more digestible review chunks.

[Michael van Ouwerkerk, 2014-03-06 11:24:38]

This change looks trivial, so it should be fine. I've added the BUG= line.

Please run the try bots.

Could you also provide a link to any followup CLs? I'd be happy to review those
as well.

[jamesr, 2014-03-06 21:37:43]

lgtm

https://codereview.chromium.org/185813004/diff/1/content/browser/geolocation/...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/185813004/diff/1/content/browser/geolocation/...
content/browser/geolocation/geolocation_dispatcher_host.cc:164: bool
user_gesture) {
please add a TODO() or something here indicating that you intend to use this
flag for something.  As it is, this patch adds a flag that goes nowhere and thus
the entire patch appears to be dead code.  If you plan to extend this later
that's fine but you need to leave an indication of that or another developer
could look at the code, rightly determine that it's dead, and delete it.

[Greg Billock, 2014-03-08 17:15:32]

https://codereview.chromium.org/185813004/diff/1/content/browser/geolocation/...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/185813004/diff/1/content/browser/geolocation/...
content/browser/geolocation/geolocation_dispatcher_host.cc:164: bool
user_gesture) {
On 2014/03/06 21:37:43, jamesr wrote:
> please add a TODO() or something here indicating that you intend to use this
> flag for something.  As it is, this patch adds a flag that goes nowhere and
thus
> the entire patch appears to be dead code.  If you plan to extend this later
> that's fine but you need to leave an indication of that or another developer
> could look at the code, rightly determine that it's dead, and delete it.

I pushed this out to the browser side and added the todo there.

[Michael van Ouwerkerk, 2014-03-10 14:02:57]

lgtm

[Greg Billock, 2014-03-14 08:34:10]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-14 08:34:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185813004/60001

[Greg Billock, 2014-03-14 08:35:21]

The CQ bit was unchecked by gbillock@chromium.org

[Greg Billock, 2014-03-24 21:38:41]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-24 21:41:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185813004/100001

[commit-bot: I haz the power, 2014-03-24 22:26:27]

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

[commit-bot: I haz the power, 2014-03-24 22:26:28]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Greg Billock, 2014-03-24 23:12:05]

On 2014/03/24 22:26:28, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

+joi for content/
+fsamuel for browser_plugin
+mkosiba for android_webview

[Jói, 2014-03-25 10:51:03]

I'm not an owner for all of //content, just //content/public (which LGTM). See
//content/OWNERS.

[mkosiba (inactive), 2014-03-25 11:24:36]

android_webview/ LGTM

[Fady Samuel, 2014-03-25 14:24:53]

browser_plugin lgtm

[Greg Billock, 2014-03-25 17:08:47]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-25 17:08:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185813004/100001

[commit-bot: I haz the power, 2014-03-25 17:52:31]

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

[commit-bot: I haz the power, 2014-03-25 17:52:32]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Greg Billock, 2014-03-25 18:03:47]

On 2014/03/25 17:52:32, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

+palmer for messages file

[Greg Billock, 2014-03-27 22:32:12]

On 2014/03/25 18:03:47, Greg Billock wrote:
> On 2014/03/25 17:52:32, I haz the power (commit-bot) wrote:
> > Retried try job too often on chromium_presubmit for step(s) presubmit
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
> 
> +palmer for messages file

ping

[palmer, 2014-03-28 00:35:05]

https://codereview.chromium.org/185813004/diff/100001/content/renderer/geoloc...
File content/renderer/geolocation_dispatcher.cc (right):

https://codereview.chromium.org/185813004/diff/100001/content/renderer/geoloc...
content/renderer/geolocation_dispatcher.cc:94:
blink::WebUserGestureIndicator::isProcessingUserGesture()));
The trouble is, we can't trust renderers to tell the truth about this. We need
for the browser process to do the check before sending geolocation (or other)
information to a renderer.

http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....

[Greg Billock, 2014-03-28 15:52:51]

https://codereview.chromium.org/185813004/diff/100001/content/renderer/geoloc...
File content/renderer/geolocation_dispatcher.cc (right):

https://codereview.chromium.org/185813004/diff/100001/content/renderer/geoloc...
content/renderer/geolocation_dispatcher.cc:94:
blink::WebUserGestureIndicator::isProcessingUserGesture()));
On 2014/03/28 00:35:05, Chromium Palmer wrote:
> The trouble is, we can't trust renderers to tell the truth about this. We need
> for the browser process to do the check before sending geolocation (or other)
> information to a renderer.
> 
>
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....

I agree, but the failure mode here is "user shown permission prompt when they
otherwise wouldn't be." And as far as I know we'd be asking the
(possibly-compromised) renderer for user gesture anyway, since we don't
correlate this in the browser process. (i.e. see the RegisterProtocolHandler
msg)

[Greg Billock, 2014-03-31 17:30:26]

On 2014/03/28 15:52:51, Greg Billock wrote:
>
https://codereview.chromium.org/185813004/diff/100001/content/renderer/geoloc...
> File content/renderer/geolocation_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/185813004/diff/100001/content/renderer/geoloc...
> content/renderer/geolocation_dispatcher.cc:94:
> blink::WebUserGestureIndicator::isProcessingUserGesture()));
> On 2014/03/28 00:35:05, Chromium Palmer wrote:
> > The trouble is, we can't trust renderers to tell the truth about this. We
need
> > for the browser process to do the check before sending geolocation (or
other)
> > information to a renderer.
> > 
> >
>
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....
> 
> I agree, but the failure mode here is "user shown permission prompt when they
> otherwise wouldn't be." And as far as I know we'd be asking the
> (possibly-compromised) renderer for user gesture anyway, since we don't
> correlate this in the browser process. (i.e. see the RegisterProtocolHandler
> msg)

To expand on this: the trust is whether the call was made with a user gesture or
not. If the renderer is compromised, it can potentially spoof the entire IPC
message, so the policy of whether a user gesture is required to show a
permission prompt or not is still up to the browser process.

[Greg Billock, 2014-03-31 23:33:24]

On 2014/03/31 17:30:26, Greg Billock wrote:
> On 2014/03/28 15:52:51, Greg Billock wrote:
> >
>
https://codereview.chromium.org/185813004/diff/100001/content/renderer/geoloc...
> > File content/renderer/geolocation_dispatcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/185813004/diff/100001/content/renderer/geoloc...
> > content/renderer/geolocation_dispatcher.cc:94:
> > blink::WebUserGestureIndicator::isProcessingUserGesture()));
> > On 2014/03/28 00:35:05, Chromium Palmer wrote:
> > > The trouble is, we can't trust renderers to tell the truth about this. We
> need
> > > for the browser process to do the check before sending geolocation (or
> other)
> > > information to a renderer.
> > > 
> > >
> >
>
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....
> > 
> > I agree, but the failure mode here is "user shown permission prompt when
they
> > otherwise wouldn't be." And as far as I know we'd be asking the
> > (possibly-compromised) renderer for user gesture anyway, since we don't
> > correlate this in the browser process. (i.e. see the RegisterProtocolHandler
> > msg)
> 
> To expand on this: the trust is whether the call was made with a user gesture
or
> not. If the renderer is compromised, it can potentially spoof the entire IPC
> message, so the policy of whether a user gesture is required to show a
> permission prompt or not is still up to the browser process.

Any more thoughts on this, Chris? I'd like to get this in the cq

[palmer, 2014-03-31 23:35:32]

If the browser is going to rely on information about the presence/absence of a
user gesture when deciding how to set UX policy, then the browser needs to
determine that in a trustworthy way (i.e. not trusting the renderer).

If the browser is not going to rely on information about the presence/absence of
a user gesture, then we don't need this at all.

[Greg Billock, 2014-04-01 15:30:06]

On 2014/03/31 23:35:32, Chromium Palmer wrote:
> If the browser is going to rely on information about the presence/absence of a
> user gesture when deciding how to set UX policy, then the browser needs to
> determine that in a trustworthy way (i.e. not trusting the renderer).
> 
> If the browser is not going to rely on information about the presence/absence
of
> a user gesture, then we don't need this at all.

Can you suggest such a way? There's several existing places where this
information flow is used, such as opening pop-ups, marking navigations, etc. The
only way we determine this currently that I'm aware of is the
WebUserGestureIndicator, which is renderer-based. I don't know that we have a
way to connect that up in the browser process which wouldn't rely on renderer
process integrity.

I'm not seeing the threat model equivalence between "spams API calls from
Javascript" and "compromises renderer process." Has it gotten so these are
roughly equivalent?

[Greg Billock, 2014-04-01 20:59:49]

On 2014/04/01 15:30:06, Greg Billock wrote:
> On 2014/03/31 23:35:32, Chromium Palmer wrote:
> > If the browser is going to rely on information about the presence/absence of
a
> > user gesture when deciding how to set UX policy, then the browser needs to
> > determine that in a trustworthy way (i.e. not trusting the renderer).
> > 
> > If the browser is not going to rely on information about the
presence/absence
> of
> > a user gesture, then we don't need this at all.
> 
> Can you suggest such a way? There's several existing places where this
> information flow is used, such as opening pop-ups, marking navigations, etc.
The
> only way we determine this currently that I'm aware of is the
> WebUserGestureIndicator, which is renderer-based. I don't know that we have a
> way to connect that up in the browser process which wouldn't rely on renderer
> process integrity.
> 
> I'm not seeing the threat model equivalence between "spams API calls from
> Javascript" and "compromises renderer process." Has it gotten so these are
> roughly equivalent?

mkosiba, the argument overran the Bind args max so I extracted an args param.
Want to look again?

[Greg Billock, 2014-04-01 21:01:15]

On 2014/04/01 20:59:49, Greg Billock wrote:
> On 2014/04/01 15:30:06, Greg Billock wrote:
> > On 2014/03/31 23:35:32, Chromium Palmer wrote:
> > > If the browser is going to rely on information about the presence/absence
of
> a
> > > user gesture when deciding how to set UX policy, then the browser needs to
> > > determine that in a trustworthy way (i.e. not trusting the renderer).
> > > 
> > > If the browser is not going to rely on information about the
> presence/absence
> > of
> > > a user gesture, then we don't need this at all.
> > 
> > Can you suggest such a way? There's several existing places where this
> > information flow is used, such as opening pop-ups, marking navigations, etc.
> The
> > only way we determine this currently that I'm aware of is the
> > WebUserGestureIndicator, which is renderer-based. I don't know that we have
a
> > way to connect that up in the browser process which wouldn't rely on
renderer
> > process integrity.
> > 
> > I'm not seeing the threat model equivalence between "spams API calls from
> > Javascript" and "compromises renderer process." Has it gotten so these are
> > roughly equivalent?
> 
> mkosiba, the argument overran the Bind args max so I extracted an args param.
> Want to look again?

+jam for content/public/browser change

[palmer, 2014-04-03 22:30:26]

On 2014/04/01 15:30:06, Greg Billock wrote:
> On 2014/03/31 23:35:32, Chromium Palmer wrote:
> > If the browser is going to rely on information about the presence/absence of
a
> > user gesture when deciding how to set UX policy, then the browser needs to
> > determine that in a trustworthy way (i.e. not trusting the renderer).
> > 
> > If the browser is not going to rely on information about the
presence/absence
> of
> > a user gesture, then we don't need this at all.
> 
> Can you suggest such a way? There's several existing places where this
> information flow is used, such as opening pop-ups, marking navigations, etc.
The
> only way we determine this currently that I'm aware of is the
> WebUserGestureIndicator, which is renderer-based. I don't know that we have a
> way to connect that up in the browser process which wouldn't rely on renderer
> process integrity.
> 
> I'm not seeing the threat model equivalence between "spams API calls from
> Javascript" and "compromises renderer process." Has it gotten so these are
> roughly equivalent?

I may be confused.

The threat I see is, a renderer compromised by some other attack (e.g. buffer
overflow in Blink) could elevate its privilege by claiming to the browser
process that the user clicked OK on a permission dialog, and then the browser
might foolishly believe that lie. Then the renderer would get geolocation (or
whatever) power.

In an ideal world, it should not be possible to write code that is vulnerable in
that way. I hope that we live in that world; if we do, then I am confused and
this CL is about applying some sort of advisory policy for convenience, rather
than attempting to create an actual security boundary. And if I am confused I
apologize. :)

[palmer, 2014-04-03 22:30:56]

+tsepez in case I am confused. He is less likely to be confused than I am.

[Greg Billock, 2014-04-03 23:26:12]

On 2014/04/03 22:30:26, Chromium Palmer wrote:
> On 2014/04/01 15:30:06, Greg Billock wrote:
> > On 2014/03/31 23:35:32, Chromium Palmer wrote:
> > > If the browser is going to rely on information about the presence/absence
of
> a
> > > user gesture when deciding how to set UX policy, then the browser needs to
> > > determine that in a trustworthy way (i.e. not trusting the renderer).
> > > 
> > > If the browser is not going to rely on information about the
> presence/absence
> > of
> > > a user gesture, then we don't need this at all.
> > 
> > Can you suggest such a way? There's several existing places where this
> > information flow is used, such as opening pop-ups, marking navigations, etc.
> The
> > only way we determine this currently that I'm aware of is the
> > WebUserGestureIndicator, which is renderer-based. I don't know that we have
a
> > way to connect that up in the browser process which wouldn't rely on
renderer
> > process integrity.
> > 
> > I'm not seeing the threat model equivalence between "spams API calls from
> > Javascript" and "compromises renderer process." Has it gotten so these are
> > roughly equivalent?
> 
> I may be confused.
> 
> The threat I see is, a renderer compromised by some other attack (e.g. buffer
> overflow in Blink) could elevate its privilege by claiming to the browser
> process that the user clicked OK on a permission dialog, and then the browser
> might foolishly believe that lie. Then the renderer would get geolocation (or
> whatever) power.
> 
> In an ideal world, it should not be possible to write code that is vulnerable
in
> that way. I hope that we live in that world; if we do, then I am confused and
> this CL is about applying some sort of advisory policy for convenience, rather
> than attempting to create an actual security boundary. And if I am confused I
> apologize. :)

Ah, yes. That would be a bad vulnerability. This differs from that in at least
three important ways:

1. The privilege in question is earlier in the process, and applies only to
whether the browser process asks the user for location permission, not to
provisioning the location itself.

2. The maximum privilege escalation (at least for this API) is simply to the
current state live in the wild -- a page can currently ask for location
permission whenever it wants. If we change that policy to require user gesture
at times after page load, a hostile page that took over the renderer process
would only escalate to the permissions the page had anyway during page load, so
there's no new threat there.

3. A post-load XSS exploit that took over the renderer process would indeed have
an escalated ability, but only to the level we have live on the wild today for
any plain vanilla XSS -- the page can ask for location information whenever it
wants, so if we change the policy, we would then have this request vulnerable
only to XSS-exploit-that-takes-over-the-renderer, whereas today this
vulnerability exists for any-XSS-exploit-at-all.

So the issue is a real one, but the overall effect is not at the critical stage
of location acquisition, and is overall, only a potential tightening of
permissions.

[palmer, 2014-04-04 18:06:05]

> Ah, yes. That would be a bad vulnerability. This differs from that in at least
> three important ways:
> 
> 1. The privilege in question is earlier in the process, and applies only to
> whether the browser process asks the user for location permission, not to
> provisioning the location itself.
> 
> 2. The maximum privilege escalation (at least for this API) is simply to the
> current state live in the wild -- a page can currently ask for location
> permission whenever it wants. If we change that policy to require user gesture
> at times after page load, a hostile page that took over the renderer process
> would only escalate to the permissions the page had anyway during page load,
so
> there's no new threat there.
> 
> 3. A post-load XSS exploit that took over the renderer process would indeed
have
> an escalated ability, but only to the level we have live on the wild today for
> any plain vanilla XSS -- the page can ask for location information whenever it
> wants, so if we change the policy, we would then have this request vulnerable
> only to XSS-exploit-that-takes-over-the-renderer, whereas today this
> vulnerability exists for any-XSS-exploit-at-all.

LGTM then. Thanks!

[Greg Billock, 2014-04-04 18:17:34]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-04-04 18:17:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185813004/250001

[commit-bot: I haz the power, 2014-04-04 18:47:40]

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

[commit-bot: I haz the power, 2014-04-04 18:47:41]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Greg Billock, 2014-04-06 18:52:33]

On 2014/04/04 18:47:41, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

ping jam for content/public/browser owners

[Tom Sepez, 2014-04-07 18:37:25]

Messages LGTM.

[jam, 2014-04-09 18:43:33]

lgtm

[Greg Billock, 2014-04-09 18:43:55]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-04-09 18:44:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185813004/250001

[commit-bot: I haz the power, 2014-04-09 19:49:39]

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

[commit-bot: I haz the power, 2014-04-09 19:49:40]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[Greg Billock, 2014-04-10 01:04:12]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-04-10 01:04:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185813004/270001

[commit-bot: I haz the power, 2014-04-10 01:54:06]

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

[commit-bot: I haz the power, 2014-04-10 01:54:07]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Greg Billock, 2014-04-10 03:11:19]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-04-10 03:11:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185813004/270001

[commit-bot: I haz the power, 2014-04-10 13:26:28]

Change committed as 262963