Reflect.construct can cause cross-realm leakage of Date and Promise constructors

[erights]

This is a security problem now in the shim, to be described. With a likely fix to be described.

This is also a spec bug that should be fixed as the Realms spec proposal advances.

[erights]

Needs security label

[caridy]

@erights can you provide more details?

[erights]

See also @claudepache 's comment at tc39/ecma262#1313 (comment) . Is this the same bug?

(Note: I still need to explain the bug itself before this additional note will make sense.)

[erights]

Found it:

tc39/ecma262#1264

[littledan]

Could you describe the security implications a bit more? I don't see how this can lead to accessing a constructor that you don't already have access to.

[caridy]

Resolution:

The issue is really with the way Reflect.construct works, probably worth a reform, while there is nothing to do in the realm proposal to mitigate this. In fact, if you use a membrane to interact with the realm, the membrane can provide the right corrections.

[bathos]

I’m not sure how important this detail is or isn’t, but @caridy, Reflect.construct is not uniquely exposing a capability in this regard. It’s pretty obscure, but you can access the arbitrary ‘realm lookup’ behavior through new:

iframe = document.createElement('iframe');
document.head.append(iframe);
globalThat = iframe.contentWindow;

// The foreign-realm constructor is [[ConstructorKind]] "derived". It doesn’t
// matter what its heritage expression is really.

foreignConstructor = globalThat.eval(`(class extends Object {})`);

// Now we’ll set the [[Prototype]] of foreignConstructor to an intrinsic
// constructor from our own realm:

foreignConstructor.__proto__ = RegExp;

// At this point, if we instantiate it, we’ll get a RegExp-branded object...

console.assert(RegExp.prototype.exec.call(new foreignConstructor));

// ...but it won’t be inheriting from either realm’s RegExp.prototype.

console.assert(new foreignConstructor instanceof globalThis.RegExp === false);
console.assert(new foreignConstructor instanceof globalThat.RegExp === false);

// Instead, it will still be inheriting from the original foreign prototype (and
// indirectly, the foreign realm’s Object.prototype).

console.assert(new foreignConstructor instanceof foreignConstructor);
console.assert(new foreignConstructor instanceof globalThat.Object);

// At first, it may seem like there’s no way to hit a construction path using
// this constructor where new.target.prototype is not an object. This is a class
// syntax function, so prototype is non-configurable and non-writable; even a
// Proxy can’t get us past that. And if we were using function syntax, we’d need
// to use Reflect.construct — and the whole point of this exercise is to show
// that Reflect.construct isn’t actually exposing a unique capability.
//
// The twist is Bound Function Exotic Objects.

boundForeignConstructor = foreignConstructor.bind();

// Now, we can define 'prototype' on this new object, but that in itself doesn’t
// change anything if not using Reflect.construct, because a BFEO’s [[Construct]]
// pulls out `If SameValue(F, newTarget) is true, set newTarget to target`. But
// you know what’s *not* a ‘SameValue’ of F that will still hit its construct? A proxy!
// Note: GetFunctionRealm bypasses both BFEOs and PEOs for their targets!

Object.defineProperty(boundForeignConstructor, 'prototype', {});

proxiedBoundFunctionConstructor = new Proxy(boundForeignConstructor, {});

// 🥁🥁🥁

console.assert(new proxiedBoundFunctionConstructor instanceof globalThis.RegExp === false);
console.assert(new proxiedBoundFunctionConstructor instanceof globalThat.RegExp === true);
console.assert((new proxiedBoundFunctionConstructor).constructor === globalThat.RegExp);

In other words, even if our code did not have access to globalThat or Reflect.construct above, foreignConstructor would be enough to give us access to (potentially every, if it satisfied their arguments-related needs) intrinsic constructor from its own realm.

[caridy]

@bathos we discussed this case yesterday, and yes, it goes beyond Reflect.construct, but I think what we are saying is that this problem is not going to be solved with the Realms itself. We are also saying that there are two avenues to explore:

• what can we do in the language (I don't think there is much to be done there honestly).
• educate folks on this problems, and show them how this can be solved with membranes.

If will be interesting to run this example in https://github.com/caridy/secure-javascript-environment, and see how far can you go, maybe @jdalton can help.

[bathos]

Yep, I would also expect membranes to address it, since you need at least one constructable object from outside the local realm to do any of it. I offered this example because

The issue is really with the way Reflect.construct works, probably worth a reform [...]

made me think that folks might not have been aware that (a) there was an avenue to this behavior that doesn’t require Reflect.construct / (b) changes to Reflect.construct alone would not be sufficient to eliminate the issue.

[erights]

The security breach that caught us, due to the bizarre nature of Realm.construct is documented well at Agoric/realms-shim#51 (comment)

Thanks @XmiliaH for finding and filing this!

I would support reforming the semantics of Reflect.construct if someone else would take the lead on advancing it. Volunteers?

[bathos]

@erights The ‘bizarre behavior’ belongs to construction itself afaict, not Reflect.construct. Adjusting Reflect.construct won’t eliminate it; the problematic code path can be reached using new. (The example I posted above is a demonstration of doing it without Reflect.construct.)

(The difference without Reflect.construct is that you need a PEO whose target is a BFEO, whereas with Reflect.construct, a BFEO is sufficient, and that the [[ConstructorKind]] of the foreign function must be derived, though I’m not 100% certain there isn’t another path that doesn’t require that last bit.)

[erights]

OMG @bathos , should we reopen Agoric/realms-shim#51 ?

Attn @warner @jfparadis @XmiliaH

[bathos]

As caridy indicated, I think the only generic salve is membranes. I’m not aware of anything existing in a default SES env that would expose the necessary ingredients, though; while at least one new path to cross-realm prototype lookup exists, the requirements to achieve it are more difficult to satisfy than the requirements when using Reflect.construct. However I really don’t know SES stuff much, so I couldn’t say no issue or no major issue exists confidently. I just happen to know a bit about the cross-realm lookup specifically from general experimentation, as I am very attracted to JavaScript: the Weird Shit.

One thing I don’t know at all though: why it exists! I previously assumed that the lookup stemmed from a backwards compat issue, but I’m not really sure. The ES5 spec didn’t have anything that obviously corresponds to it and the only avenues I know of for reaching it require at least one thing which wasn’t introduced until ES2015 (Reflect.construct, Proxy, and derived constructors).

It’s not especially intuitive, at least to me, why there would be reason to perform these lookups based on new.target instead of the constructor, given it’s already a weird edge case (is there a ‘right’ answer for what to do if no 'prototype' is present, really?) and cross realm prototype lookup is like, a whole concept that would seemingly not otherwise need to exist. So there must be a pretty interesting story behind it...

In addition to all ES intrinsic constructors exhibiting this behavior, Web IDL interfaces replicate it, too. At least in theory. It’s not implemented the same in all browsers, or it wasn’t last time I checked anyway.

[erights]

@allenwb , what do you know of the history of this bizarre behavior? Do you think it's possible to fix it in the spec? Do you think fixing it would break anything other than tests?

[bathos]

I would love to see it eliminated if it is actually web compatible to do so.

I’m actually relying on it right now — but it’s entirely for circular reasons and it would be great to remove it. I’m using it because I’ve implemented the Web IDL construction algorithm, for Web IDL interfaces, in the ES layer, which entails this behavior. To handle it, if nt.prototype is found to not be an object, we attempt constructing Object using it as NT (proxied, to prevent a second observable/untrusted Get(nt, 'prototype') that could be different from the first). If this returns something inheriting from the local Object.prototype, we furnish the corresponding default interface prototype, but otherwise, we throw: the ES-implemented interfaces don’t actually have interface objects for each realm to be looking up like the platform-implemented ones do, and I’d rather throw for unrealizable behavior than do something randomly different.

[jfparadis]

@bathos I too would love to see that system eliminated. There was a very informative thread last year questioning the behavior:
tc39/ecma262#1264

The fallback mechanism is actually in GetPrototypeFromConstructor:

3. Let proto be ? Get(constructor, "prototype").
4. If Type(proto) is not Object, then
   a. Let realm be ? GetFunctionRealm(constructor).
   b. Set proto to realm's intrinsic object named intrinsicDefaultProto.
5. Return proto.
   https://www.ecma-international.org/ecma-262/#sec-getprototypefromconstructor

It's is triggered almost every time a function takes a constructor to create a new object:

Reflect.construct()

var iframe = document.createElement('iframe');
document.body.append(iframe);
var other = iframe.contentWindow;

var C = new other.Function();
C.prototype = null;

var o = Reflect.construct(Function, [], C);

console.assert(Object.getPrototypeOf(o) === other.Function.prototype);

Array.from()

var iframe = document.createElement('iframe');
document.body.append(iframe);
var other = iframe.contentWindow;

var C = new other.Function();
C.prototype = null;

var o = Array.from.call(C, []);

console.assert(Object.getPrototypeOf(o) === other.Object.prototype);

Generators

var iframe = document.createElement('iframe');
document.body.append(iframe);
var other = iframe.contentWindow;

var g = other.eval('(function*() {})');
var otherGeneratorPrototype = Object.getPrototypeOf(g.prototype);
g.prototype = null;

var o = g();

console.assert(Object.getPrototypeOf(o) === otherGeneratorPrototype);

new TypedArray from typedArray

var iframe = document.createElement('iframe');
document.body.append(iframe);
var other = iframe.contentWindow;

var C = new other.Function();
C.prototype = null;

var ctor = {};
ctor[Symbol.species] = C;

var typedArray1 = new Int8Array();
typedArray1.buffer.constructor = ctor;
var typedArray2 = new Int8Array(typedArray1);

console.assert(Object.getPrototypeOf(typedArray2.buffer) === other.ArrayBuffer.prototype);

etc, etc, etc

@erights For the realm-shim, the fix doesn't rely on taming Reflect.construct but instead on preventing constructors belonging to foreign realms (always function C above) from being exposed in the realm where the potentially adversary code is executed.

For the same reason, membranes can't let any function escape. At the time, I created a proxy/membrane to isolate functions.

[allenwb]

@allenwb , what do you know of the history of this bizarre behavior? Do you think it's possible to fix it in the spec? Do you think fixing it would break anything other than tests?

I thing I covered that in my two replies to tc39/ecma262#1264

Further questions should probably start from that?