Re-enable Object.observe and add enforcement for security invariants.

Re-enable Object.observe and add enforcement for security invariants.

This patch reverts r21062 which disabled Object.observe and the relevant tests.

It also adds enforcement for the following three invariants:

1) No observer may receive a change record describing changes to an object which is in different security origin (context have differing security tokens)

2) No observer may receive a change record whose context's security token is different from that of the object described by the change.

3) Object.getNotifier will return null if the caller and the provided object are in differing security origins

Further, it ensures that the global object can never be observed nor a notifier retrieved for it.

Tests are included.
R=verwaest@chromium.org, rossberg
LOG=Y

Committed: https://code.google.com/p/v8/source/detail?r=21122

[dcarney, 2014-04-30 07:16:36]

Seems reasonable to me, but I wonder if we care about the access_check bit at
all?  Is that just for performance that check?  It seems like you could just
always do the same origin check on enqueue and call it a day.

We're trying to move everything except property reads and writes away from
access checks and into the same origin check in places where things can leak
cross context.

[rossberg, 2014-04-30 11:28:32]

https://codereview.chromium.org/265503002/diff/20001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/265503002/diff/20001/src/object-observe.js#ne...
src/object-observe.js:442: var needsAccessCheck =
%IsAccessCheckNeeded(changeRecord.object);
I believe you always need to do the same-origin check. As Dan suggests, this
runtime function should go.

https://codereview.chromium.org/265503002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/265503002/diff/20001/src/runtime.cc#newcode14907
src/runtime.cc:14907: return *context1 == *context2 ||
Drop this micro opt (i.e., just check the security token).

https://codereview.chromium.org/265503002/diff/20001/src/runtime.cc#newcode14912
src/runtime.cc:14912: RUNTIME_FUNCTION(Runtime_IsAccessAllowedForObserver) {
Rename this to talk about SameOrigin, not Access.

https://codereview.chromium.org/265503002/diff/20001/src/runtime.cc#newcode14916
src/runtime.cc:14916: CONVERT_ARG_HANDLE_CHECKED(JSObject, object, 1);
Side note: shouldn't this be JSReceiver, to include proxies?

Likewise, it seems that we currently do not handle observers that are function
proxies (i.e., the previous line also would have to be JSReceiver, not
JSFunction).

https://codereview.chromium.org/265503002/diff/20001/src/runtime.cc#newcode14937
src/runtime.cc:14937: return isolate->heap()->true_value();
Is this safe? It seems this would allow you to apply the getNotifier method from
one context to get the notifier for an object (even a locked-down one) from a
different context, and from there you can go to Function and boom.

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:489: TEST(ObserverSecurityAAA) {
How about merging all these tests into a single one using a triple loop over an
array of different contexts, to cover all possible combinations?

Also, please add tests observing some special objects, in particular the global
one, and a couple of built-ins.

For the global, add tests that navigate in the middle of a splice.

Also, we probably need tests that turn on or off observation during a splice (I
highly suspect that the current implementation breaks in that situation).

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:587: TEST(ObserverSecurityAB1B2) {
How is this one different from BA1A2 above?

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:602: TEST(ObserverSecurityB1B2A) {
And this seems to be the same as A1A2B.

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:617: TEST(ObserverSecurityBAB) {
Same as ABA?

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:626: TEST(ObserverSecurityB1AB2) {
Same as A1BA2?

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:783: // no way for V8 to prevent and API user
from doing it.
Typo: s/and/an/

https://codereview.chromium.org/265503002/diff/20001/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (left):

https://codereview.chromium.org/265503002/diff/20001/test/mjsunit/mjsunit.sta...
test/mjsunit/mjsunit.status:131: 'es6/promises': [SKIP],
Can you do the relocations I proposed in my previous review now?

Also, please run test/webkit/fast/js tests as well, there is at least one where
you need to adjust the test expectation (otherwise the V8 waterfall will turn
red)).

[adamk, 2014-04-30 16:24:33]

+abarth for web security

[rafaelw, 2014-04-30 19:47:06]

So backing up a level here, the core concern here has to do with leaking object
references across disparate origin contexts.

Our whole system is currently based on the idea that, in general, disparate
origin contexts *cannot* share object references. There a known set of
exceptions to this (window, location), and the goal is to prevent the fact that
these objects can be shared from causing other objects to be shared.

Thus,

-We need not be worried about scenarios whose start state is that other objects
have already leaked. IOW, at that point, you don't need O.o to run code in the
other origin -- you just use the leaked objects you are holding

For example, Andreas' concern about using a foreign origin's Object.getNotifier
function. If you've got that function, you've already lost.

-In general, the approach of only checking origins when the subject of a change
is an object which is currently shared across contexts (e.g. the window object)
is sufficient.

I spoke with Dan about this this morning and he agrees, but prefers to be extra
conservative and thus we'll make the same origin check for *all* change records
that are enqueued, with the idea that we can change to only making this check
for already shared objects at a later date.

[rafaelw, 2014-05-02 03:22:32]

All comments addressed except for concerns regarding splice and file re-org. As
discussed with Toon & Andreas, non-security concerns can wait for follow-on
patches.

Note that this last patch adds the behavior that attempting to Observe a global
object throws -- as does asking for it's notifier.
 
Per discussion with Toon, until we can implement some kind of membrane, it's
simply too hard to reason about observing the global object.

https://codereview.chromium.org/265503002/diff/20001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/265503002/diff/20001/src/object-observe.js#ne...
src/object-observe.js:442: var needsAccessCheck =
%IsAccessCheckNeeded(changeRecord.object);
On 2014/04/30 11:28:32, rossberg wrote:
> I believe you always need to do the same-origin check. As Dan suggests, this
> runtime function should go.

Done.

https://codereview.chromium.org/265503002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/265503002/diff/20001/src/runtime.cc#newcode14907
src/runtime.cc:14907: return *context1 == *context2 ||
On 2014/04/30 11:28:32, rossberg wrote:
> Drop this micro opt (i.e., just check the security token).

Done.

https://codereview.chromium.org/265503002/diff/20001/src/runtime.cc#newcode14912
src/runtime.cc:14912: RUNTIME_FUNCTION(Runtime_IsAccessAllowedForObserver) {
On 2014/04/30 11:28:32, rossberg wrote:
> Rename this to talk about SameOrigin, not Access.

Done.

https://codereview.chromium.org/265503002/diff/20001/src/runtime.cc#newcode14916
src/runtime.cc:14916: CONVERT_ARG_HANDLE_CHECKED(JSObject, object, 1);
Per offline discussion with Adam, we're going to assume that proxies aren't
presently supported by O.o.

On 2014/04/30 11:28:32, rossberg wrote:
> Side note: shouldn't this be JSReceiver, to include proxies?
> 
> Likewise, it seems that we currently do not handle observers that are function
> proxies (i.e., the previous line also would have to be JSReceiver, not
> JSFunction).

https://codereview.chromium.org/265503002/diff/20001/src/runtime.cc#newcode14937
src/runtime.cc:14937: return isolate->heap()->true_value();
This isn't really a concern since if we had access to this method from another
origin, the game would already be up -- and if we used our "own" then the only
gain would be passing an object from a remote origin -- which, again, should
have this bit set.

However, per the discussion about being conservative (and also because we're
already in a runtime call, so simply doing the origin check isn't much more
work), we'll always do that check.

FWIW, Adam prepaired this part of the patch and this check wasn't a perf
consideration it was to make the proxy tests pass -- which we are now punting.

On 2014/04/30 11:28:32, rossberg wrote:
> Is this safe? It seems this would allow you to apply the getNotifier method
from
> one context to get the notifier for an object (even a locked-down one) from a
> different context, and from there you can go to Function and boom.

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:587: TEST(ObserverSecurityAB1B2) {
yes. removed.

On 2014/04/30 11:28:32, rossberg wrote:
> How is this one different from BA1A2 above?

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:602: TEST(ObserverSecurityB1B2A) {
yes. removed.

On 2014/04/30 11:28:32, rossberg wrote:
> And this seems to be the same as A1A2B.

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:617: TEST(ObserverSecurityBAB) {
yes. removed.

On 2014/04/30 11:28:32, rossberg wrote:
> Same as ABA?

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:626: TEST(ObserverSecurityB1AB2) {
yes. removed.

On 2014/04/30 11:28:32, rossberg wrote:
> Same as A1BA2?

https://codereview.chromium.org/265503002/diff/20001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:783: // no way for V8 to prevent and API user
from doing it.
Test & comment removed. 

On 2014/04/30 11:28:32, rossberg wrote:
> Typo: s/and/an/

[Toon Verwaest, 2014-05-02 13:47:59]

lgtm with nit

https://codereview.chromium.org/265503002/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/265503002/diff/100001/src/objects.cc#newcode1966
src/objects.cc:1966: ASSERT(this->IsJSFunction());
This assert is done already by JSFunction::cast so is superfluous.

[rafaelw, 2014-05-02 13:55:18]

Committed patchset #7 manually as r21122 (presubmit successful).

[rossberg, 2014-05-05 09:25:10]

On 2014/05/02 03:22:32, rafaelw wrote:
> All comments addressed except for concerns regarding splice and file re-org.
As
> discussed with Toon & Andreas, non-security concerns can wait for follow-on
> patches.

In that follow-up CL, please also reorganise the various ObserverSecurity tests
into one loop that covers all possible combinations, as I suggested in my
comments.