`await` and `E.when` are not safe against evil promise reentrancy

[mhofman]

Both await and Promise.resolve use the spec PromiseResolve operation.

If the value is a native promise (recognized through an IsPromise brand-check), the operation does a Get(promise, "constructor"), which can trigger an own-property getter on a native promise (or a getter or proxy trap in its prototype chain).

If the value is anything else, PromiseResolve will call the built-in Promise [[Resolve]] function, which if the value is an object, will perform a Get(value, "then"), which similarly can trigger a getter or trap.

In both cases, this could trigger synchronous user code execution that result in unexpected reentrancy. Promise.resolve is currently called synchronously by E.when on the value (through HandledPromise.resolve).

https://github.com/endojs/endo/blob/345ea246ef4f2f8162d5073ed761136cfd7cd2d6/packages/eventual-send/src/handled-promise.js#L428-L433

Currently the only safe way to handle this is to do a basic type check before calling Promise.resolve, which will incur an extra tick for every object, including native promise, and wrap every awaited value in such a cast.

const cast = (val) => isPrimitive(val) ? Promise.resolve(val) : Promise.resolve().then(() => val)

Unfortunately there is no way to do a native promise brand check without triggering either constructor or then getters/traps.

We've had preliminary pushback from TC39 representatives regarding a Promise.isNativePromise API as it is ripe for confusion and abuse by regular developers. Most developers should not care if something is a native promise and not a simple thenable. One way to add this feature in a backhanded way would be to change Promise.prototype[Symbol.toStringTag] from a value to a getter that does an IsPromise brand check, like the TypedArray.prototype, however that might not be web compatible (in particular the SES shim would fail to initialize because of a whitelist mismatch).

An alternative would be to specify a Promise.safeResolve() that relies on the fact that the value once it passes an internal IsPromise check is guaranteed to not be a proxy, so it could check through GetOwnPropertyDescriptor if the constructor and then are safe. There are questions to resolve for that approach around the prototype chain (which could include a proxy or other custom object), including whether safeResolve would be somehow generic to work as a CustomPromise.safeResolve as well.

[Chris-Hibbert]

I'm trying to understand the extent of the problem.

Am I right to assume that it's safe to call .then() on a promise you created yourself? If the issue mainly arises from handling promises that are provided via a service's external APIs, can we make this safe by having a standard toolkit for validating parameters ensure that all ERef parameters are platform promises or remotables?

[mhofman]

As @dtribble pointed out in chat, contract code shouldn't be impacted by this issue. However the risk is in layers in the same vat under the contract code that handle promises created by potentially malicious code. Examples of such layers are ZCF and liveslots. Until we have a small membrane between these compartments that can guarantee that all values passing through are safe to handle, technically these layers are at risk. However that is only a risk we'd run into for MN-3.

[kriskowal]

Promise.safeResolve doesn’t make await safe. TC39 might need to change await behavior, at least as an effect of lockdown. If I read the situation right, we need Jessie to mandate await take an E wrapper. await null is safe, await x is not necessarily safe, we can make await E(x) safe at the cost of a tick, and we need TC39 to throw us a bone to make await x both safe and not waste a tick.

[mhofman]

Right, a generic hardened JS approach would require await semantics to switch to SafePromiseResolve, likely along with the other spec things which adopt promises like Promise.all and co., and async-from-sync-iterators. Unfortunately it's not possible to emulate new semantics for await and the iterator wrappers in a shim (at least not without code transforms, or hooks into the PromiseResolve machinery).

await E(x) however does not necessarily have to pay the cost of a tick in the most common cases. It could rely on Promise.safeResolve, which is itself shimable if we have proxy tagging/stamping capabilities (https://github.com/Agoric/agoric-sdk/issues/3905).

Finally since the problem is really between compartments, we could relent to this being a documented hazard that Agoric mitigates on its side through the membrane based on Far / Passable checks @erights already plans to introduce.