[Extensions] Update web API cloberring for platform apps

[Extensions] Update web API cloberring for platform apps

Platform App clobbering for certain Web APIs was incomplete.  Make it more
complete.  See bug for more details.

BUG=585282
Committed: https://crrev.com/dc42ae208c2744f7fb144b2e396358a1fc34db87
Cr-Commit-Position: refs/heads/master@{#376784}

[Devlin, 2016-02-19 01:11:14]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-02-19 01:12:28]

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

[Devlin, 2016-02-19 01:12:29]

Heya Rob, mind taking a look?

[robwu, 2016-02-19 10:38:01]

It might also be a good idea to check whether the property/method exists, using
$Object.getOwnPropertyDescriptor(obj, key) before restricting the APIs (and
throw an error if the property does not exist). That will catch regressions like
the ones with the document methods.

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

https://codereview.chromium.org/1716513002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/platform_app.js:212: if (type == 'unload' || type
== 'beforeunload')
This does not work, there is a TOCTOU issue here, e.g. if type = {
  toString() {
    this.toString = function() { return 'load'; };
    return 'toctou in action';
 }
};
// You can try the above in the devtools using:
if (type != 'load')
  alert(type); // 'load'

I suggest type += '';, which will certainly reduce the input to a primitive
string, and then perform a check using ===.

[Devlin, 2016-02-19 20:44:00]

On 2016/02/19 10:38:01, robwu wrote:
> It might also be a good idea to check whether the property/method exists,
using
> $Object.getOwnPropertyDescriptor(obj, key) before restricting the APIs (and
> throw an error if the property does not exist). That will catch regressions
like
> the ones with the document methods.

I like it. It also found some other fun issues, like the fact that
Window.prototype doesn't have a whole lot on it.

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

https://codereview.chromium.org/1716513002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/platform_app.js:212: if (type == 'unload' || type
== 'beforeunload')
On 2016/02/19 10:38:01, robwu wrote:
> This does not work, there is a TOCTOU issue here, e.g. if type = {
>   toString() {
>     this.toString = function() { return 'load'; };
>     return 'toctou in action';
>  }
> };
> // You can try the above in the devtools using:
> if (type != 'load')
>   alert(type); // 'load'
> 
> I suggest type += '';, which will certainly reduce the input to a primitive
> string, and then perform a check using ===.

sgtm, done.

[robwu, 2016-02-19 21:22:53]

https://codereview.chromium.org/1716513002/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/platform_app.js (right):

https://codereview.chromium.org/1716513002/diff/40001/extensions/renderer/res...
extensions/renderer/resources/platform_app.js:225: if ((type + '') === 'unload'
|| (type + '') === 'beforeunload')
This is still vulnerable to a TOCTOU, because the operand in the comparison
differs from what's actually passed to eventTargetAddEventListener.

I suggest something that is functionally equivalent to this (with a suitable
comment):

EventTarget.prototype.addEventListener = function() {
  var args = $Array.slice(arguments);
  var type = (args[0] += '');
  if (type === 'unload' || type === 'beforeunload')
    generateDisabledMethodStub(type)();
  else
    return $Function.apply(eventTargetAddEventListener, this, args);
};

[Devlin, 2016-02-19 22:05:09]

https://codereview.chromium.org/1716513002/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/platform_app.js (right):

https://codereview.chromium.org/1716513002/diff/40001/extensions/renderer/res...
extensions/renderer/resources/platform_app.js:225: if ((type + '') === 'unload'
|| (type + '') === 'beforeunload')
On 2016/02/19 21:22:53, robwu wrote:
> This is still vulnerable to a TOCTOU, because the operand in the comparison
> differs from what's actually passed to eventTargetAddEventListener.
> 
> I suggest something that is functionally equivalent to this (with a suitable
> comment):
> 
> EventTarget.prototype.addEventListener = function() {
>   var args = $Array.slice(arguments);
>   var type = (args[0] += '');
>   if (type === 'unload' || type === 'beforeunload')
>     generateDisabledMethodStub(type)();
>   else
>     return $Function.apply(eventTargetAddEventListener, this, args);
> };

Hmm... does that really make a difference?  What's an example of when that would
work, but this wouldn't catch it?

Curiosity aside, I don't vastly prefer mine over that, so done in the interest
of fewer patch sets.

[robwu, 2016-02-19 22:32:15]

https://codereview.chromium.org/1716513002/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/platform_app.js (right):

https://codereview.chromium.org/1716513002/diff/40001/extensions/renderer/res...
extensions/renderer/resources/platform_app.js:225: if ((type + '') === 'unload'
|| (type + '') === 'beforeunload')
On 2016/02/19 22:05:09, Devlin wrote:
> On 2016/02/19 21:22:53, robwu wrote:
> > This is still vulnerable to a TOCTOU, because the operand in the comparison
> > differs from what's actually passed to eventTargetAddEventListener.
> > 
> > I suggest something that is functionally equivalent to this (with a suitable
> > comment):
> > 
> > EventTarget.prototype.addEventListener = function() {
> >   var args = $Array.slice(arguments);
> >   var type = (args[0] += '');
> >   if (type === 'unload' || type === 'beforeunload')
> >     generateDisabledMethodStub(type)();
> >   else
> >     return $Function.apply(eventTargetAddEventListener, this, args);
> > };
> 
> Hmm... does that really make a difference?  What's an example of when that
would
> work, but this wouldn't catch it?

The issue with separately doing
1. ('' + type) === 'onload'
2. addEventListener.apply(this, arguments); // arguments[0] === type

is that the value at (1) is not necessarily the same as the value at (2). When
the argument is a non-primitive object, you cannot be sure that its internal
value stays the same.

My first review comment showed an example that you can run directly from
Chrome's devtools.

For a full annotated example of a bypass, see my next comment.

https://codereview.chromium.org/1716513002/diff/60001/extensions/renderer/res...
File extensions/renderer/resources/platform_app.js (right):

https://codereview.chromium.org/1716513002/diff/60001/extensions/renderer/res...
extensions/renderer/resources/platform_app.js:225: // to pass in something that
evals to 'load' but wouldn't === 'load'.
Nit: 'load' -> 'onload'

https://codereview.chromium.org/1716513002/diff/60001/extensions/renderer/res...
extensions/renderer/resources/platform_app.js:226: var type = args[0] + '';
Sorry, this is still not sufficient.

You have to *change* args[0] to a primitive string (the += in my suggestion was
very significant). The check here can still be bypassed with the definition of
type from my 1st review comment.

// Example:
var myType = {
  toString() {
    myType.toString = function() { return 'beforeunload'; };
    return 'xxx';
  }
};
var myFunc = function() { return "shouldn't happen"; }
addEventListener(myType, myFunc);

// This happens:
var args = $Array.slice(arguments);
// args = [myType, myFunc]

var type = args[0] + '';
// is similar to type = myType.toString() + ''; // = 'xxx'
// and args[0] = {toString: function() { return 'beforeunload'; }}.

if (type === 'unload' || type === 'beforeunload')
// Because type === 'xxx', both conditions are obviously not true

else
  return $Function.apply(eventTargetAddEventListener, this, args);
// addEventListener now converts args[0] to a string.
// args[0] is an object, args[0].toString() === 'beforeunload'
// ---> Bypassed!

[Devlin, 2016-02-19 23:16:22]

Patchset #4 (id:80001) has been deleted

[Devlin, 2016-02-19 23:23:11]

https://codereview.chromium.org/1716513002/diff/60001/extensions/renderer/res...
File extensions/renderer/resources/platform_app.js (right):

https://codereview.chromium.org/1716513002/diff/60001/extensions/renderer/res...
extensions/renderer/resources/platform_app.js:225: // to pass in something that
evals to 'load' but wouldn't === 'load'.
On 2016/02/19 22:32:15, robwu wrote:
> Nit: 'load' -> 'onload'

s/onload/unload Done.

https://codereview.chromium.org/1716513002/diff/60001/extensions/renderer/res...
extensions/renderer/resources/platform_app.js:226: var type = args[0] + '';
On 2016/02/19 22:32:15, robwu wrote:
> Sorry, this is still not sufficient.
> 
> You have to *change* args[0] to a primitive string (the += in my suggestion
was
> very significant). The check here can still be bypassed with the definition of
> type from my 1st review comment.
> 
> // Example:
> var myType = {
>   toString() {
>     myType.toString = function() { return 'beforeunload'; };
>     return 'xxx';
>   }
> };
> var myFunc = function() { return "shouldn't happen"; }
> addEventListener(myType, myFunc);
> 
> // This happens:
> var args = $Array.slice(arguments);
> // args = [myType, myFunc]
> 
> var type = args[0] + '';
> // is similar to type = myType.toString() + ''; // = 'xxx'
> // and args[0] = {toString: function() { return 'beforeunload'; }}.
> 
> if (type === 'unload' || type === 'beforeunload')
> // Because type === 'xxx', both conditions are obviously not true
> 
> else
>   return $Function.apply(eventTargetAddEventListener, this, args);
> // addEventListener now converts args[0] to a string.
> // args[0] is an object, args[0].toString() === 'beforeunload'
> // ---> Bypassed!

Ah, I missed a few of the subtleties when I looked at this earlier.  Very
clever.  Now addressed and tested.

[robwu, 2016-02-19 23:35:16]

lgtm

[Devlin, 2016-02-22 18:43:30]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-02-22 18:43:31]

The patchset sent to the CQ was uploaded after l-g-t-m from rob@robwu.nl
Link to the patchset: https://codereview.chromium.org/1716513002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-02-22 18:44:32]

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

[commit-bot: I haz the power, 2016-02-22 20:09:34]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-02-22 20:11:01]

Description was changed from

==========
[Extensions] Update web API cloberring for platform apps

Platform App clobbering for certain Web APIs was incomplete.  Make it more
complete.  See bug for more details.

BUG=585282
==========

to

==========
[Extensions] Update web API cloberring for platform apps

Platform App clobbering for certain Web APIs was incomplete.  Make it more
complete.  See bug for more details.

BUG=585282

Committed: https://crrev.com/dc42ae208c2744f7fb144b2e396358a1fc34db87
Cr-Commit-Position: refs/heads/master@{#376784}
==========

[commit-bot: I haz the power, 2016-02-22 20:11:02]

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