General re-entrancy problem when handling promise-like objects

[mhofman]

During the plenary I expressed my wish to explore the more general problem of risk of reentrancy when handling promise-like objects,

I believe all security issues are in fact rooted in this problem, and handling an object that is unexpectedly a thenable is only one of the manifestations. While unexpected thenables can also cause confusion and unexpected result values (e.g. thenable modules dynamically imported), I don't believe these would cause security issue if it the implicit promise resolve mechanism guarded against reentrancy.

Reentrancy in promise resolution

Overview of spec operations

The PromiseResolve(C, x) steps:

1. If IsPromise(x) is true, then
   a. Let xConstructor be ? Get(x, "constructor").
   b. If SameValue(xConstructor, C) is true, return x.
2. Let promiseCapability be ? NewPromiseCapability(C).
3. Perform ? Call(promiseCapability.[[Resolve]], undefined, « x »).
4. Return promiseCapability.[[Promise]].

Where [[Resolve]] for the intrinsic %Promise% constructor is:

1. Let F be the active function object.
2. Assert: F has a [[Promise]] internal slot whose value is an Object.
3. Let promise be F.[[Promise]].
4. Let alreadyResolved be F.[[AlreadyResolved]].
5. If alreadyResolved.[[Value]] is true, return undefined.
6. Set alreadyResolved.[[Value]] to true.
7. If SameValue(resolution, promise) is true, then
   a. Let selfResolutionError be a newly created TypeError object.
   b. Perform RejectPromise(promise, selfResolutionError).
   c. Return undefined.
8. If resolution is not an Object, then
   a. Perform FulfillPromise(promise, resolution).
   b. Return undefined.
9. Let then be Completion(Get(resolution, "then")).
10. If then is an abrupt completion, then
    a. Perform RejectPromise(promise, then.[[Value]]).
    b. Return undefined.
11. Let thenAction be then.[[Value]].
12. If IsCallable(thenAction) is false, then
    a. Perform FulfillPromise(promise, resolution).
    b. Return undefined.
13. Let thenJobCallback be HostMakeJobCallback(thenAction).
14. Let job be NewPromiseResolveThenableJob(promise, resolution, thenJobCallback).
15. Perform HostEnqueuePromiseJob(job.[[Job]], job.[[Realm]]).
16. Return undefined.

The most common reentrancy point in step 9 in the resolve steps above:

Get(resolution, "then")

The less common but still dangerous one is in step 1.a of Promise.resolve():

Get(x, "constructor")

Reentrancy cases

PromiseResolve

Any code using PromiseResolve(%Promise%, value) to transform a value into what they believe to be a safe promise value would encounter the following risks of reentrancy during PromiseResolve itself:

• A promise object which has an own constructor getter property
• A Promise subclass object which has a constructor getter property on its prototype
• A promise object when the %Promise.prototype%'s constructor property has been modified into an accessor.

Please note that the value returned by PromiseResolve(%Promise%, value) may not in fact be safe to handle. See below.

Resolve steps

During the Resolve Function Steps, any resolution object which has a custom then behavior may cause reentrancy. Some notable cases:

• An object with an own then accessor, including a promise object.
• A proxy object with a get trap (cannot be a promise object)
• An object with a then accessor on its prototype chain
  • an object created by the spec which inherits from %Object.prototype%, and %Object.prototype% had a .then accessor property added
  • a promise object which inherits from %Promise.prototype%, which has been modified with a then accessor property

The PromiseResolve operation may have returned a promise object with custom then behavior, either as an own property or through a modified %Promise.prototype%.

Any Call(promiseCapability.[[Resolve]] in the spec may in fact be susceptible to reentrancy if the result is one of these objects. In particular, PromiseResolve would have itself triggered these then custom behavior for any non promise object that didn't match the C promise constructor.

Proposed mitigation

Exact details TBD, but the general idea is to make PromiseResolve guaranteed to return a promise without causing reentrancy itself when called with the intrinsic %Promise% constructor. Then any spec code can use PerformPromiseThen on that returned promise and be guaranteed not to have caused reentrancy (or a throw), including Await.

User code can build a SafePromiseResolve that uses Promise.resolve() and verifies that the returned promise does not have own constructor and then properties (and inherits from %Promise.prototype% if not already checked by PromiseResolve itself), which it can all do safely since the returned promise is guaranteed to not be a proxy. The user code is then guaranteed that calling .then is safe (as long as Promise.prototype.then was not replaced, but that's not something it's in position to protect itself against unless the user code has first run on realm creation).

Unfortunately I don't think there is a way to make the "promise resolve function" not trigger any then hooks without either incurring a 1 tick delay for every non promise object used as resolution value, or allowing the creation of an async predicate that can detect whether an object is a proxy. Code interested in guarding itself against reentrancy would need to use PromiseResolve explicitly.

Related discussions

tc39/proposal-faster-promise-adoption#1 discusses some similar options for fast-pathing promise adoption which would prevent some reentrancy cases.

[mgaudet]

Thank you very much for writing this up. It's really really helpful to have this level of detail and the time to digest not in plenary. Still digesting if I'm honest :)

So make sure I am partially following: one solution (if we could make it work) would be for 'Safe' resolution to delay by one tick in cases where some operation might cause re-entrancy.

So you could imagine that Promise.resolve would conditionally adopt a given promise, unless it is potentially dangerous to do so (eg. there exists a constructor or then, ... etc). If the promise wasn't safe, resolution would be delayed instead by a microtask tick. Then in the next microtask tick you'd actually do the operations. Worst case scenario it would double the length of time a promise takes to resolve in the compatibility case?

You mentioned throwing, which I didn't understand there.

[mhofman]

You mentioned throwing, which I didn't understand there.

I was confused, I thought at some point that [[Resolve]] could throw, but it cannot.

So you could imagine that Promise.resolve would conditionally adopt a given promise

In the more limited form, yes, Promise.resolve would indeed return a new promise where today it would pass through an existing promise. Where I'm hesitant is that it requires an explicit opt-in for cases that don't use Promise.resolve today, such as using the resolvers directly: e.g. promiseCapability.resolve(foo), would have to become promiseCapability.resolve(Promise.resolve(foo)) if you don't trust foo unless promiseCapability.resolve comes with the same safety guarantees.

So make sure I am partially following: one solution (if we could make it work) would be for 'Safe' resolution to delay by one tick in cases where some operation might cause re-entrancy.

Correct. The main problem is that for resolution values which are objects, you effectively have to incur a one tick penalty because of proxies. @gibson042 is not convinced this this delay is an actual issue in real life, but given other efforts for "faster promise adoption", there seem to be some evidence that people care about these one tick delays, even if the operation is async in the first place.

We could rework the reactions triggering mechanism to mitigate this, but I suspect that there wouldn't be a massive appetite for it (and complexity with the current realm logic for reactions jobs).

[jridgewell]

The main problem is that for resolution values which are objects, you effectively have to incur a one tick penalty because of proxies.

We could carve out Proxies and thenables and still make promise resolution. I don't think making all code that resolves to an object (new Promise(r => r(obj), Promise.resolve(obj), and p.then(() => obj) would be affected) slower is acceptable.

We could introduce a new GetStoppingAtProxy(property) op that detects a proxy to not trigger any user code:

let current = obj;
while (current) {
  if (current.[[ProxyTarget]]) return { tag: ~proxy~, value: current };
  const desc = %GetOwnDesc%(current, property);
  if (desc) return { tag: ~desc~, value: current };
}
return { tag: ~safe~, value: obj }

Doing so, we could detect that the object isn't a proxy (and doesn't have a proxy in prototype chain), doesn't define a .then, and resolve immediately. If it's a proxy, we defer a tick before triggering a trap (or potentially an accessor). If it's a thenable, we defer a tick before potentially triggering an accessor.

As long as normal object (and native Promises) don't incur more ticks, I'm happy with this. But maybe this should fall in the faster-resolve proposal?

[mhofman]

that detects a proxy

That would effectively expose a way to test whether an object is a proxy or not. Albeit an asynchronous test by counting ticks, but a test that is accessible by userland. That is a non-starter. We would have to carefully consider whether that is acceptable or not.

[mhofman]

Where I'm hesitant is that it requires an explicit opt-in for cases that don't use Promise.resolve today, such as using the resolvers directly: e.g. promiseCapability.resolve(foo), would have to become promiseCapability.resolve(Promise.resolve(foo)) if you don't trust foo unless promiseCapability.resolve comes with the same safety guarantees.

If #4 is any indication of the web incompatibility of making promise resolve functions safe by default, I think that means we have no choice but to require explicit handling of potentially unsafe values before handing them to a resolver.

Maybe we can have a "GetSafePromiseCapability" operation for these places that need it? Filed #6