proposal-array-is-template-object icon indicating copy to clipboard operation
proposal-array-is-template-object copied to clipboard

Resolve Realm-agnosticism

Open mikesamuel opened this issue 6 years ago • 64 comments

This is a separate consensus seeking issue to resolve a blocker for #2.

+@ljharb who suggested Realm-agnostic current semantics. +@erights who has concerns about the proposal in its current form and is a reviewer +@jridgewell who is the other reviewer. +@littledan who raised #3

Please correct me if I'm wrong, but IIUC, in https://github.com/tc39/proposal-array-is-template-object/issues/9#issuecomment-501697638 Mark is concerned that to preserve Proxy transparency (which impacts SES), a proxy-compatible version of Array.isTemplateObject would need a step 3 like that in Array.isArray.

7.2.2 IsArray ( argument )

The abstract operation IsArray takes one argument argument, and performs the following steps:

  1. If Type(argument) is not Object, return false.
  2. If argument is an Array exotic object, return true.
  3. If argument is a Proxy exotic object, then a. If argument.[[ProxyHandler]] is null, throw a TypeError exception. a. Let target be argument.[[ProxyTarget]]. a. Return ? IsArray(target).

The previous spec text was realm-gnostic.

https://github.com/tc39/proposal-array-is-template-object/blob/f689383dbf9061497dbc8b6c421619ec0eb6b730/spec.emu#L37-L44

Which affected this test:

https://github.com/tc39/proposal-array-is-template-object/blob/6c8e1e0e1970ad8dc0e4e44d84fb5a92cf9f60a9/test262/test/built-ins/Array/is-template-object.js#L44-L48

Is that an accurate summary?

Would people be ok with going back to the realm-gnostic version?

mikesamuel avatar Aug 10 '19 18:08 mikesamuel

I wouldn’t. You already have to virtualize everything that does brand checks in order to control cross-realm behavior; why is this any different than all the other mechanisms?

The Array.isArray behavior is currently an aberration; it’d have been nice if it applied to all internal slot lookups, but as it is, i don’t see the value in adding a second special case.

ljharb avatar Aug 10 '19 19:08 ljharb

@ljharb To be clear, I'm not proposing replicating the Array.isArray behavior.

I'm asking about going back to the realm gnostic behavior: https://github.com/tc39/proposal-array-is-template-object/blob/f689383dbf9061497dbc8b6c421619ec0eb6b730/spec.emu

mikesamuel avatar Aug 10 '19 20:08 mikesamuel

That doesn’t make any sense to me either - if the point of this proposal is to identify objects created by a literal template tag, why would one in another realm be any different?

Slots are cross realm. Unless all of them become same-realm (i doubt this is web compat), they should all stay cross-realm.

ljharb avatar Aug 10 '19 22:08 ljharb

@ljharb , @erights has some concerns about interactions with js-membranes.

I think the outcome that he wants to avoid is a realm-agnostic Array.isTemplateObject returns false for a faithful proxy of a template array from another realm.

I think the idea is that if you have two realms, R and S, connected by some channel C.

  • R passes a template object, o, over C to S.
  • S receives a Proxy(o), op
  • S calls Array.isTemplateObject(op)

Presently, the realm-agnostic Array.isTemplateObject(op) returns false.

My understanding is that @erights would be ok with

  • A realm-gnostic Array.isTemplateObject(op) returns false
  • A realm-agnostic Array.isTemplateObject(op) returns true but would rather not replicate the Array.isArray trick to achieve that.

mikesamuel avatar Aug 11 '19 19:08 mikesamuel

The way to manage that is the same way to manage everything else that brand checks - virtualize Array.isTemplateObject too.

I don’t think it’s a reasonable request to repeat the deviation that is isArray anywhere else, when there’s hundreds of other places the standard pattern (of virtualizing the brand checker) applies.

ljharb avatar Aug 11 '19 19:08 ljharb

@ljharb, In this scenario, realm S is using its own Array.isTemplateObject.

I don’t think it’s a reasonable request to repeat the deviation that is isArray anywhere else

AFAICT, noone is requesting that. That's why I'm asking about realm-gnostic Array.isTemplateObject.

mikesamuel avatar Aug 11 '19 20:08 mikesamuel

There shouldn’t be a need to wrap o in a proxy, it’s a deeply frozen object.

jridgewell avatar Aug 11 '19 20:08 jridgewell

@mikesamuel some current axioms we have are a) internal slots are always cross-realm, b) except for Array.isArray and [[Call]], Proxies do not forward internal slots. Realm-gnostic violates the first, Proxy unwrapping violates the second.

ljharb avatar Aug 11 '19 20:08 ljharb

There shouldn’t be a need to wrap o in a proxy, it’s a deeply frozen object.

Deeply frozen objects may have unfrozen prototypes.

const o = {};
console.log(o.x);  // -> undefined
Object.freeze(o);
console.log(o.x);  // -> undefined
Object.getPrototypeOf(o).x = 1;
console.log(o.x);  // -> 1

mikesamuel avatar Aug 12 '19 15:08 mikesamuel

@erights Thanks for organizing that discussion. I hope you're feeling better. I know you still have some outstanding concerns, and you're short on time. Are there any questions I might help provide clarity on?

mikesamuel avatar Aug 22 '19 19:08 mikesamuel

We discussed my major concern: That the security purpose of addressing your threat model is only well served by a per-realm (in particular, per-compartment) check. However, we did not get to a secondary-but-important concern: yet another compromise of membrane transparency. We should stop adding cross-realm internal slots. Each one creates a painful difference between a root-realm boundary and a root-realm-membrane boundary.

Mostly, this non-transparency is not visible to normal use patterns, because normally builtin methods that operate on an exotic object are looked up from the exotic object. It's the difference between:

aDate.getFullYear()  // works when a sDate is across a membrane
Date.prototype.getFullYear.call(aState)  // does not

However, the difference between arrays and non-arrays is more visible, via Array.isArray, JSON.stringify and more. Thus, we made array-ness punch through proxies. If we support a realm-independent Array.isTemplateObject on Array, then we should have template-ness also punch through proxies. Unlike Date.prototype.getFullYear.call(aState), Array.isTemplateObject would be too visible and too clearly marked for normal use to do otherwise. But we also need to beware the slippery slope. We must stop adding cases to what punches through proxies. Rather, we need to stop adding new cross-realm exotic internal slots.

A reminder: If we do make the test per-realm and thus per-compartment, we should put the test on eval, i.e., eval.isTemplateObject rather than Array.isTemplateObject, since the test will be specific to an evaluation context.

erights avatar Aug 23 '19 09:08 erights

How else can i do a cross-realm brand check?

ljharb avatar Aug 23 '19 14:08 ljharb

@ljharb

If Array.isTemplateObject is per-realm, and it lived at Array.isTemplateObject then a cross-realm-predicate could be done approximately (breakable via intrinsic tinkering) by reaching through the prototype to get the appropriate per-realm predicate:

function isTemplateObjectCrossRealm(x) {
  // Not a source of error since all template objects are arrays.
  if (!Array.isArray(x)) { return false; }
  const proto = Object.getPrototypeOf(x);
  // Not a source of error since all template objects have a prototype that is %Array%.prototype
  if (!Array.isArray(proto)) { return false; }
  const { constructor } = proto;
  // A possible source of error due to tinkering in x's realm.  May false negative.
  if (typeof constructor !== 'function') { return false; }
  const { isTemplateObject } = constructor;
  // A possible source of error due to tinkering in x's realm.  May false negative.
  if (typeof isTemplateObject !== 'function') { return false; }
  // A possible source of error due to tinkering in x's realm.  May fail either direction.
  return !!isTemplateObject(x);
}

mikesamuel avatar Aug 23 '19 14:08 mikesamuel

That can’t be trusted because the other realm may have replaced the function - the only thing i can trust is caching a function in my own realm prior to untrusted code running.

ljharb avatar Aug 23 '19 14:08 ljharb

@ljharb. True. Hence the "breakable via instrinsic tinkering".

mikesamuel avatar Aug 23 '19 14:08 mikesamuel

Breakable means it’s not a brand check :-)

ljharb avatar Aug 23 '19 14:08 ljharb

the only thing i can trust is caching a function in my own realm prior to untrusted code running.

Quite right, a parent realm could maintain a Map from descendent Realms' Array.prototype to isTemplateObject.

mikesamuel avatar Aug 23 '19 14:08 mikesamuel

Edited language s/brand check/predicate/.

A predicate is sufficient for the non-security use cases.

The per-realm brand check is sufficient for known security use cases.

mikesamuel avatar Aug 23 '19 14:08 mikesamuel

I don't understand the language about "yet another compromise of membrane transparency". Membrane transparency is nowhere near the default. Almost nothing using internal slots operates in a membrane-transparent way. Private fields are similarly non-transparent. The proposal is simply following the normal pattern. I'm not in favor of making a habit of piercing through Proxies in cases like this.

littledan avatar Sep 02 '19 14:09 littledan

Almost nothing using internal slots operates in a membrane-transparent way.

Almost all use of internal slots is membrane transparent. See the Date example at https://github.com/tc39/proposal-array-is-template-object/issues/10#issuecomment-524248326 above.

Private fields are similarly non-transparent.

Private fields are completely fully totally membrane transparent with no exception. If realm R1 has a class C1, from evaluating source code for class C, and realm R2 has a class C2 from evaluating the same code C, C1 and C2 already cannot access each other's private fields. If you place a full membrane between these realms, it is behaviorally identical. C1 only encounters membrane proxies for C2, and vice versa. But what they see is that they cannot access each other's private fields in exactly the same way. Neither can tell that they're talking to proxies rather than the other class.

Were internal slots accessible only per realm, and not across realms, then membrane transparency would be almost perfect across the entire language.

erights avatar Sep 02 '19 14:09 erights

I'm not in favor of making a habit of piercing through Proxies in cases like this.

Neither am I. I prefer not to create the problem in the first place.

erights avatar Sep 02 '19 14:09 erights

I'm sorry, I used "membrane transparent" when I meant to say "proxy forwarding". I believe none of these forwards through Proxies, and all of them should work out well with membranes (including the current specification draft as is).

littledan avatar Sep 02 '19 14:09 littledan

The problem is that

aDate.getFullYear()  // is normal usage
Date.prototype.getFullYear.call(aState)  // is unusual, so breakage less painful
Array.isArray(x)  // is normal usage, so breakage would have been painful
Array.isTemplate(x)  // would be normal usage, so breakage would be painful

That last line would become true with this proposal as it stands, creating a problem that we have successfully avoided.

I recommend that we fix this by making the Array.isTemplate test per-realm, which we can do easily since the specification already has the semantic state (specification data structure) of a per-realm collection of all templates created in that realm. We need merely specify that eval.isTemplate query that data structure in its own realm. We do not need to add any more semantic state to the spec to accomplish this.

Only if we make the terrible decision to make this test cross-realm, do I advocate that it punch through proxies, as Array.isArray did. I agree this would be horrible. Please let's not create the need for this kludge.

erights avatar Sep 02 '19 14:09 erights

Hi! I'll take over championing this from @mikesamuel.

Things might have changed since the last update here, and I was also not part of the offline discussion about this issue, so I might be lacking some context. Perhaps some of those concerns are no longer true. What are the next best steps, given that there seems to be conflicting and mutually-exclusive requests from @ljharb and @erights ?

Having read this thread, I disagree that we should replace the Array.isTemplate with eval.isTemplate - it's very non-idiomatic, and I don't see a strong enough reason to do so. Realm-agnostic check is preferable for web-compatibility and the reasons already mentioned in the thread, and the implementation based on internal slot follows from that. I agree with @ljharb we should not follow the proxy-piercing Array.isArray hack here.

@erights, how is Array.isTemplate case in your example different from Date.prototype.getFullYear.call(aState) and why you don't consider the latter an issue? As currently described, you seem to argue that the former might become "popular", "too clearly visible and marked for normal use", while the latter is rare. Can one account for that fact and simply virtualize Array.isTemplateObject like @ljharb suggests? That seems like a simple solution to a problem that very rarely would occur in practice, whereas realm-gnosticism is very invasive to the spec.

koto avatar Nov 26 '20 13:11 koto

@erights, friendly ping.

koto avatar Dec 09 '20 15:12 koto

Moved back here for clarity, this is a continuation of the discussion with @erights in https://github.com/tc39/proposal-array-is-template-object/issues/15#issuecomment-764853568.

The terms "security" and "security boundary" are unclear umbrella terms that mean different things to different people. See A Taxonomy of Security Issues for understanding language-based security and modularity and please explain in more precise terms what you mean. I suspect this remains the heart of our disagreement.

In my understanding (apologies If I misunderstood it!) the essay proposes a taxonomy that seems to be advocating for creating finer-than-process-level boundaries inside the language, and claims that they are secure - as in; they preserve integrity across boundaries (as a sidenote, I don't think that claim is sufficiently evidenced in the essay, but I'm definitely missing a larger context).

Membranes based on Proxies can surely be used to provide some level of separation between different parts of code; both sides of the membrane could also be running in different Realms. Such a setup could be called a security boundary within some threat model, but it doesn't follow that Realms are security boundaries themselves - and Javascript already does not make it easy to use them as such. Even with identity discontinuity there are many intentional "leaks" across Realms (e.g. internal slots).

If I understand correctly you oppose adding an algorithm that relies on the internal slot value (and as such is cross-realm). Would you agree that we should also change other algorithms not to use internal slots? For example:

// anotherRealm is e.g. a sameOriginIframe.contentWindow

JSON.stringify({"foo": 1, "bar": 1}, [new anotherRealm.String("foo")], new anotherRealm.Number(4))
ArrayBuffer.isView(new anotherRealm.Int32Array());
Promise.resolve(anotherRealm.Promise.resolve(4))

All of the above functions will read from internal slots, and, if I understand your objection, without proxy-piercing, they are not "safe" (please explain safety in this context, I feel we still are not on the same page here).

Note that I avoided using vectors based on prototype like in https://github.com/tc39/proposal-array-is-template-object/issues/10#issuecomment-527172900 as you consider these unusual. Calling functions on objects from a different Realm is instead quite common (imagine the objects are not created inline like above, but taken from their realm elsewhere). What makes Array.isTemplateObject worse than the established Promise resolution mechanism?

I am not. [OK with the limitation that eval in any reachable Realm weakens the brand check as the template objects can be created from strings there].

Can you expand on that? Can't the eval-capable Realm simply redefine Array.isTemplateObject in the checking Realm instead or otherwise confuse this check? In any case, many existing mechanisms may be subverted from dynamic code evaluation (and we're tackling that too separately!) and I don't think Array.isTemplateObject is very special in that regard. With eval() in any Realm you can't really trust your prototype chain, Function.apply etc.

I also don't see a way around it. For a truly robust-against-eval (even only local eval!) mechanism the Array.isTemplateObject would have to throw under eval presence (I guess detected via a host hook), and that seems undesirable to me. Array.isTemplateObject is "just" a brand check, with all regular limitations.

That said, it is still possible to create a robust check - just assert that eval is blocked in all participating Realms.

The argument I find the most convincing for cross-realm check as a primitive is that it's easy to go from it to the same-realm one, whereas the opposite is very difficult and requires tracing Realms as they get created to capture their prototypes https://github.com/tc39/proposal-array-is-template-object/issues/10#issuecomment-524337885. Would you agree?

koto avatar Jan 25 '21 13:01 koto

JSON.stringify({"foo": 1, "bar": 1}, [new anotherRealm.String("foo")], new anotherRealm.Number(4))

Is this the example you meant? I just tried it in Chrome with and without anotherRealm. and the output looks the same. See attached screenshot.

cross-realm-json

erights avatar Jan 25 '21 16:01 erights

cross-realm-isView

erights avatar Jan 25 '21 16:01 erights

What happens with these across transparent membranes? I have not yet tried that.

erights avatar Jan 25 '21 16:01 erights

Assuming the JSON example does not work across membranes, the problem is the use of the object wrappers for primitive types. For strict code, this object wrapping never happens implicitly. There is no reason for normal code to ever use object wrappers explicitly. At least I have never found a reason. In their absence, the first example doesn't arise.

The main internal state test that I'm aware of that JSON.stringify does is to test arrayness, the equivalent of Array.isArray. By design this does punch through proxies and so is transparent across membranes.

erights avatar Jan 25 '21 16:01 erights