[Extensions] Update last_error.js, send_request.js

[Extensions] Update last_error.js, send_request.js

BUG=591164
Committed: https://crrev.com/4368f22e9c477c1ff13a244bb59c0635d94355b0
Cr-Commit-Position: refs/heads/master@{#389840}

[Devlin, 2016-04-22 22:56:53]

Description was changed from

==========
Test
BUG=
==========

to

==========
[Extensions] Update last_error.js, send_request.js

BUG=591164
==========

[Devlin, 2016-04-23 00:26:30]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-04-23 00:26:44]

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

[robwu, 2016-04-23 09:54:38]

I can't see crbug.com/591164, can you cc me?

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

https://codereview.chromium.org/1914643003/diff/20001/extensions/renderer/res...
extensions/renderer/resources/last_error.js:71: return
$Object.hasOwnProperty(targetChrome.runtime, 'lastError');
Note that |targetChrome| is fully under control of a third party, so this does
not offer any protection against tampering (anything can be intercepted through
a Proxy, including $Object.defineProperty).

If you find that worrisome, then you could save the generated runtime bindings
in runtime_custom_bindings.js to privates of the global object, and retrieve it
here.

But I wouldn't bother, since maybe people are relying on being able to intercept
chrome.runtime.lastError for logging or something, and there is no sensitive
information being exposed here.

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

https://codereview.chromium.org/1914643003/diff/20001/extensions/renderer/res...
extensions/renderer/resources/send_request.js:118: optArgs = { __proto__: null
};
Can you also check the callers of sendRequest and make sure that optArgs
inherits from null?

E.g. the properties of optArgs in binding.js will leak when you "optArgs.stack"
is accessed below.

[Devlin, 2016-04-25 21:50:36]

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

https://codereview.chromium.org/1914643003/diff/20001/extensions/renderer/res...
extensions/renderer/resources/last_error.js:71: return
$Object.hasOwnProperty(targetChrome.runtime, 'lastError');
On 2016/04/23 09:54:38, robwu wrote:
> Note that |targetChrome| is fully under control of a third party, so this does
> not offer any protection against tampering (anything can be intercepted
through
> a Proxy, including $Object.defineProperty).
> 
> If you find that worrisome, then you could save the generated runtime bindings
> in runtime_custom_bindings.js to privates of the global object, and retrieve
it
> here.
> 
> But I wouldn't bother, since maybe people are relying on being able to
intercept
> chrome.runtime.lastError for logging or something, and there is no sensitive
> information being exposed here.

Yeah, this change was just because IIRC, 'in' actually relies on built-in object
methods, which means that it fails if someone clobbers the Object prototype (so
just trying to be a good citizen and use safe builtins here).

It'd actually be nice to get rid of targetChrome for other reasons, but I don't
think we can - we let the caller pass in an arbitrary chrome because we allow
cross-context (though not cross origin) chrome calls, and the error should be
set on the calling chrome.  See handleResponse() in send_request.js, and
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...
for an example of a cross-context call.

I'd love to be able to disallow that, but, well, it's probably too late (and at
least orthogonal to the rest of these issues).

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

https://codereview.chromium.org/1914643003/diff/20001/extensions/renderer/res...
extensions/renderer/resources/send_request.js:118: optArgs = { __proto__: null
};
On 2016/04/23 09:54:38, robwu wrote:
> Can you also check the callers of sendRequest and make sure that optArgs
> inherits from null?
> 
> E.g. the properties of optArgs in binding.js will leak when you
"optArgs.stack"
> is accessed below.

That wasn't as painful as I thought it'd be, given how many callers of
sendRequest() there are.  Done.

[robwu, 2016-04-25 22:25:53]

lgtm with nit.

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

https://codereview.chromium.org/1914643003/diff/20001/extensions/renderer/res...
extensions/renderer/resources/last_error.js:71: return
$Object.hasOwnProperty(targetChrome.runtime, 'lastError');
On 2016/04/25 21:50:35, Devlin wrote:
> On 2016/04/23 09:54:38, robwu wrote:
> > Note that |targetChrome| is fully under control of a third party, so this
does
> > not offer any protection against tampering (anything can be intercepted
> through
> > a Proxy, including $Object.defineProperty).
> > 
> > If you find that worrisome, then you could save the generated runtime
bindings
> > in runtime_custom_bindings.js to privates of the global object, and retrieve
> it
> > here.
> > 
> > But I wouldn't bother, since maybe people are relying on being able to
> intercept
> > chrome.runtime.lastError for logging or something, and there is no sensitive
> > information being exposed here.
> 
> Yeah, this change was just because IIRC, 'in' actually relies on built-in
object
> methods, which means that it fails if someone clobbers the Object prototype
(so
> just trying to be a good citizen and use safe builtins here).

hasOwnProperty is better than "in" because it only looks on the object itself,
but if the object is under control of a third party, then hasOwnProperty can
also be trapped, FYI:

chrome = {};
chrome.runtime = new Proxy({
  __proto__: null, // Obviously no "lastError" here.
}, {
  getOwnPropertyDescriptor() {
    return {configurable:true};
  },
});
$Object.hasOwnProperty(chrome.runtime, 'lastError'); // true

> It'd actually be nice to get rid of targetChrome for other reasons, but I
don't
> think we can - we let the caller pass in an arbitrary chrome because we allow
> cross-context (though not cross origin) chrome calls, and the error should be
> set on the calling chrome.  See handleResponse() in send_request.js, and
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...
> for an example of a cross-context call.
> 
> I'd love to be able to disallow that, but, well, it's probably too late (and
at
> least orthogonal to the rest of these issues).

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

https://codereview.chromium.org/1914643003/diff/40001/extensions/renderer/res...
extensions/renderer/resources/send_request.js:119:
logging.DCHECK(optArgs.__proto__ == null);
$Object.getPrototypeOf(optArgs) === null

If not set, __proto__ can be spoofed.

[Devlin, 2016-04-25 22:35:00]

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

https://codereview.chromium.org/1914643003/diff/20001/extensions/renderer/res...
extensions/renderer/resources/last_error.js:71: return
$Object.hasOwnProperty(targetChrome.runtime, 'lastError');
On 2016/04/25 22:25:53, robwu wrote:
> On 2016/04/25 21:50:35, Devlin wrote:
> > On 2016/04/23 09:54:38, robwu wrote:
> > > Note that |targetChrome| is fully under control of a third party, so this
> does
> > > not offer any protection against tampering (anything can be intercepted
> > through
> > > a Proxy, including $Object.defineProperty).
> > > 
> > > If you find that worrisome, then you could save the generated runtime
> bindings
> > > in runtime_custom_bindings.js to privates of the global object, and
retrieve
> > it
> > > here.
> > > 
> > > But I wouldn't bother, since maybe people are relying on being able to
> > intercept
> > > chrome.runtime.lastError for logging or something, and there is no
sensitive
> > > information being exposed here.
> > 
> > Yeah, this change was just because IIRC, 'in' actually relies on built-in
> object
> > methods, which means that it fails if someone clobbers the Object prototype
> (so
> > just trying to be a good citizen and use safe builtins here).
> 
> hasOwnProperty is better than "in" because it only looks on the object itself,
> but if the object is under control of a third party, then hasOwnProperty can
> also be trapped, FYI:
> 
> chrome = {};
> chrome.runtime = new Proxy({
>   __proto__: null, // Obviously no "lastError" here.
> }, {
>   getOwnPropertyDescriptor() {
>     return {configurable:true};
>   },
> });
> $Object.hasOwnProperty(chrome.runtime, 'lastError'); // true
> 
> > It'd actually be nice to get rid of targetChrome for other reasons, but I
> don't
> > think we can - we let the caller pass in an arbitrary chrome because we
allow
> > cross-context (though not cross origin) chrome calls, and the error should
be
> > set on the calling chrome.  See handleResponse() in send_request.js, and
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...
> > for an example of a cross-context call.
> > 
> > I'd love to be able to disallow that, but, well, it's probably too late (and
> at
> > least orthogonal to the rest of these issues).
> 

Right, again this isn't to defend against anything other than clobbering,
because we don't have much control over targetChrome (so anything we do here is
pretty much irrelevant).

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

https://codereview.chromium.org/1914643003/diff/40001/extensions/renderer/res...
extensions/renderer/resources/send_request.js:119:
logging.DCHECK(optArgs.__proto__ == null);
On 2016/04/25 22:25:53, robwu wrote:
> $Object.getPrototypeOf(optArgs) === null
> 
> If not set, __proto__ can be spoofed.

I had thought about that, but this is only to check our own code for issues.  If
anyone can call sendRequest but us, we're already in a bad state.  This is just
to verify that our own callsites are correct, and, for that, this is sufficient.
 Can you think of a counter case where doing $Object.getPrototypeOf() would be
more helpful?

[robwu, 2016-04-25 22:53:27]

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

https://codereview.chromium.org/1914643003/diff/40001/extensions/renderer/res...
extensions/renderer/resources/send_request.js:119:
logging.DCHECK(optArgs.__proto__ == null);
On 2016/04/25 22:35:00, Devlin wrote:
> On 2016/04/25 22:25:53, robwu wrote:
> > $Object.getPrototypeOf(optArgs) === null
> > 
> > If not set, __proto__ can be spoofed.
> 
> I had thought about that, but this is only to check our own code for issues. 
If
> anyone can call sendRequest but us, we're already in a bad state.  This is
just
> to verify that our own callsites are correct, and, for that, this is
sufficient.
>  Can you think of a counter case where doing $Object.getPrototypeOf() would be
> more helpful?

It's just a debug assertion, so it doesn't matter.

[asargent_no_longer_on_chrome, 2016-04-26 17:59:33]

lgtm

[Devlin, 2016-04-26 18:19:48]

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

[commit-bot: I haz the power, 2016-04-26 18:20:22]

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

[commit-bot: I haz the power, 2016-04-26 19:12:40]

Description was changed from

==========
[Extensions] Update last_error.js, send_request.js

BUG=591164
==========

to

==========
[Extensions] Update last_error.js, send_request.js

BUG=591164
==========

[commit-bot: I haz the power, 2016-04-26 19:12:45]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-04-26 19:15:05]

Description was changed from

==========
[Extensions] Update last_error.js, send_request.js

BUG=591164
==========

to

==========
[Extensions] Update last_error.js, send_request.js

BUG=591164

Committed: https://crrev.com/4368f22e9c477c1ff13a244bb59c0635d94355b0
Cr-Commit-Position: refs/heads/master@{#389840}
==========

[commit-bot: I haz the power, 2016-04-26 19:15:06]

Patchset 2 (id:??) landed as
https://crrev.com/4368f22e9c477c1ff13a244bb59c0635d94355b0
Cr-Commit-Position: refs/heads/master@{#389840}