fix: make Promise.resolve safe.

[erights]

My original intention on tc39 was that an expression like

Promise.resolve(p).then(whatever);

protect the code it appears in from reentrancy attacks no matter what p is. IOW, even if p and whatever are provided by an adversary, that adversary should not be able to get of their choosing to run within the caller's turn. However, this was attackable by installing a then function` as an own property on a genuine promise.

Recently I learned that Promise.resolve does more than simply coerce p to a genuine promise. It also checks that p.constructor === Promise. This PR experiments with making Promise.prototype.constructor into an accessor function whose getter ensures the rest is safe.

This PR is currently a draft because, after I got it "working" I realized there is a remaining vulnerability: The adversary can install an own constructor property on p, overriding the protective accessor. This flaw may be fatal for this approach.

Stacked on #1115

[mhofman]

The adversary can install an own constructor property on p, overriding the protective accessor. This flaw may be fatal for this approach.

Right I was gonna say all the attacker needs to do is restore the original constructor as an own property. The only way the attacker wouldn't be able to attack this is if the original constructor was never exposed to the user code, aka that the global Promise and that Promise.prototype.constructor always evaluated to the "fake" constructor. Besides the extreme cost (forcing the engine to recreate a new promise every time it goes through PromiseResolve), I'm not even sure that would actually work correctly, since technically any promise the engine (re)creates will never satisfy the check.

[michaelfig]

I don't know enough about the mechanics to comment other than to ask: why not install a SES-specific wrapper as Promise.resolve?

[mhofman]

That would work for explicit calls to Promise.resolve but it wouldn't neuter evil promises through await or the other few places in the spec which use PromiseResolve under the hood.

[erights]

@mhofman states exactly why I was interested. But at least for the implicit resolve in await I don't think there's an actual problem because it is followed by an implicit then using the internal then which is equivalent to the original then. IOW, await does not ask the promise for the then method to use. That's part of why I was surprised that it does ask the promise for its constructor property.

We need to look at all the other places in the spec that call the internal PromiseResolve to see if they are as safe as await. I would not be surprised either way.

[erights]

@mhofman , I did not understand

Besides the extreme cost (forcing the engine to recreate a new promise every time it goes through PromiseResolve), I'm not even sure that would actually work correctly, since technically any promise the engine (re)creates will never satisfy the check.

I don't think either part is true, but I may be misunderstanding your point.

[mhofman]

We need to look at all the other places in the spec that call the internal PromiseResolve to see if they are as safe as await. I would not be surprised either way.

• Promise.prototype.finally isn't safe, as it calls .then directly. We may be able to go for a spec change here.
• All the async-from-sync iterator operations are safe since AsyncFromSyncIteratorContinuation calls PerformPromiseThen
• AsyncGeneratorAwaitReturn is similarly safe.
• As @erights mentioned, await is indeed safe as well.

finally has other issues (realm inconsistency) and needs fixing in the spec anyway, so it's probably worth getting it changed to use PerformPromiseThen.

Changing Promise.resolve would also automatically benefit Promise.all and friends, which dynamically lookup the .resolve (TIL that one too)

Wondering what the test262 coverage is on all those behaviors.

[mhofman]

• Promise.prototype.finally isn't safe, as it calls .then directly.

Actually this may be fine.

finally calls .then directly in 2 places:

• the .then of the "this" promise. I assume people doing p.finally would have to trust p to not have an evil .then in the first place. It may be surprising if anyone does Promise.prototype.finally.call(untrustedP, onFinally) tho.
• inside the thenCallback: on the promise returned by PromiseResolve of the result of the onFinally call. The problematic case here is if onFinally returns an evil promise, but since we're inside a thenCallback already, a synchronous throw would result in the rejection of the promise result of the finally call.

So TL;DR, I think the only unexcepted case may be Promise.prototype.finally.call(untrustedP, onFinally) which would defer to an evil own untrustedP.then.

[mhofman]

@mhofman , I did not understand

Let me try to think about this again, in context of the above existing defenses in await, async-from-sync, and async-generator, as well as to a lesser extent Promise.prototype.finally.

• The first 3 operations call PromiseResolve with %Promise% as the expected constructor. Let's call them "implicit resolve".
• Promise.resolve calls PromiseResolve with the "this" as the expected constructor, aka the Promise that resolve is attached to.
• Promise.prototype.finally calls PromiseResolve inside the thenCallback with the result of the SpeciesConstructor on the "this" promise instance finally was called with.

There are 3 approaches we can take:

• Make Promise.prototype.constructor a getter which inspects the "this" promise like in this PR before either returning the original constructor, or the fake constructor
• Unconditionally swap Promise.prototype.constructor and Promise to the "fake" constructor value
• Patch Promise.resolve and possibly Promise.prototype.finally

The first approach as stated a few comments above can be defeated by the attacker adding an own .constructor to the promise instance.

The second approach would cause the "implicit resolve" invocations of PromiseResolve to attempt recreating a promise instance every time, since native promises would never match because the %Promise.prototype%.constructor would no longer equal %Promise%.

The third approach might actually be sufficient since Promise.resolve is the only path that is vulnerable to passing through an evil .then. We may also want to patch Promise.prototype.finally so that it internally does apply(originalFinally, Promise.resolve(this), args).

The remaining question is the auto freeze for "implicit resolve" cases like await. How much do we care about that?

[erights]

The remaining question is the auto freeze for "implicit resolve" cases like await. How much do we care about that?

If it provided a useful guarantee, then we might care. But I don't think it does. It is at best a useful diagnostic to catch a certain kind of accident. With this diagnostic on, I was very pleased that we were green without needing to fix anything!

[erights]

Closing as a failed experiment

[mhofman]

Closing as a failed experiment

Can we open an issue to investigate patching Promise.resolve instead? I believe it would accomplish your goal.

[erights]

Closing as a failed experiment

Can we open an issue to investigate patching Promise.resolve instead? I believe it would accomplish your goal.

Yes, please do! I'd be happy to review. Thanks ;)