Ensure that privates are private.

Ensure that privates are private.

- Remove JS code injection functionality from UtilsNativeHandler.
- Ensure that utils.expose only exposes public properties.
- Prevent privates from getting poisoned via arbitrary constructor invocations.
- Prevent privates from leaking through prototypes.

BUG=603748
Committed: https://crrev.com/77e0fbe12e32b16d5c1d7c0380b45fde363004b2
Cr-Commit-Position: refs/heads/master@{#389292}

[robwu, 2016-04-20 16:34:47]

rob@robwu.nl changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-04-20 17:42:07]

https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
File extensions/renderer/utils_native_handler.cc (right):

https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
extensions/renderer/utils_native_handler.cc:60: "  function %s() {\n"
Wow.  I don't think I've ever looked closely at this code.  Executing from JS -
crazy.

I'm also a little confused on why exactly this needs to be in C++ - utils has
privates already, so what's the benefit in having this in a native handler at
all?  Could we just have utils.expose construct it's own function?

[robwu, 2016-04-21 08:48:11]

https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
File extensions/renderer/utils_native_handler.cc (right):

https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
extensions/renderer/utils_native_handler.cc:60: "  function %s() {\n"
On 2016/04/20 17:42:07, Devlin wrote:
> Wow.  I don't think I've ever looked closely at this code.  Executing from JS
-
> crazy.
> 
> I'm also a little confused on why exactly this needs to be in C++ - utils has
> privates already, so what's the benefit in having this in a native handler at
> all?  Could we just have utils.expose construct it's own function?

This exists to generate meaningful stack traces, see https://crbug.com/345429

[Devlin, 2016-04-21 16:32:32]

On 2016/04/21 08:48:11, robwu wrote:
>
https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
> File extensions/renderer/utils_native_handler.cc (right):
> 
>
https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
> extensions/renderer/utils_native_handler.cc:60: "  function %s() {\n"
> On 2016/04/20 17:42:07, Devlin wrote:
> > Wow.  I don't think I've ever looked closely at this code.  Executing from
JS
> -
> > crazy.
> > 
> > I'm also a little confused on why exactly this needs to be in C++ - utils
has
> > privates already, so what's the benefit in having this in a native handler
at
> > all?  Could we just have utils.expose construct it's own function?
> 
> This exists to generate meaningful stack traces, see https://crbug.com/345429

Couldn't we still trivially do this in JS through something like:
// utils.js
function create(cls, superclass) {
  return function() { /* util ctor-y stuff */ };
}

function expose(ctor, exposed) {
  // utils exposing stuff
}

// messaging.js
var Port = utils.create(PortImpl);
utils.expose(Port, {functions: [...], properties: [...] });

In that example, we have two separate calls, but we might be able to do away
with that if we thought about it hard enough, and even if not, there are 12
calls to utils.expose, so I'd take the 12 extra lines in exchange for not
executing any passed-in JS. :)

[Devlin, 2016-04-21 16:33:05]

On 2016/04/21 16:32:32, Devlin wrote:
> On 2016/04/21 08:48:11, robwu wrote:
> >
>
https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
> > File extensions/renderer/utils_native_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
> > extensions/renderer/utils_native_handler.cc:60: "  function %s() {\n"
> > On 2016/04/20 17:42:07, Devlin wrote:
> > > Wow.  I don't think I've ever looked closely at this code.  Executing from
> JS
> > -
> > > crazy.
> > > 
> > > I'm also a little confused on why exactly this needs to be in C++ - utils
> has
> > > privates already, so what's the benefit in having this in a native handler
> at
> > > all?  Could we just have utils.expose construct it's own function?
> > 
> > This exists to generate meaningful stack traces, see
https://crbug.com/345429
> 
> Couldn't we still trivially do this in JS through something like:
> // utils.js
> function create(cls, superclass) {
>   return function() { /* util ctor-y stuff */ };
> }
> 
> function expose(ctor, exposed) {
>   // utils exposing stuff
> }
> 
> // messaging.js
> var Port = utils.create(PortImpl);
> utils.expose(Port, {functions: [...], properties: [...] });
> 
> In that example, we have two separate calls, but we might be able to do away
> with that if we thought about it hard enough, and even if not, there are 12
> calls to utils.expose, so I'd take the 12 extra lines in exchange for not
> executing any passed-in JS. :)

(and removing utils native handler is a plus)

[robwu, 2016-04-21 17:29:36]

https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
File extensions/renderer/utils_native_handler.cc (right):

https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
extensions/renderer/utils_native_handler.cc:60: "  function %s() {\n"
On 2016/04/21 16:33:05, Devlin wrote:
> On 2016/04/21 16:32:32, Devlin wrote:
> > On 2016/04/21 08:48:11, robwu wrote:
> > > On 2016/04/20 17:42:07, Devlin wrote:
> > > > Wow.  I don't think I've ever looked closely at this code.  Executing
from
> > JS
> > > -
> > > > crazy.
> > > > 
> > > > I'm also a little confused on why exactly this needs to be in C++ -
utils
> > has
> > > > privates already, so what's the benefit in having this in a native
handler
> > at
> > > > all?  Could we just have utils.expose construct it's own function?
> > > 
> > > This exists to generate meaningful stack traces, see
> https://crbug.com/345429
> > 
> > Couldn't we still trivially do this in JS through something like:
> > // utils.js
> > function create(cls, superclass) {
> >   return function() { /* util ctor-y stuff */ };
> > }
> > 
> > function expose(ctor, exposed) {
> >   // utils exposing stuff
> > }
> > 
> > // messaging.js
> > var Port = utils.create(PortImpl);
> > utils.expose(Port, {functions: [...], properties: [...] });
> > 
> > In that example, we have two separate calls, but we might be able to do away
> > with that if we thought about it hard enough, and even if not, there are 12
> > calls to utils.expose, so I'd take the 12 extra lines in exchange for not
> > executing any passed-in JS. :)
> 
> (and removing utils native handler is a plus)

With that snippet, you'll be in the same situation again as
https://crbug.com/345429, except then the functions are not called
"publicClass", but anonymous.

What do you think of something like this? Three lines extra per caller in
exchange for no natives.

function Port(params) {
  privates(utils).constructPrivate(this, arguments);
}

utils.expose(Port, PortImpl, {__proto__:null, ...func & prop...});

// utils.js
function expose(publicClass, cls, exposed) {
  logging.CHECK(typeof publicClass === 'function');
  if (typeof exposed.superclass === 'function') {
    publicClass.prototype = $Object.create(exposed.superclass.prototype);
  }
  // Set up functions/properties/readonly as currently implemented in expose.
  ...

  privates(publicClass).constructPrivate = function(self, args) {
    var privateObj = $Object.create(cls.prototype);
    $Function.apply(cls, privateObj, args);
    privateObj.wrapper = self;
    privates(self).impl = privateObj;
  };
}

[robwu, 2016-04-21 17:33:37]

https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
File extensions/renderer/utils_native_handler.cc (right):

https://codereview.chromium.org/1903303002/diff/1/extensions/renderer/utils_n...
extensions/renderer/utils_native_handler.cc:60: "  function %s() {\n"
On 2016/04/21 17:29:35, robwu wrote:
> On 2016/04/21 16:33:05, Devlin wrote:
> > On 2016/04/21 16:32:32, Devlin wrote:
> > > On 2016/04/21 08:48:11, robwu wrote:
> > > > On 2016/04/20 17:42:07, Devlin wrote:
> > > > > Wow.  I don't think I've ever looked closely at this code.  Executing
> from
> > > JS
> > > > -
> > > > > crazy.
> > > > > 
> > > > > I'm also a little confused on why exactly this needs to be in C++ -
> utils
> > > has
> > > > > privates already, so what's the benefit in having this in a native
> handler
> > > at
> > > > > all?  Could we just have utils.expose construct it's own function?
> > > > 
> > > > This exists to generate meaningful stack traces, see
> > https://crbug.com/345429
> > > 
> > > Couldn't we still trivially do this in JS through something like:
> > > // utils.js
> > > function create(cls, superclass) {
> > >   return function() { /* util ctor-y stuff */ };
> > > }
> > > 
> > > function expose(ctor, exposed) {
> > >   // utils exposing stuff
> > > }
> > > 
> > > // messaging.js
> > > var Port = utils.create(PortImpl);
> > > utils.expose(Port, {functions: [...], properties: [...] });
> > > 
> > > In that example, we have two separate calls, but we might be able to do
away
> > > with that if we thought about it hard enough, and even if not, there are
12
> > > calls to utils.expose, so I'd take the 12 extra lines in exchange for not
> > > executing any passed-in JS. :)
> > 
> > (and removing utils native handler is a plus)
> 
> With that snippet, you'll be in the same situation again as
> https://crbug.com/345429, except then the functions are not called
> "publicClass", but anonymous.
> 
> What do you think of something like this? Three lines extra per caller in
> exchange for no natives.
> 
> function Port(params) {
>   privates(utils).constructPrivate(this, arguments);
> }
> 
> utils.expose(Port, PortImpl, {__proto__:null, ...func & prop...});
> 
> // utils.js
> function expose(publicClass, cls, exposed) {
>   logging.CHECK(typeof publicClass === 'function');
>   if (typeof exposed.superclass === 'function') {
>     publicClass.prototype = $Object.create(exposed.superclass.prototype);
>   }
>   // Set up functions/properties/readonly as currently implemented in expose.
>   ...
> 
>   privates(publicClass).constructPrivate = function(self, args) {
>     var privateObj = $Object.create(cls.prototype);
>     $Function.apply(cls, privateObj, args);
>     privateObj.wrapper = self;
>     privates(self).impl = privateObj;
>   };
> }

privates(utils).constructPrivate should be privates(Port).constructPrivate. Or
even abstracted away to utils and then utils.privateCostructor(port)(this,
arguments), or some other signature. WDYT?

[Devlin, 2016-04-21 18:31:39]

Any of those would probably be fine.  Another option which might be cleaner
could also be:
function Port() {
  this.init_();
}

utils.expose(Port, PortImpl, etc);  // Modifies the prototype of Port and adds
an init_() function that does the ctor stuff.

I'll leave it up to you which of these options you prefer - I'm happy with any
that lets us avoid doing anything like pasting js and running it.

[robwu, 2016-04-22 16:43:59]

Description was changed from

==========
Block code injection in CreateClassWrapper

CreateClassWrapper is used by utils.expose. The current consumers only
specify lowercase/uppercase alpha characters, so add an assertion that
the input only consists of such characters to avoid JS code injection.

BUG=603748
==========

to

==========
Ensure that privates are private.
    
- Remove JS code injection functionality from UtilsNativeHandler.
- Ensure that utils.expose only exposes public properties.
- Prevent privates from getting poisoned via arbitrary constructor invocations.
- Prevent privates from leaking through prototypes.
    
BUG=603748
==========

[robwu, 2016-04-22 16:48:51]

On 2016/04/21 18:31:39, Devlin wrote:
> Any of those would probably be fine.  Another option which might be cleaner
> could also be:
> function Port() {
>   this.init_();
> }
> 
> utils.expose(Port, PortImpl, etc);  // Modifies the prototype of Port and adds
> an init_() function that does the ctor stuff.
> 
> I'll leave it up to you which of these options you prefer - I'm happy with any
> that lets us avoid doing anything like pasting js and running it.

this.init_() doesn't work with inheritance, so I decided to stick with my
original suggestion (in a more elaborate form). PTAL!

[Devlin, 2016-04-22 17:08:21]

I'm getting "error: old chunk mismatch" when I try to see side-by-side, so I'd
like to take another look.  A few nits that stood out, though.

https://codereview.chromium.org/1903303002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_event.js
(right):

https://codereview.chromium.org/1903303002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_event.js:26: 
We should always do this, right?  Can we move it into expose()?

https://codereview.chromium.org/1903303002/diff/20001/extensions/renderer/res...
File extensions/renderer/resources/utils.js (right):

https://codereview.chromium.org/1903303002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/utils.js:67: 
nit: since this is just an argument, it should be publicClass (and privateClass)

https://codereview.chromium.org/1903303002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/utils.js:75: 
Let's make this blatantly clear:

* |publicClass| should be a constructor that calls constructPrivate() like so:
* function PublicClass() {
*   privates(PublicClass).constructPrivate(this, arguments);
* }

https://codereview.chromium.org/1903303002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/utils.js:171: 
$Object.self?

[robwu, 2016-04-22 17:37:40]

On 2016/04/22 17:08:21, Devlin wrote:
> I'm getting "error: old chunk mismatch" when I try to see side-by-side, so I'd
> like to take another look.  A few nits that stood out, though.

My bad, I ^C'd during git cl upload. The very first patch was very small (and
now erased), so you didn't miss out on anything though.

https://codereview.chromium.org/1903303002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_event.js
(right):

https://codereview.chromium.org/1903303002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_event.js:26: 
On 2016/04/22 17:08:21, Devlin wrote:
> We should always do this, right?  Can we move it into expose()?

Done. using $Object.setPrototypeOf, NOT by setting __proto__ because this can be
intercepted.

https://codereview.chromium.org/1903303002/diff/20001/extensions/renderer/res...
File extensions/renderer/resources/utils.js (right):

https://codereview.chromium.org/1903303002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/utils.js:67: 
On 2016/04/22 17:08:21, Devlin wrote:
> nit: since this is just an argument, it should be publicClass (and
privateClass)

Done.

https://codereview.chromium.org/1903303002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/utils.js:75: 
On 2016/04/22 17:08:21, Devlin wrote:
> Let's make this blatantly clear:
> 
> * |publicClass| should be a constructor that calls constructPrivate() like so:
> * function PublicClass() {
> *   privates(PublicClass).constructPrivate(this, arguments);
> * }

Done.

https://codereview.chromium.org/1903303002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/utils.js:171: 
On 2016/04/22 17:08:21, Devlin wrote:
> $Object.self?

Done.

[Devlin, 2016-04-22 18:52:46]

lgtm w/ nits

https://codereview.chromium.org/1903303002/diff/20001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/automation/automation_event.js
(right):

https://codereview.chromium.org/1903303002/diff/20001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/automation/automation_event.js:26: 
On 2016/04/22 17:37:40, robwu wrote:
> On 2016/04/22 17:08:21, Devlin wrote:
> > We should always do this, right?  Can we move it into expose()?
> 
> Done. using $Object.setPrototypeOf, NOT by setting __proto__ because this can
be
> intercepted.

FYI, I hear from V8 experts that Object.defineProperty would also work for
bypassing getters/setters (not saying we should use it here -
Object.setPrototypeOf makes more sense - just a possibly-useful tidbit).

https://codereview.chromium.org/1903303002/diff/80001/extensions/renderer/res...
File extensions/renderer/resources/utils.js (right):

https://codereview.chromium.org/1903303002/diff/80001/extensions/renderer/res...
extensions/renderer/resources/utils.js:80: *   be a named function, and the name
appears in stack traces.
nit: I think this should be +4 indentation from the previous line.

https://codereview.chromium.org/1903303002/diff/80001/extensions/renderer/res...
extensions/renderer/resources/utils.js:169: publicClassPrototype.__proto__ =
exposed.superclass.prototype || $Object.self;
Won't this throw if exposed.superclass is undefined?

https://codereview.chromium.org/1903303002/diff/80001/extensions/renderer/res...
File extensions/renderer/resources/web_request_internal_custom_bindings.js
(right):

https://codereview.chromium.org/1903303002/diff/80001/extensions/renderer/res...
extensions/renderer/resources/web_request_internal_custom_bindings.js:194:
exports.$set('binding', webRequestInternal);
nit: since you're here, may as well inline the binding.generate() call

[robwu, 2016-04-22 20:33:43]

https://codereview.chromium.org/1903303002/diff/80001/extensions/renderer/res...
File extensions/renderer/resources/utils.js (right):

https://codereview.chromium.org/1903303002/diff/80001/extensions/renderer/res...
extensions/renderer/resources/utils.js:80: *   be a named function, and the name
appears in stack traces.
On 2016/04/22 18:52:46, Devlin wrote:
> nit: I think this should be +4 indentation from the previous line.

Done.

https://codereview.chromium.org/1903303002/diff/80001/extensions/renderer/res...
extensions/renderer/resources/utils.js:169: publicClassPrototype.__proto__ =
exposed.superclass.prototype || $Object.self;
On 2016/04/22 18:52:46, Devlin wrote:
> Won't this throw if exposed.superclass is undefined?

Fixed. And I should also have used $Object.self.prototype instead of
$Object.self.

https://codereview.chromium.org/1903303002/diff/80001/extensions/renderer/res...
File extensions/renderer/resources/web_request_internal_custom_bindings.js
(right):

https://codereview.chromium.org/1903303002/diff/80001/extensions/renderer/res...
extensions/renderer/resources/web_request_internal_custom_bindings.js:194:
exports.$set('binding', webRequestInternal);
On 2016/04/22 18:52:46, Devlin wrote:
> nit: since you're here, may as well inline the binding.generate() call

webRequestInternal is also used internally within this file, so this should stay
as a separate variable.

[robwu, 2016-04-22 20:34:07]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-04-22 20:34:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1903303002/#ps100001
(title: "Nits + disable DCHECK")

[commit-bot: I haz the power, 2016-04-22 20:35:10]

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

[commit-bot: I haz the power, 2016-04-22 21:10:38]

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

[commit-bot: I haz the power, 2016-04-22 21:10:39]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[robwu, 2016-04-22 22:58:05]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-04-22 22:58:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1903303002/#ps120001
(title: "Fix for failing tests")

[commit-bot: I haz the power, 2016-04-22 22:58:30]

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

[commit-bot: I haz the power, 2016-04-22 23:17:04]

Description was changed from

==========
Ensure that privates are private.
    
- Remove JS code injection functionality from UtilsNativeHandler.
- Ensure that utils.expose only exposes public properties.
- Prevent privates from getting poisoned via arbitrary constructor invocations.
- Prevent privates from leaking through prototypes.
    
BUG=603748
==========

to

==========
Ensure that privates are private.
    
- Remove JS code injection functionality from UtilsNativeHandler.
- Ensure that utils.expose only exposes public properties.
- Prevent privates from getting poisoned via arbitrary constructor invocations.
- Prevent privates from leaking through prototypes.
    
BUG=603748
==========

[commit-bot: I haz the power, 2016-04-22 23:17:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-22 23:19:13]

Description was changed from

==========
Ensure that privates are private.
    
- Remove JS code injection functionality from UtilsNativeHandler.
- Ensure that utils.expose only exposes public properties.
- Prevent privates from getting poisoned via arbitrary constructor invocations.
- Prevent privates from leaking through prototypes.
    
BUG=603748
==========

to

==========
Ensure that privates are private.

- Remove JS code injection functionality from UtilsNativeHandler.
- Ensure that utils.expose only exposes public properties.
- Prevent privates from getting poisoned via arbitrary constructor invocations.
- Prevent privates from leaking through prototypes.

BUG=603748

Committed: https://crrev.com/77e0fbe12e32b16d5c1d7c0380b45fde363004b2
Cr-Commit-Position: refs/heads/master@{#389292}
==========

[commit-bot: I haz the power, 2016-04-22 23:19:14]

Patchset 7 (id:??) landed as
https://crrev.com/77e0fbe12e32b16d5c1d7c0380b45fde363004b2
Cr-Commit-Position: refs/heads/master@{#389292}