Promise.resolve(p).then(t => {...})` not actually safe from reentrancy attack.

[erights]

Promise.resolve(p).then(t => {...})

is not actually safe from reentrancy attack.

This vulnerability is suggested by https://github.com/Agoric/eventual-send/issues/32 but not actually stated there. We carefully designed the semantics of Promise.resolve so that it was safe from reentrancy attacks, even when its argument was a thenable that triggers absorption. We had in mind that patterns like that above would likewise be

• safe under SES because Promise.prototype.then would be frozen (good assumption)
• safe under SES because genuine promises would be born frozen (bad assumptiion)

The attacker could make a genuine promise with an own then property that executes immediately, attacking any reentrancy available to it. Thus, we must find some alternative to the above code pattern.

[erights]

@dtribble suggested two safe alternatives:

• use of await is already safe against this, as it uses the internal [[Then]] behavior rather than doing a [[Get]] lookup of the name "then".
• We could patch Promise.resolve to check if the promise has a .then own property.

I'll add:

• We could add into our E convenience an E.resolve that does what @dtribble suggests Promise.resolve should do, avoiding a battle of existing standard behavior we're likely to lose.
• We could add into our E convenience an E.when(p, onSuccess, onFailure) operation that internally does an

   // at module init time:
   const { resolve } = Promise;
   const { thenFunc } = Promise.prototype.then;
   const { apply } = Reflect;

   ...

   // static method context
   when(p, onSuccess, onFailure) {
     return apply(thenFunc, resolve(p), [onSuccess, onFailure]);
  }
```js

[erights]

The reason the new static method on E is named when rather than then is to avoid making E into a thenable, which would be terrible.

[erights]

On @dtribble 's suggested patch to Promise.resolve or my derived suggested E.resolve, @dtribble points out there are two possibilities of what it should do if it does detect an own then method on the promise:

• throw
• fall back to assimilation, as if the object were a thenable instead of a genuine promise.

[michaelfig]

• fall back to assimilation, as if the object were a thenable instead of a genuine promise.

I would like to implement assimilation for E.resolve (which is currently just another name for HandledPromise.resolve), and I think it would be reasonable to standardize as part of proposal-eventual-send.

Something like:

const { hasOwnProperty } = Object.prototype;
const { apply } = Reflect;
[...]
    static resolve(value) {
      ensureMaps();
      // Resolving a Presence returns the pre-registered handled promise.
      let resolvedPromise = presenceToPromise.get(value);
      if (!resolvedPromise) {
        resolvedPromise = baseResolve(value);
      }
      if (apply(hasOwnProperty, resolvedPromise, ['then'])) {
        // Make sure thenables are assimilated, not directly returned.
        // This prevents reentrancy attacks even in case resolvedPromise is
        // a genuine promise, but has a 'then' own property.
        return new HandledPromise(resolve => resolve(resolvedPromise));
      }
      return resolvedPromise;
    }

[erights]

The line

return new HandledPromise(resolve => resolve(resolvedPromise));

does successfully hide the resolvePromise's then, by wrapping it in a fresh promise which forwards to that one. I think it does so without interacting with resolvedPromise, protecting against reentrancy, depending on the semantics of the resolve argument, which we should check.

However, it does not assimilate resolvedPromise as if it is a thennable. Is that the intention, or did you want to emulate the full assimilation-of-thennable semantics?

[michaelfig]

Is there a reference or sample code that could assimilate as a thenable? That's my intention, of which I fell short.

[erights]

I'm about to take off, so briefly. But you should google for a test262-passing Promise implementation to get something accurate.

if (value && typeof value.then === 'function') {
  p = Promise.resolve().then(_ => new HandledPromise((resolve, reject) => value.then(resolve, reject)));
}

The initial part is just to postpone the rest of it to a later turn.

[erights]

The key idea is to use the thenable's then method's behavior to resolve the genuine promise that wraps it.

[michaelfig]

Thanks, I looked at Bluebird's implementation, and it appears the only significant difference is some defensiveness around looking up the then property (i.e. in case it has an accessor that throws).

[michaelfig]

@erights This is harder for me than it first seemed.

How do I preserve the functionality of:

HandledPromise.resolve(p) === p // when p is a Promise

as well as assimilation for arbitrary p? I don't know of any robust way to test that it didn't override its p.then() method that won't be fooled by a malicious proxy.

[erights]

Can you attack the following?

const {
  isFrozen,
  getOwnPropertyDescriptor: gopd,
  getPrototypeOf,
} = Object;

const { resolve: promiseResolve, prototype: promiseProto } = Promise;
const { then: originalThen } = promiseProto;

function isNormalPromiseThen(p) {
  return isFrozen(p) &&
         isFrozen(promiseProto) &&  // unnecessary under SES
         getPrototypeOf(p) === promiseProto &&
         promiseResolve(p) === p &&
         gopd(p, 'then') === undefined &&
         gopd(promiseProto, 'then').value === originalThen;  // unnecessary under SES
}

[michaelfig]

Ahh, yes, that gets me going! I had forgotten about isFrozen, which is key to defending without calling harden in the test.

[erights]

This issue was closed when we introduced E.when( as a safe alternative to .then(. However, as shown by the screenshot at #2885 , we have mostly failed to switch to the safe way. I am therefore reopening this issue until we are actually safe from this reentrancy attack.

[Tartuffo]

@erights Is this still an issue, and if so, does it need to be done for Mainnet 1?

[erights]

It is still an issue. A search just now shows that we still have a massive overuse of .then( that we need to replace with E.when(. But it is not a MN-1 issue.

[mhofman]

As implied in endojs/endo#1126, constructor lookup may also be a problem

[mhofman]

If we have proxy brand/stamping checks (#3905), it should be possible to implement a safe Promise.resolve operation. See endojs/endo#1181.

[erights]

If we have proxy brand/stamping checks (#3905), it should be possible to implement a safe Promise.resolve operation. See endojs/endo#1181.

See also endojs/endo#1250 , which would benefit from this.

[erights]

Hi @mhofman I just assigned the two of us. Do we have a separate issue where we document the constructor attack?