[proxies] do not leak private symbols to proxy traps

[proxies] do not leak private symbols to proxy traps

BUG=v8:4537
LOG=N
R=neis@chromium.org, rossberg@chromium.org, jkummerow@chromium.org

Committed: https://crrev.com/3ed71daff4f378e9562ef28ae6077829e3c4bd7e
Cr-Commit-Position: refs/heads/master@{#32570}

[caitp (gmail), 2015-12-02 18:49:53]

quick fix for this issue, will add more meaningful tests shortly

[caitp (gmail), 2015-12-02 18:56:30]

Description was changed from

==========
[proxies] do not leak private symbols via to traps

BUG=v8:4537
LOG=N
R=neis@chromium.org, rossberg@chromium.org
==========

to

==========
[proxies] do not leak private symbols to proxy traps

BUG=v8:4537
LOG=N
R=neis@chromium.org, rossberg@chromium.org
==========

[caitp (gmail), 2015-12-03 11:41:56]

more tests have been added, PTAL

[caitp (gmail), 2015-12-03 11:59:24]

caitpotter88@gmail.com changed reviewers:
+ adamk@chromium.org, verwaest@chromium.org

[neis, 2015-12-03 12:01:24]

Thanks, lgtm. Minor comments below.

https://codereview.chromium.org/1492923002/diff/40001/test/mjsunit/harmony/pr...
File test/mjsunit/harmony/proxies-define-property.js (right):

https://codereview.chromium.org/1492923002/diff/40001/test/mjsunit/harmony/pr...
test/mjsunit/harmony/proxies-define-property.js:89: { value: "value2",
configurable: true }));
assertTrue

https://codereview.chromium.org/1492923002/diff/40001/test/mjsunit/harmony/pr...
test/mjsunit/harmony/proxies-define-property.js:94: })();
assertFalse

https://codereview.chromium.org/1492923002/diff/40001/test/mjsunit/harmony/pr...
File test/mjsunit/harmony/proxies-get.js (right):

https://codereview.chromium.org/1492923002/diff/40001/test/mjsunit/harmony/pr...
test/mjsunit/harmony/proxies-get.js:109: assertThrows(function() {
Can you move the assertThrows down to the line that throws?  Makes it easier to
read the test.

[caitp (gmail), 2015-12-03 13:03:39]

On 2015/12/03 12:01:24, neis wrote:
> Thanks, lgtm. Minor comments below.
> 
>
https://codereview.chromium.org/1492923002/diff/40001/test/mjsunit/harmony/pr...
> File test/mjsunit/harmony/proxies-define-property.js (right):
> 
>
https://codereview.chromium.org/1492923002/diff/40001/test/mjsunit/harmony/pr...
> test/mjsunit/harmony/proxies-define-property.js:89: { value: "value2",
> configurable: true }));
> assertTrue
> 
>
https://codereview.chromium.org/1492923002/diff/40001/test/mjsunit/harmony/pr...
> test/mjsunit/harmony/proxies-define-property.js:94: })();
> assertFalse
> 
>
https://codereview.chromium.org/1492923002/diff/40001/test/mjsunit/harmony/pr...
> File test/mjsunit/harmony/proxies-get.js (right):
> 
>
https://codereview.chromium.org/1492923002/diff/40001/test/mjsunit/harmony/pr...
> test/mjsunit/harmony/proxies-get.js:109: assertThrows(function() {
> Can you move the assertThrows down to the line that throws?  Makes it easier
to
> read the test.

done, but needs an owner to sign off

[Jakob Kummerow, 2015-12-03 14:13:59]

jkummerow@chromium.org changed reviewers:
+ jkummerow@chromium.org

[Jakob Kummerow, 2015-12-03 14:14:00]

I'm surprised that we even need these extra checks. Can you explain why? Is
there a way to trigger wrong behavior without using internals (like
%CreatePrivateSymbol)?

[caitp (gmail), 2015-12-03 14:15:21]

On 2015/12/03 14:14:00, Jakob wrote:
> I'm surprised that we even need these extra checks. Can you explain why? Is
> there a way to trigger wrong behavior without using internals (like
> %CreatePrivateSymbol)?

See the ArrayIterator example in "proxies-get.js", currently private symbols are
leaking to trap handlers, which should not happen

[Toon Verwaest, 2015-12-03 14:20:15]

What about never returning JSPROXY in the LookupIterator if name is private?

[rossberg, 2015-12-03 14:30:01]

lgtm

[Jakob Kummerow, 2015-12-03 14:31:29]

Not lgtm.

The fix Toon has in mind (and I agree with) is the following:

diff --git a/src/lookup.cc b/src/lookup.cc
index be0edaa..3b953c9 100644
--- a/src/lookup.cc
+++ b/src/lookup.cc
@@ -609,7 +609,10 @@ LookupIterator::State LookupIterator::LookupInHolder(Map*
const map,
   }
   switch (state_) {
     case NOT_FOUND:
-      if (map->IsJSProxyMap()) return JSPROXY;
+      if (map->IsJSProxyMap()) {
+        if (!name_.is_null() && name_->IsPrivate()) return NOT_FOUND;
+        return JSPROXY;
+      }
       if (map->is_access_check_needed() &&
           (IsElement() || !isolate_->IsInternallyUsedPropertyName(name_))) {
         return ACCESS_CHECK;

You can keep the one test case that doesn't rely on %CreatePrivateSymbol.

Let's not add checks for things that can't happen; such checks only serve to
confuse future readers of the code.

[caitp (gmail), 2015-12-03 14:39:09]

On 2015/12/03 14:31:29, Jakob wrote:
> Not lgtm.
> 
> The fix Toon has in mind (and I agree with) is the following:
> 
> diff --git a/src/lookup.cc b/src/lookup.cc
> index be0edaa..3b953c9 100644
> --- a/src/lookup.cc
> +++ b/src/lookup.cc
> @@ -609,7 +609,10 @@ LookupIterator::State LookupIterator::LookupInHolder(Map*
> const map,
>    }
>    switch (state_) {
>      case NOT_FOUND:
> -      if (map->IsJSProxyMap()) return JSPROXY;
> +      if (map->IsJSProxyMap()) {
> +        if (!name_.is_null() && name_->IsPrivate()) return NOT_FOUND;
> +        return JSPROXY;
> +      }
>        if (map->is_access_check_needed() &&
>            (IsElement() || !isolate_->IsInternallyUsedPropertyName(name_))) {
>          return ACCESS_CHECK;
> 
> You can keep the one test case that doesn't rely on %CreatePrivateSymbol.
> 
> Let's not add checks for things that can't happen; such checks only serve to
> confuse future readers of the code.

Fair enough, done

[Jakob Kummerow, 2015-12-03 14:40:14]

OK, LGTM.

[caitp (gmail), 2015-12-03 14:42:58]

Description was changed from

==========
[proxies] do not leak private symbols to proxy traps

BUG=v8:4537
LOG=N
R=neis@chromium.org, rossberg@chromium.org
==========

to

==========
[proxies] do not leak private symbols to proxy traps

BUG=v8:4537
LOG=N
R=neis@chromium.org, rossberg@chromium.org, jkummerow@chromium.org
==========

[caitp (gmail), 2015-12-03 14:43:18]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-12-03 14:43:19]

The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1492923002/#ps80001
(title: "Move check to LookupIterator")

[commit-bot: I haz the power, 2015-12-03 14:43:23]

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

[rossberg, 2015-12-03 14:49:53]

I don't think the latest change is correct. Not all property accesses are
lookups, so only catching this in the lookup iterator doesn't sound right.

[caitp (gmail), 2015-12-03 14:51:17]

On 2015/12/03 14:49:53, rossberg wrote:
> I don't think the latest change is correct. Not all property accesses are
> lookups, so only catching this in the lookup iterator doesn't sound right.

I agree, but I don't think it makes an observable difference, so maybe it
doesn't matter

[Jakob Kummerow, 2015-12-03 14:51:42]

On 2015/12/03 14:49:53, rossberg wrote:
> I don't think the latest change is correct. Not all property accesses are
> lookups, so only catching this in the lookup iterator doesn't sound right.

Most trap calls are initiated from user code. User code can't access private
symbols. Internal code that does work with private symbols should ensure not to
call into *any* user code, including proxy traps. If there other cases than
lookups of private symbols where this can happen, we should fix those as well.
Are there any?

[commit-bot: I haz the power, 2015-12-03 15:04:28]

Description was changed from

==========
[proxies] do not leak private symbols to proxy traps

BUG=v8:4537
LOG=N
R=neis@chromium.org, rossberg@chromium.org, jkummerow@chromium.org
==========

to

==========
[proxies] do not leak private symbols to proxy traps

BUG=v8:4537
LOG=N
R=neis@chromium.org, rossberg@chromium.org, jkummerow@chromium.org
==========

[commit-bot: I haz the power, 2015-12-03 15:04:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-03 15:05:11]

Description was changed from

==========
[proxies] do not leak private symbols to proxy traps

BUG=v8:4537
LOG=N
R=neis@chromium.org, rossberg@chromium.org, jkummerow@chromium.org
==========

to

==========
[proxies] do not leak private symbols to proxy traps

BUG=v8:4537
LOG=N
R=neis@chromium.org, rossberg@chromium.org, jkummerow@chromium.org

Committed: https://crrev.com/3ed71daff4f378e9562ef28ae6077829e3c4bd7e
Cr-Commit-Position: refs/heads/master@{#32570}
==========

[commit-bot: I haz the power, 2015-12-03 15:05:12]

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

[rossberg, 2015-12-03 15:31:48]

On 2015/12/03 14:51:42, Jakob wrote:
> On 2015/12/03 14:49:53, rossberg wrote:
> > I don't think the latest change is correct. Not all property accesses are
> > lookups, so only catching this in the lookup iterator doesn't sound right.
> 
> Most trap calls are initiated from user code. User code can't access private
> symbols. Internal code that does work with private symbols should ensure not
to
> call into *any* user code, including proxy traps. If there other cases than
> lookups of private symbols where this can happen, we should fix those as well.
> Are there any?

I don't know, but I would rather not rely on it. Any oversight would be a
potential security risk. The whole point of private symbols is that they don't
leak, by construction. We should implement them that way, correctly, and not
just rely on brittle coding conventions.

[Toon Verwaest, 2015-12-03 15:36:22]

So I don't understand what you are saying. How can you do a property access
without doing a lookup?

As for security: that's exactly the point of the current implementation. There's
a single bottleneck for all lookups, through the LookupIterator. Having 10
copies that manually check means you can forget 4 others.
We already filter out symbols for interceptors in exactly the same way. The
configuration is updated to skip interceptors.

[rossberg, 2015-12-03 15:37:28]

On 2015/12/03 15:36:22, Toon Verwaest wrote:
> So I don't understand what you are saying. How can you do a property access
> without doing a lookup?

E.g. defineProperty?

[rossberg, 2015-12-03 15:40:19]

On 2015/12/03 15:37:28, rossberg wrote:
> On 2015/12/03 15:36:22, Toon Verwaest wrote:
> > So I don't understand what you are saying. How can you do a property access
> > without doing a lookup?
> 
> E.g. defineProperty?

More generally, attempts to add or set a private property should reliably be
rejected for proxies.

[caitp (gmail), 2015-12-03 15:41:38]

On 2015/12/03 15:40:19, rossberg wrote:
> On 2015/12/03 15:37:28, rossberg wrote:
> > On 2015/12/03 15:36:22, Toon Verwaest wrote:
> > > So I don't understand what you are saying. How can you do a property
access
> > > without doing a lookup?
> > 
> > E.g. defineProperty?
> 
> More generally, attempts to add or set a private property should reliably be
> rejected for proxies.

Yes, defineProperty does still leak, it looks like :(

```
var secret = %CreatePrivateSymbol("secret");
var O = {};
var proxy = new Proxy(O, {
  get(t, n) {
    print(`[[Get]](proxy, ${String(n)})`);
    return Reflect.get(t, n);
  },

  set(t, n, v) {
    print(`[[Set]](proxy, ${String(n)}, ${String(v)})`);
    return Reflect.set(t, n, v);
  },

  has(t, n) {
    print(`[[Has]](proxy, ${String(n)})`);
    return Reflect.has(t, n);
  },

  deleteProperty(t, n) {
    print(`[[Delete]](proxy, ${String(n)})`);
    return Reflect.deleteProperty(t, n);
  },

  defineProperty(t, n, d) {
    print(`proxy.[[DefineOwnProperty]](${String(n)}, ${JSON.stringify(d)})`);
    return Reflect.defineProperty(t, n, d);
  }
});

proxy[secret] = "meow";
print(`1. ${proxy[secret]}`);
print(`2. ${Reflect.set(proxy, secret, "meow")}`);
print(`3. ${Object.defineProperty(proxy, secret, { value: "meow", configurable:
true })}`);
print(`4. ${Reflect.defineProperty(proxy, secret, { value: "meow", configurable:
true })}`);
```

yields
```
1. undefined
2. false
proxy.[[DefineOwnProperty]](Symbol(secret),
{"value":"meow","configurable":true})
[[Get]](proxy, Symbol(Symbol.toPrimitive))
[[Get]](proxy, toString)
[[Get]](proxy, Symbol(Symbol.toStringTag))
3. [object Proxy]
proxy.[[DefineOwnProperty]](Symbol(secret),
{"value":"meow","configurable":true})
4. true
```

with step 4 indicating that Reflect.defineProperty() can cause the leak :|

[Toon Verwaest, 2015-12-03 15:43:04]

We don't care about those cases really since it's not a property access. That's
like saying no function call should ever accept a private symbol unless you know
which function and which argument. Once you leak it through a function call it
can be used somewhere else to access through the "allowed" mechanism. Are you
saying we should sanitize all arguments of all function calls?

[Toon Verwaest, 2015-12-03 15:45:20]

Object.defineProperty = function(o, k, v) { leak(k); } yay :)

[Jakob Kummerow, 2015-12-03 16:00:52]

Caitlin, whenever %CreatePrivateSymbol() is involved, all bets are off anyway,
and that's never the case in production code.

Object.defineProperty (or any other function) called from regular JS code
doesn't pose a risk, as such code doesn't have access to any private symbols it
could pass in.

Andreas, if you're concerned about internal V8 C++ code calling
JSReceiver::DefineOwnProperty(some_proxy, some_private_symbol, ...) (which would
be a bug), then feel free to add a DCHECK (which is the appropriate way to guard
against such bugs). Then again I don't see how this case is special compared to
any other internal function call that can end up calling user code.

The fully bullet-proof way to check for such bugs would be to validate every
argument to every call in Execution::Call to be a "public" value (i.e. one
that's safe to pass to JS). But that's probably prohibitively expensive even in
Debug mode (and we've been fine for years without such a DCHECK).

[caitp (gmail), 2015-12-03 16:02:22]

I was just pointing out that it can still leak. So it means that it's something
for chrome extension and v8 reviewers to watch out for (although I doubt it
comes up often).

[rossberg, 2015-12-03 16:18:50]

I think there is some confusion here. Our internal code performs
%CreatePrivateSymbol, and adds those to objects. If there ever was a way for
user code to sneak in a proxy as that object, then those private symbols would
leak, with potentially critical security implications. That should not happen,
and it should be guaranteed _by the semantics of private symbols_ that this
cannot happen. That's the very definition of their being "private": you cannot
get access to them unless somebody hands them to you explicitly.

[Toon Verwaest, 2015-12-03 16:38:49]

As far as I can tell the only allowed operations with private symbols are
HAS_PRIVATE HAS_DEFINED_PRIVATE GET_PRIVATE and SET_PRIVATE. The last 3 are
keyed accesses, the first calls %HasOwnProperty, a known runtime function that
(after this CL) does the right thing. There's no currently no hard limitation to
which functions they can be passed, but those are the only ones for which it is
known to be safe.

Calling anything else is not guaranteed by design to work. If it needs to work,
e.g., defineProperty, then we'll have those extend that particular function.
(And we'll have to be very careful that that function cannot be replaced by the
user.)

The semantics of private symbols don't guarantee at all where they flow and how.
If there ever was a way for a user to replace a called function with another,
then those private symbols would also leak. Hence we can only allow certain
constructs such as runtime functions and known types of property access.

[rossberg, 2015-12-03 16:46:34]

It sounds like you saying that it doesn't matter to break the semantics because
there are so many other ways to trip up in JS anyway. If so, I hope your joking.
;)

The macros are rather immaterial here. Their role wasn't to ensure safety but to
abstract from a particular implementation of private state as used in our
built-ins. The requirement that private symbols shouldn't leak is orthogonal to
what they do.

In particular, private symbols are also accessible through the API.

[Toon Verwaest, 2015-12-03 17:05:16]

What I'm saying is that by design there's a limited set of operations to which
it is safe to pass private symbols. That set consists of keyed stores and loads,
and %HasOwnProperty.

What it sounds like you are saying is that "any function that does something
with properties" should be safe. That would be another set than what we have
now; and I'd be ok with that one if there's a need for it. At this point I don't
see the need for it, since we don't use that set. We use the set as mentioned
above.

In either case, the exact set of functions that accept "but don't leak" is
fairly arbitrary, and is a contract that needs to be known by the user.

There is however a secondary set of functions that does need to be safe. Those
are the functions that would leak symbols already installed on objects, such as
Object.keys. Those are language-defined and *have* to all support private
symbols.

[Toon Verwaest, 2015-12-03 19:33:13]

I think for SetProperty my suggested approach doesn't work. You'll hit the
ASSERT at
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.cc&...

[rossberg, 2015-12-03 19:36:08]

What I'm saying is that there must not be any way for user code to discover or
intercept private symbols through any reflection mechanism, including proxies. I
don't think there is much leeway. We provide private symbols through the API, so
they have to work correctly.

I don't think the set of operations is fuzzy at all. It is clearly delineated by
the MOP.

[Toon Verwaest, 2015-12-03 19:50:56]

So e.g., privateSymbol.x should be guaranteed to not cause a getter /
proxy-get-trap to be triggered since it would leak the symbol as a receiver? ...
can you write down an exhaustive list of such features?

[Toon Verwaest, 2015-12-03 19:52:59]

The question is: when do they work correctly? Afaik there's no (complete) ES6
extension for them.