[Android] Add messages for Gin Java Bridge implementation

[Android] Add messages for Gin Java Bridge implementation

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

[mnaganov (inactive), 2014-04-23 14:54:58]

Hi Chris,

Please review the messages for the new Gin-based Java Bridge implementation.

Some docs are available at go/clank/engineering/legacy-webview/java-bridge

[palmer, 2014-04-25 22:37:06]

Is the implementation already in place, or coming in the future, or...?

https://codereview.chromium.org/249473002/diff/1/content/common/gin_java_brid...
File content/common/gin_java_bridge_messages.h (right):

https://codereview.chromium.org/249473002/diff/1/content/common/gin_java_brid...
content/common/gin_java_bridge_messages.h:25: // Sent from browser to renderer
to remove a Java object with the given name.
This should be merely advisory, for the benefit of the renderer, right?

The actual security/correctness comes from the fact that the browser will not
honor future requests to use this object?

https://codereview.chromium.org/249473002/diff/1/content/common/gin_java_brid...
content/common/gin_java_bridge_messages.h:29: // Sent from browser to renderer
to control enumerability of methods exposed
Does this mean we trust the renderer to not expose non-enumerable methods? That
doesn't sound secure/correct.

https://codereview.chromium.org/249473002/diff/1/content/common/gin_java_brid...
content/common/gin_java_bridge_messages.h:58: // corresponding wrapper will be
successfully created on the renderer side.
What happens if the optimistic assumption does not hold?

[mnaganov (inactive), 2014-04-28 11:38:42]

> Is the implementation already in place, or coming in the future, or...?

I have a complete implementation locally and I'm uploading it in bite-sized
chunks tagged with "BUG=355644". This change is kind of a corner stone one, as
landing it will enable me to land the code of the implementation on both browser
and renderer sides.

https://codereview.chromium.org/249473002/diff/1/content/common/gin_java_brid...
File content/common/gin_java_bridge_messages.h (right):

https://codereview.chromium.org/249473002/diff/1/content/common/gin_java_brid...
content/common/gin_java_bridge_messages.h:25: // Sent from browser to renderer
to remove a Java object with the given name.
On 2014/04/25 22:37:06, Chromium Palmer wrote:
> This should be merely advisory, for the benefit of the renderer, right?
> 
> The actual security/correctness comes from the fact that the browser will not
> honor future requests to use this object?

Yes, that's correct.

https://codereview.chromium.org/249473002/diff/1/content/common/gin_java_brid...
content/common/gin_java_bridge_messages.h:29: // Sent from browser to renderer
to control enumerability of methods exposed
On 2014/04/25 22:37:06, Chromium Palmer wrote:
> Does this mean we trust the renderer to not expose non-enumerable methods?
That
> doesn't sound secure/correct.

OK, I will make renderers to query for method names each time they need them.
Then, if inspection has never been enabled, the renderer will never have a
complete list of methods (more exactly, it will not be able to find out, whether
it actually knows all the method names).

https://codereview.chromium.org/249473002/diff/1/content/common/gin_java_brid...
content/common/gin_java_bridge_messages.h:58: // corresponding wrapper will be
successfully created on the renderer side.
On 2014/04/25 22:37:06, Chromium Palmer wrote:
> What happens if the optimistic assumption does not hold?

Then the renderer sends this message. I'll try to rephrase the doc to make this
more clear.

[palmer, 2014-04-28 21:51:31]

> I have a complete implementation locally and I'm uploading it in bite-sized
> chunks tagged with "BUG=355644". This change is kind of a corner stone one, as
> landing it will enable me to land the code of the implementation on both
browser
> and renderer sides.

OK, makes sense.

> > This should be merely advisory, for the benefit of the renderer, right?
> > 
> > The actual security/correctness comes from the fact that the browser will
not
> > honor future requests to use this object?
> 
> Yes, that's correct.

Cool, thanks. I assume from this answer that the Java methods execute in the
browser process, not the renderer process?

> > Does this mean we trust the renderer to not expose non-enumerable methods?
> That
> > doesn't sound secure/correct.
> 
> OK, I will make renderers to query for method names each time they need them.
> Then, if inspection has never been enabled, the renderer will never have a
> complete list of methods (more exactly, it will not be able to find out,
whether
> it actually knows all the method names).

Wouldn't it be better to let the renderer get the list of allowable methods
once, and then have the browser deny requests to invoke methods that are not
supposed to be exposed to the renderer? (In a sense, this should be the same
enforcement mechanism as in the above comment.)

[mnaganov (inactive), 2014-04-29 09:04:09]

On 2014/04/28 21:51:31, Chromium Palmer wrote:
> > I have a complete implementation locally and I'm uploading it in bite-sized
> > chunks tagged with "BUG=355644". This change is kind of a corner stone one,
as
> > landing it will enable me to land the code of the implementation on both
> browser
> > and renderer sides.
> 
> OK, makes sense.
> 
> > > This should be merely advisory, for the benefit of the renderer, right?
> > > 
> > > The actual security/correctness comes from the fact that the browser will
> not
> > > honor future requests to use this object?
> > 
> > Yes, that's correct.
> 
> Cool, thanks. I assume from this answer that the Java methods execute in the
> browser process, not the renderer process?
>

Yes, that's also correct. I kindly invite you to take a look at
go/clank/engineering/legacy-webview/java-bridge where all these details are
described.
 
> > > Does this mean we trust the renderer to not expose non-enumerable methods?
> > That
> > > doesn't sound secure/correct.
> > 
> > OK, I will make renderers to query for method names each time they need
them.
> > Then, if inspection has never been enabled, the renderer will never have a
> > complete list of methods (more exactly, it will not be able to find out,
> whether
> > it actually knows all the method names).
> 
> Wouldn't it be better to let the renderer get the list of allowable methods
> once, and then have the browser deny requests to invoke methods that are not
> supposed to be exposed to the renderer? (In a sense, this should be the same
> enforcement mechanism as in the above comment.)

I think we have a misunderstanding here. Let me provide more details, why these
queries are needed.

Historically, WebView didn't expose method names of injected Java objects to
JavaScript. So, invoking "Object.keys()" or doing a "for .. in" loop on an
injected object produced nothing. When we have released the new WebView with
DevTools support, this started to pose some inconvenience, as when you type the
name of the injected object in DevTools console, you expect to see the list of
its methods and properties (in fact, in modern DevTools versions you can see
methods of the injected object's prorotype -- JS Object, but still not methods
exposed from Java). So we have fixed this issue, see http://crbug.com/333258

As you can see from the mentioned bug's comments, we made this behavior
configurable to mitigate compatibility risks. This is why there is a switch for
toggling object's inspectability. We don't expect this switch to be toggled more
than once in the application lifetime though. At least, it's OK for any object
to only capture the value of this switch on creation.

Thus, denying method _invocation_ is done on the renderer side and is totally
orthogonal to methods enumeration. You can expose a method on JS side, but then
reject invoking it. You also can avoid exposing a method, but then accept
requests for its invocation (if JS code knows that the method actually exists).
See http://crbug.com/359528

W.r.t method enumeration, I see 3 possible approaches:

 1. Browser tells the renderer, whether methods are enumerable, and relies on
the renderer to comply to this. Renderer asks for method names once and
remembers them (basically, this is what I've done in PS1)

 2. Renderer asks the browser any time when it needs to enumerate methods and
doesn't memorize method names at all (this is done in PS2)

 3. Renderer asks the browser any time when it needs to enumerate methods, but
once it had received them (that means, the browser has enabled methods
enumerability), it memorizes them, and asks the browser no more. This is similar
to approach #2, but caches the result. Should also be OK, as I have told before,
the enumerability switch can only flip once in the app's lifetime.

WDYT?

[mnaganov (inactive), 2014-05-01 10:33:04]

Chris, ping!

On 2014/04/29 09:04:09, Mikhail Naganov (Cr) wrote:
> On 2014/04/28 21:51:31, Chromium Palmer wrote:
> > > I have a complete implementation locally and I'm uploading it in
bite-sized
> > > chunks tagged with "BUG=355644". This change is kind of a corner stone
one,
> as
> > > landing it will enable me to land the code of the implementation on both
> > browser
> > > and renderer sides.
> > 
> > OK, makes sense.
> > 
> > > > This should be merely advisory, for the benefit of the renderer, right?
> > > > 
> > > > The actual security/correctness comes from the fact that the browser
will
> > not
> > > > honor future requests to use this object?
> > > 
> > > Yes, that's correct.
> > 
> > Cool, thanks. I assume from this answer that the Java methods execute in the
> > browser process, not the renderer process?
> >
> 
> Yes, that's also correct. I kindly invite you to take a look at
> go/clank/engineering/legacy-webview/java-bridge where all these details are
> described.
>  
> > > > Does this mean we trust the renderer to not expose non-enumerable
methods?
> > > That
> > > > doesn't sound secure/correct.
> > > 
> > > OK, I will make renderers to query for method names each time they need
> them.
> > > Then, if inspection has never been enabled, the renderer will never have a
> > > complete list of methods (more exactly, it will not be able to find out,
> > whether
> > > it actually knows all the method names).
> > 
> > Wouldn't it be better to let the renderer get the list of allowable methods
> > once, and then have the browser deny requests to invoke methods that are not
> > supposed to be exposed to the renderer? (In a sense, this should be the same
> > enforcement mechanism as in the above comment.)
> 
> I think we have a misunderstanding here. Let me provide more details, why
these
> queries are needed.
> 
> Historically, WebView didn't expose method names of injected Java objects to
> JavaScript. So, invoking "Object.keys()" or doing a "for .. in" loop on an
> injected object produced nothing. When we have released the new WebView with
> DevTools support, this started to pose some inconvenience, as when you type
the
> name of the injected object in DevTools console, you expect to see the list of
> its methods and properties (in fact, in modern DevTools versions you can see
> methods of the injected object's prorotype -- JS Object, but still not methods
> exposed from Java). So we have fixed this issue, see http://crbug.com/333258
> 
> As you can see from the mentioned bug's comments, we made this behavior
> configurable to mitigate compatibility risks. This is why there is a switch
for
> toggling object's inspectability. We don't expect this switch to be toggled
more
> than once in the application lifetime though. At least, it's OK for any object
> to only capture the value of this switch on creation.
> 
> Thus, denying method _invocation_ is done on the renderer side and is totally
> orthogonal to methods enumeration. You can expose a method on JS side, but
then
> reject invoking it. You also can avoid exposing a method, but then accept
> requests for its invocation (if JS code knows that the method actually
exists).
> See http://crbug.com/359528
> 
> W.r.t method enumeration, I see 3 possible approaches:
> 
>  1. Browser tells the renderer, whether methods are enumerable, and relies on
> the renderer to comply to this. Renderer asks for method names once and
> remembers them (basically, this is what I've done in PS1)
> 
>  2. Renderer asks the browser any time when it needs to enumerate methods and
> doesn't memorize method names at all (this is done in PS2)
> 
>  3. Renderer asks the browser any time when it needs to enumerate methods, but
> once it had received them (that means, the browser has enabled methods
> enumerability), it memorizes them, and asks the browser no more. This is
similar
> to approach #2, but caches the result. Should also be OK, as I have told
before,
> the enumerability switch can only flip once in the app's lifetime.
> 
> WDYT?

[palmer, 2014-05-05 18:09:36]

> > Cool, thanks. I assume from this answer that the Java methods execute in the
> > browser process, not the renderer process?
> 
> Yes, that's also correct. I kindly invite you to take a look at
> go/clank/engineering/legacy-webview/java-bridge where all these details are
> described.

OK. (go/clank/engineering/legacy-webview/java-bridge doesn't seem to say for
sure in what process the Java object's methods are executed.) But this seems to
cause a problem when combined with:

> Thus, denying method _invocation_ is done on the renderer side and is totally
> orthogonal to methods enumeration. You can expose a method on JS side, but
then
> reject invoking it. You also can avoid exposing a method, but then accept
> requests for its invocation (if JS code knows that the method actually
exists).
> See http://crbug.com/359528

So the Java code runs in the browser process, but the browser trusts the
renderer to deny invocation of any dangerous methods/methods the Java developer
did not intend to expose to the web?

That is a recipe for trouble.

> W.r.t method enumeration, I see 3 possible approaches:
> 
>  1. Browser tells the renderer, whether methods are enumerable, and relies on
> the renderer to comply to this. Renderer asks for method names once and
> remembers them (basically, this is what I've done in PS1)
> 
>  2. Renderer asks the browser any time when it needs to enumerate methods and
> doesn't memorize method names at all (this is done in PS2)
> 
>  3. Renderer asks the browser any time when it needs to enumerate methods, but
> once it had received them (that means, the browser has enabled methods
> enumerability), it memorizes them, and asks the browser no more. This is
similar
> to approach #2, but caches the result. Should also be OK, as I have told
before,
> the enumerability switch can only flip once in the app's lifetime.

Any of these options is fine — whatever you prefer — as long as the
privilege-holder (in this case, the browser process) exercises final control
over whether or not to execute code on behalf of a non-privilege-holder (in this
case, JavaScript in the renderer process).

[mnaganov (inactive), 2014-05-05 18:22:53]

On 2014/05/05 18:09:36, Chromium Palmer wrote:
> > > Cool, thanks. I assume from this answer that the Java methods execute in
the
> > > browser process, not the renderer process?
> > 
> > Yes, that's also correct. I kindly invite you to take a look at
> > go/clank/engineering/legacy-webview/java-bridge where all these details are
> > described.
> 
> OK. (go/clank/engineering/legacy-webview/java-bridge doesn't seem to say for
> sure in what process the Java object's methods are executed.) But this seems
to
> cause a problem when combined with:
>

Sorry for that. I will make the description more clear. Thanks for pointing this
out!
 
> > Thus, denying method _invocation_ is done on the renderer side and is
totally
> > orthogonal to methods enumeration. You can expose a method on JS side, but
> then
> > reject invoking it. You also can avoid exposing a method, but then accept
> > requests for its invocation (if JS code knows that the method actually
> exists).
> > See http://crbug.com/359528
> 
> So the Java code runs in the browser process, but the browser trusts the
> renderer to deny invocation of any dangerous methods/methods the Java
developer
> did not intend to expose to the web?
>

No, it doesn't. Not sure what makes you think so.

This is what happens. On the browser side, a list of methods allowed to be
called is maintained. If a method name passed from a renderer is not on this
list, it will not be called. So a renderer can only _suggest_ which method to
call, the decision whether to invoke it or not is always up to the browser.
 
> That is a recipe for trouble.
> 
> > W.r.t method enumeration, I see 3 possible approaches:
> > 
> >  1. Browser tells the renderer, whether methods are enumerable, and relies
on
> > the renderer to comply to this. Renderer asks for method names once and
> > remembers them (basically, this is what I've done in PS1)
> > 
> >  2. Renderer asks the browser any time when it needs to enumerate methods
and
> > doesn't memorize method names at all (this is done in PS2)
> > 
> >  3. Renderer asks the browser any time when it needs to enumerate methods,
but
> > once it had received them (that means, the browser has enabled methods
> > enumerability), it memorizes them, and asks the browser no more. This is
> similar
> > to approach #2, but caches the result. Should also be OK, as I have told
> before,
> > the enumerability switch can only flip once in the app's lifetime.
> 
> Any of these options is fine — whatever you prefer — as long as the
> privilege-holder (in this case, the browser process) exercises final control
> over whether or not to execute code on behalf of a non-privilege-holder (in
this
> case, JavaScript in the renderer process).

As I've said, this is already in place.

[palmer, 2014-05-05 18:33:59]

> This is what happens. On the browser side, a list of methods allowed to be
> called is maintained. If a method name passed from a renderer is not on this
> list, it will not be called. So a renderer can only _suggest_ which method to
> call, the decision whether to invoke it or not is always up to the browser.

Ohhhhhkay. This is what I wanted to hear. Thanks for bearing with me. :)

[palmer, 2014-05-05 18:35:10]

LGTM

[mnaganov (inactive), 2014-05-06 08:47:39]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-05-06 08:47:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/249473002/40001

[commit-bot: I haz the power, 2014-05-06 10:26:48]

Change committed as 268480