[inspector] exposed builtins for injected script source

[inspector] exposed builtins for injected script source

Methods on Object can be overriden by user, we should be prepared.

BUG=chromium:595206
R=dgozman@chromium.org,luoe@chromium.org,yangguo@chromium.org

Review-Url: https://codereview.chromium.org/2772093002
Cr-Commit-Position: refs/heads/master@{#44128}
Committed: https://chromium.googlesource.com/v8/v8/+/2beb56137fe4194a69686abdad4c9246cbc93fea

[kozy, 2017-03-24 18:14:20]

Description was changed from

==========
[inspector] expose builtins for injected script source

Methods on Object can be override by user, we should be prepared.

BUG=chromium:595206
R=dgozman@chromium.org,luoe@chromium.org
==========

to

==========
[inspector] expose builtins for injected script source

Methods on Object can be override by user, we should be prepared.

BUG=chromium:595206
R=dgozman@chromium.org,luoe@chromium.org,yangguo@chromium.org
==========

[kozy, 2017-03-24 18:14:20]

kozyatinskiy@chromium.org changed reviewers:
+ yangguo@chromium.org

[kozy, 2017-03-24 18:14:29]

Description was changed from

==========
[inspector] expose builtins for injected script source

Methods on Object can be override by user, we should be prepared.

BUG=chromium:595206
R=dgozman@chromium.org,luoe@chromium.org,yangguo@chromium.org
==========

to

==========
[inspector] exposed builtins for injected script source

Methods on Object can be override by user, we should be prepared.

BUG=chromium:595206
R=dgozman@chromium.org,luoe@chromium.org,yangguo@chromium.org
==========

[kozy, 2017-03-24 18:17:37]

Dmitry, Yang and Erik, please take a look!

In this CL I'm only exposed static methods. We probably need a way to expose
non-static methods, e.g. InjectedScriptHost.objectHasOwnProperty. For them I'm
only worried about idea that .call can be overriden - I'd like to check what
will happens if .call overriden and should I explicitly put clean function
prototype for non-static methods or shouldn't.

[kozy, 2017-03-24 18:17:49]

Description was changed from

==========
[inspector] exposed builtins for injected script source

Methods on Object can be override by user, we should be prepared.

BUG=chromium:595206
R=dgozman@chromium.org,luoe@chromium.org,yangguo@chromium.org
==========

to

==========
[inspector] exposed builtins for injected script source

Methods on Object can be overriden by user, we should be prepared.

BUG=chromium:595206
R=dgozman@chromium.org,luoe@chromium.org,yangguo@chromium.org
==========

[Yang, 2017-03-24 18:22:35]

On 2017/03/24 18:17:37, kozy wrote:
> Dmitry, Yang and Erik, please take a look!
> 
> In this CL I'm only exposed static methods. We probably need a way to expose
> non-static methods, e.g. InjectedScriptHost.objectHasOwnProperty. For them I'm
> only worried about idea that .call can be overriden - I'd like to check what
> will happens if .call overriden and should I explicitly put clean function
> prototype for non-static methods or shouldn't.

What do you need these builtins for? I would not use the implementations for JS,
but simply expose methods from JSObject.

[kozy, 2017-03-24 18:28:15]

On 2017/03/24 18:22:35, Yang wrote:
> On 2017/03/24 18:17:37, kozy wrote:
> > Dmitry, Yang and Erik, please take a look!
> > 
> > In this CL I'm only exposed static methods. We probably need a way to expose
> > non-static methods, e.g. InjectedScriptHost.objectHasOwnProperty. For them
I'm
> > only worried about idea that .call can be overriden - I'd like to check what
> > will happens if .call overriden and should I explicitly put clean function
> > prototype for non-static methods or shouldn't.
> 
> What do you need these builtins for? I would not use the implementations for
JS,
> but simply expose methods from JSObject.

We use them in injected-script-source.js for example we use Object.keys to get
all keys from object. We run injected-script-source in the same context as a
page and these methods could be redefined by user on prototype, on objects and
everywhere and I'm worried that in some cases it could be actually be collected
and we need to get new one from builtins because we compile
injected-script-source lazily on first related invocation which can happens
after methods redefinition.

[kozy, 2017-03-24 18:30:11]

On 2017/03/24 18:22:35, Yang wrote:
> On 2017/03/24 18:17:37, kozy wrote:
> > Dmitry, Yang and Erik, please take a look!
> > 
> > In this CL I'm only exposed static methods. We probably need a way to expose
> > non-static methods, e.g. InjectedScriptHost.objectHasOwnProperty. For them
I'm
> > only worried about idea that .call can be overriden - I'd like to check what
> > will happens if .call overriden and should I explicitly put clean function
> > prototype for non-static methods or shouldn't.
> 
> What do you need these builtins for? I would not use the implementations for
JS,
> but simply expose methods from JSObject.

I thought that if user redefine methods on Object and Object.prototype then we
are not able to get original methods from JSObject. Am I wrong?

[Yang, 2017-03-24 18:58:21]

On 2017/03/24 18:30:11, kozy wrote:
> On 2017/03/24 18:22:35, Yang wrote:
> > On 2017/03/24 18:17:37, kozy wrote:
> > > Dmitry, Yang and Erik, please take a look!
> > > 
> > > In this CL I'm only exposed static methods. We probably need a way to
expose
> > > non-static methods, e.g. InjectedScriptHost.objectHasOwnProperty. For them
> I'm
> > > only worried about idea that .call can be overriden - I'd like to check
what
> > > will happens if .call overriden and should I explicitly put clean function
> > > prototype for non-static methods or shouldn't.
> > 
> > What do you need these builtins for? I would not use the implementations for
> JS,
> > but simply expose methods from JSObject.
> 
> I thought that if user redefine methods on Object and Object.prototype then we
> are not able to get original methods from JSObject. Am I wrong?

I have actually been wondering about this for some time. This CL only makes
sense if injected script runs in the same native context as the user's scripts.
This means the users script can override a lot of things, especially with newer
language features, not just the Object functions you are exposing here.

Do we want to port injected script to C++ like we are doing with V8 debugger's
debug.js and mirror.js? That way we can be sure that we're not exposed to
JavaScript semantics. Another approach would be to run everything in a separate
native context.

I think we need to agree on a high-level design to mitigate issues like this.
This CL solves part of the issue, but not at its root. The injected script in
its current form will never be fool proof.

[Yang, 2017-03-24 18:59:04]

On 2017/03/24 18:58:21, Yang wrote:
> On 2017/03/24 18:30:11, kozy wrote:
> > On 2017/03/24 18:22:35, Yang wrote:
> > > On 2017/03/24 18:17:37, kozy wrote:
> > > > Dmitry, Yang and Erik, please take a look!
> > > > 
> > > > In this CL I'm only exposed static methods. We probably need a way to
> expose
> > > > non-static methods, e.g. InjectedScriptHost.objectHasOwnProperty. For
them
> > I'm
> > > > only worried about idea that .call can be overriden - I'd like to check
> what
> > > > will happens if .call overriden and should I explicitly put clean
function
> > > > prototype for non-static methods or shouldn't.
> > > 
> > > What do you need these builtins for? I would not use the implementations
for
> > JS,
> > > but simply expose methods from JSObject.
> > 
> > I thought that if user redefine methods on Object and Object.prototype then
we
> > are not able to get original methods from JSObject. Am I wrong?
> 
> I have actually been wondering about this for some time. This CL only makes
> sense if injected script runs in the same native context as the user's
scripts.
> This means the users script can override a lot of things, especially with
newer
> language features, not just the Object functions you are exposing here.
> 
> Do we want to port injected script to C++ like we are doing with V8 debugger's
> debug.js and mirror.js? That way we can be sure that we're not exposed to
> JavaScript semantics. Another approach would be to run everything in a
separate
> native context.
> 
> I think we need to agree on a high-level design to mitigate issues like this.
> This CL solves part of the issue, but not at its root. The injected script in
> its current form will never be fool proof.

Also what I meant by using methods from JSObject is that we could just do
everything in C++ if injected script was ported to C++.

[kozy, 2017-03-24 19:25:28]

On 2017/03/24 18:58:21, Yang wrote:
> I have actually been wondering about this for some time. This CL only makes
> sense if injected script runs in the same native context as the user's
scripts.
> This means the users script can override a lot of things, especially with
newer
> language features, not just the Object functions you are exposing here.
> 
> Do we want to port injected script to C++ like we are doing with V8 debugger's
> debug.js and mirror.js? That way we can be sure that we're not exposed to
> JavaScript semantics. Another approach would be to run everything in a
separate
> native context.
> 
> I think we need to agree on a high-level design to mitigate issues like this.
> This CL solves part of the issue, but not at its root. The injected script in
> its current form will never be fool proof.

First of all, I'd like to split injected-script-source into two big parts:
- part where we just run our utility JavaScript code which generate previews,
generate remote objects for protocol, e.t.c.
- part where we inspect state of the user object.

InjectedScriptSource runs in the same context. To allow running of this script
in another native context we need to find a way to say embedder (especially
blink) that from security point of view this new native context has exactly the
same security restrictions as inspected context. I'm worried that it would be
adhoc solution since node.js can implement other security checks and we need to
make them works too. Probably we can run injected-script-source in separate
context but every time when embedder gets our injected-script-source native
context we return inspected context instead. It will make first part absolutely
save, but to inspect object state we still need additional APIs to prevent
observable side effects and running arbitrary user code.
So advantages:
- absolutely safe utility code,
Disadvnatages:
- requires manipulation with security checks - always sounds dangerous,
- we still need safe way to inspect object.

Other solution - port everything to C++. It doesn't by default solve any
problems but allows us to have more control. Utility methods could be even
implemented in raw C++ without any JS calls, inspection still required to be
implemented with special tricks but with more control we can easily be safe.
Advantages:
- potentially JS-free utility part,
- a lot of control over object inspection.
Disadvantage:
- currently injected-script-source lives in inspector backend but it defines how
we show our objects in console on frontend - we probably would like to pass some
function over the protocol which makes preview for us. Otherwise backward
compatibility looks absolutely unachievable. And only one way how frontend can
provide function for formatiing - it's passing javascript code as a string. If
we move to C++ we could not achieve this.

Based on this two ideas:
- we'd like to be able to implement our utility methods in JavaScript to allow
frontend in future just pass formatter function via protocol. To achieve this we
have two ways: run utility function in separate context, provide enough
primitives with InjectedScriptHost.
- in any case we'd like to have inspection part which provides guarantees that 
we don't change state of the inspected context and don't run arbitrary user
code.

Since I'm worried about running InjectedScriptSource in different context
because:
- provide enough methods for save object inspection,
- make utility code safe and in case when we' get an issue - enforce a policy of
required change to our InspectorTest.setupInjectedScriptSource method to prevent
further regressions.

I can additionally investigate how hard would be to pretend
injected-script-source context as inspected context for embedder.
based on this thoutgts this CL just introduces additionally save inspection
methods.

[kozy, 2017-03-24 19:29:11]

On 2017/03/24 19:25:28, kozy wrote:
> On 2017/03/24 18:58:21, Yang wrote:
> > I have actually been wondering about this for some time. This CL only makes
> > sense if injected script runs in the same native context as the user's
> scripts.
> > This means the users script can override a lot of things, especially with
> newer
> > language features, not just the Object functions you are exposing here.
> > 
> > Do we want to port injected script to C++ like we are doing with V8
debugger's
> > debug.js and mirror.js? That way we can be sure that we're not exposed to
> > JavaScript semantics. Another approach would be to run everything in a
> separate
> > native context.
> > 
> > I think we need to agree on a high-level design to mitigate issues like
this.
> > This CL solves part of the issue, but not at its root. The injected script
in
> > its current form will never be fool proof.
> 
> First of all, I'd like to split injected-script-source into two big parts:
> - part where we just run our utility JavaScript code which generate previews,
> generate remote objects for protocol, e.t.c.
> - part where we inspect state of the user object.
> 
> InjectedScriptSource runs in the same context. To allow running of this script
> in another native context we need to find a way to say embedder (especially
> blink) that from security point of view this new native context has exactly
the
> same security restrictions as inspected context. I'm worried that it would be
> adhoc solution since node.js can implement other security checks and we need
to
> make them works too. Probably we can run injected-script-source in separate
> context but every time when embedder gets our injected-script-source native
> context we return inspected context instead. It will make first part
absolutely
> save, but to inspect object state we still need additional APIs to prevent
> observable side effects and running arbitrary user code.
> So advantages:
> - absolutely safe utility code,
> Disadvnatages:
> - requires manipulation with security checks - always sounds dangerous,
> - we still need safe way to inspect object.
> 
> Other solution - port everything to C++. It doesn't by default solve any
> problems but allows us to have more control. Utility methods could be even
> implemented in raw C++ without any JS calls, inspection still required to be
> implemented with special tricks but with more control we can easily be safe.
> Advantages:
> - potentially JS-free utility part,
> - a lot of control over object inspection.
> Disadvantage:
> - currently injected-script-source lives in inspector backend but it defines
how
> we show our objects in console on frontend - we probably would like to pass
some
> function over the protocol which makes preview for us. Otherwise backward
> compatibility looks absolutely unachievable. And only one way how frontend can
> provide function for formatiing - it's passing javascript code as a string. If
> we move to C++ we could not achieve this.
> 
> Based on this two ideas:
> - we'd like to be able to implement our utility methods in JavaScript to allow
> frontend in future just pass formatter function via protocol. To achieve this
we
> have two ways: run utility function in separate context, provide enough
> primitives with InjectedScriptHost.
> - in any case we'd like to have inspection part which provides guarantees that

> we don't change state of the inspected context and don't run arbitrary user
> code.
> 
> Since I'm worried about running InjectedScriptSource in different context
> because:
> - provide enough methods for save object inspection,
> - make utility code safe and in case when we' get an issue - enforce a policy
of
> required change to our InspectorTest.setupInjectedScriptSource method to
prevent
> further regressions.
> 
> I can additionally investigate how hard would be to pretend
> injected-script-source context as inspected context for embedder.
> based on this thoutgts this CL just introduces additionally save inspection
> methods.

I think a little more and idea solution looks like following to me:
- run utility code in separate context (should be fine from security point of
view if we just always return original inspected context instead of
injected-script-source context),
- provide enough inspection primitives with InjectedScriptHost.

[dgozman, 2017-03-24 19:39:13]

> Also what I meant by using methods from JSObject is that we could just do
> everything in C++ if injected script was ported to C++.

That may be a good long-term plan, but profit/resources ratio is low
and doing small fixes is better short-term.

[Yang, 2017-03-24 21:19:32]

Can we agree on not implementing new things in JS anymore and move things to C++
whenever there is a chance to?

It's really really easy to change the behavior of JS code. For example, I see
that injected-script-source uses instanceof. Behavior of instanceof can be
overridden since ES6. I also see String.prototype.substr being used, easily
overridden. There are probably a bunch of more places like these.

https://codereview.chromium.org/2772093002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2772093002/diff/1/src/api.cc#newcode9587
src/api.cc:9587: fun->shared()->set_native(true);
No need to set this native. We only need this for stack traces.

https://codereview.chromium.org/2772093002/diff/1/src/api.cc#newcode9589
src/api.cc:9589: fun->shared()->set_length(len);
Also no need to set length, I think.

[Yang, 2017-03-24 21:19:57]

On 2017/03/24 21:19:32, Yang wrote:
> Can we agree on not implementing new things in JS anymore and move things to
C++
> whenever there is a chance to?
> 
> It's really really easy to change the behavior of JS code. For example, I see
> that injected-script-source uses instanceof. Behavior of instanceof can be
> overridden since ES6. I also see String.prototype.substr being used, easily
> overridden. There are probably a bunch of more places like these.
> 
> https://codereview.chromium.org/2772093002/diff/1/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/2772093002/diff/1/src/api.cc#newcode9587
> src/api.cc:9587: fun->shared()->set_native(true);
> No need to set this native. We only need this for stack traces.
> 
> https://codereview.chromium.org/2772093002/diff/1/src/api.cc#newcode9589
> src/api.cc:9589: fun->shared()->set_length(len);
> Also no need to set length, I think.

Otherwise LGTM. For now. We need a long term plan for this though.

[kozy, 2017-03-24 21:34:43]

On 2017/03/24 21:19:32, Yang wrote:
> Can we agree on not implementing new things in JS anymore and move things to
C++
> whenever there is a chance to?
> 
> It's really really easy to change the behavior of JS code. For example, I see
> that injected-script-source uses instanceof. Behavior of instanceof can be
> overridden since ES6. I also see String.prototype.substr being used, easily
> overridden. There are probably a bunch of more places like these.

Is it possible to override instanceof behavior without defining something
special on object (proxies)?
I think that we can easily achieve safety for objects that we use internally and
we need only provide safe API to inspect object that we pass from user page to
injected script source.

We can expose String.substr via builtins.

[Yang, 2017-03-24 21:39:25]

On 2017/03/24 21:34:43, kozy wrote:
> On 2017/03/24 21:19:32, Yang wrote:
> > Can we agree on not implementing new things in JS anymore and move things to
> C++
> > whenever there is a chance to?
> > 
> > It's really really easy to change the behavior of JS code. For example, I
see
> > that injected-script-source uses instanceof. Behavior of instanceof can be
> > overridden since ES6. I also see String.prototype.substr being used, easily
> > overridden. There are probably a bunch of more places like these.
> 
> Is it possible to override instanceof behavior without defining something
> special on object (proxies)?
> I think that we can easily achieve safety for objects that we use internally
and
> we need only provide safe API to inspect object that we pass from user page to
> injected script source.
> 
> We can expose String.substr via builtins.

Sure you can fix each item as we discover them. My point is that writing
internal code in JS is not safe. We had to learn this the hard way in V8.
Finding all the ways you can hack JS is close to impossible, and instead of
auditing existing code we would be better served rewriting them. Of course the
priority for this with inspector code is fairly low, but we should avoid
introducing new JS code.

[kozy, 2017-03-24 21:39:55]

all done.

https://codereview.chromium.org/2772093002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2772093002/diff/1/src/api.cc#newcode9587
src/api.cc:9587: fun->shared()->set_native(true);
On 2017/03/24 21:19:31, Yang wrote:
> No need to set this native. We only need this for stack traces.

Done.

https://codereview.chromium.org/2772093002/diff/1/src/api.cc#newcode9589
src/api.cc:9589: fun->shared()->set_length(len);
On 2017/03/24 21:19:31, Yang wrote:
> Also no need to set length, I think.

Done.

[kozy, 2017-03-24 21:40:30]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-03-24 21:40:30]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2772093002/#ps20001
(title: "addressed comments")

[kozy, 2017-03-24 21:40:34]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2017-03-24 21:40:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kozy, 2017-03-24 21:40:39]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[kozy, 2017-03-24 21:40:39]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2017-03-24 21:40:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kozy, 2017-03-24 21:43:00]

On 2017/03/24 21:39:25, Yang wrote:
> Sure you can fix each item as we discover them. My point is that writing
> internal code in JS is not safe. We had to learn this the hard way in V8.
> Finding all the ways you can hack JS is close to impossible, and instead of
> auditing existing code we would be better served rewriting them. Of course the
> priority for this with inspector code is fairly low, but we should avoid
> introducing new JS code.

I'll put all our points to separate doc, I'm still thinking that we prefer to
have injected script source in JavaScript to be able at some point of time get
formatter related code (most part of injected-script-source is about formating)
from frontend via protocol to provide much better backward compatibility for
remote object preview.

[kozy, 2017-03-24 21:43:57]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-03-24 21:43:57]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2772093002/#ps40001
(title: "removed unused variable")

[commit-bot: I haz the power, 2017-03-24 21:44:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 22:08:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490391837104260,
"parent_rev": "efcf883eec795d592a543d509515151ef1421562", "commit_rev":
"2beb56137fe4194a69686abdad4c9246cbc93fea"}

[commit-bot: I haz the power, 2017-03-24 22:08:42]

Description was changed from

==========
[inspector] exposed builtins for injected script source

Methods on Object can be overriden by user, we should be prepared.

BUG=chromium:595206
R=dgozman@chromium.org,luoe@chromium.org,yangguo@chromium.org
==========

to

==========
[inspector] exposed builtins for injected script source

Methods on Object can be overriden by user, we should be prepared.

BUG=chromium:595206
R=dgozman@chromium.org,luoe@chromium.org,yangguo@chromium.org

Review-Url: https://codereview.chromium.org/2772093002
Cr-Commit-Position: refs/heads/master@{#44128}
Committed:
https://chromium.googlesource.com/v8/v8/+/2beb56137fe4194a69686abdad4c9246cbc...
==========

[commit-bot: I haz the power, 2017-03-24 22:08:43]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/2beb56137fe4194a69686abdad4c9246cbc...