fix(resolve): protect against reentrancy attack (#401)

* fix(resolve): protect against reentrancy attack

Closes #9

* fix(resolve): harden argument to HandledPromise.resolve

The harden calls in eventual-send already need an SES-like
environment for proper security.  Make HandledPromise.resolve
simpler and prevent proxy trickery.

* fix(HandledPromise): set prototype to Promise

* fix(test-thenable): proper workaround of override mistake

packages/eventual-send/changelogs/9.txt

@@ -0,0 +1,5 @@
+* `HandledPromise.resolve(p)` (and therefore `E.resolve(p)`) now
+assimilate Promises that have been mutated to add a `.then` method.
+These are delayed to a future turn before calling their `.then` method.
+This protects against reentrancy attacks, but only when running
+under SES.

packages/eventual-send/src/E.js

@@ -62,7 +62,7 @@ export default function makeE(HandledPromise) {
         return true;
       },
       get(_target, prop) {
-        return HandledPromise.get(x, prop);
+        return harden(HandledPromise.get(x, prop));
       },
     });
 

packages/eventual-send/src/index.js

@@ -1,10 +1,19 @@
-/* global HandledPromise */
+/* global HandledPromise SES */
 
 import harden from '@agoric/harden';
 
 import makeE from './E';
 
-const { defineProperties, getOwnPropertyDescriptors } = Object;
+const {
+  defineProperties,
+  getOwnPropertyDescriptors,
+  getOwnPropertyDescriptor: gopd,
+  getPrototypeOf,
+  isFrozen,
+} = Object;
+
+const { prototype: promiseProto } = Promise;
+const { then: originalThen } = promiseProto;
 
 // 'E' and 'HandledPromise' are exports of the module
 
@@ -56,7 +65,7 @@ export function makeHandledPromise(Promise) {
   // handled Promises to their corresponding fulfilledHandler.
   let forwardingHandler;
   let handle;
-  let handledPromiseResolve;
+  let promiseResolve;
 
   function HandledPromise(executor, unfulfilledHandler = undefined) {
     if (new.target === undefined) {
@@ -224,12 +233,18 @@ export function makeHandledPromise(Promise) {
     return handledP;
   }
 
-  HandledPromise.prototype = Promise.prototype;
+  HandledPromise.prototype = promiseProto;
+  Object.setPrototypeOf(HandledPromise, Promise);
 
-  // Uncomment this line if needed for conformance to the proposal.
-  // Currently the proposal does not specify this, but we might change
-  // our mind.
-  // Object.setPrototypeOf(HandledPromise, Promise);
+  function isFrozenPromiseThen(p) {
+    return (
+      isFrozen(p) &&
+      getPrototypeOf(p) === promiseProto &&
+      promiseResolve(p) === p &&
+      gopd(p, 'then') === undefined &&
+      gopd(promiseProto, 'then').value === originalThen // unnecessary under SES
+    );
+  }
 
   const staticMethods = harden({
     get(target, key) {
@@ -253,15 +268,21 @@ export function makeHandledPromise(Promise) {
     resolve(value) {
       ensureMaps();
       // Resolving a Presence returns the pre-registered handled promise.
-      const handledPromise = presenceToPromise.get(value);
-      if (handledPromise) {
-        return handledPromise;
+      let resolvedPromise = presenceToPromise.get(value);
+      if (!resolvedPromise) {
+        resolvedPromise = promiseResolve(value);
       }
-      const basePromise = Promise.resolve(value);
-      if (basePromise === value) {
-        return value;
+      // Prevent any proxy trickery.
+      harden(resolvedPromise);
+      if (isFrozenPromiseThen(resolvedPromise)) {
+        return resolvedPromise;
       }
-      return handledPromiseResolve(value);
+      // Assimilate the thenable.
+      const executeThen = (resolve, reject) =>
+        resolvedPromise.then(resolve, reject);
+      return harden(
+        promiseResolve().then(_ => new HandledPromise(executeThen)),
+      );
     },
   });
 
@@ -340,6 +361,6 @@ export function makeHandledPromise(Promise) {
     return new HandledPromise(executor);
   };
 
-  handledPromiseResolve = Promise.resolve.bind(HandledPromise);
+  promiseResolve = Promise.resolve.bind(Promise);
   return harden(HandledPromise);
 }

packages/eventual-send/test/test-thenable.js

@@ -0,0 +1,34 @@
+import test from 'tape-promise/tape';
+import { E, HandledPromise } from '../src/index';
+
+test('E.resolve is always asynchronous', async t => {
+  try {
+    const p = new Promise(_ => {});
+    // Work around the override mistake on SES.
+    Object.defineProperty(p, 'then', { value: (res, _rej) => res('done') });
+    let thened = false;
+    const p2 = E.resolve(p).then(ret => (thened = ret));
+    t.is(thened, false, `p2 is not yet resolved`);
+    t.equal(await p2, 'done', `p2 is resolved`);
+  } catch (e) {
+    t.isNot(e, e, 'unexpected exception');
+  } finally {
+    t.end();
+  }
+});
+
+test('HandledPromise.resolve is always asynchronous', async t => {
+  try {
+    const p = new Promise(_ => {});
+    // Work around the override mistake on SES.
+    Object.defineProperty(p, 'then', { value: (res, _rej) => res('done') });
+    let thened = false;
+    const p2 = HandledPromise.resolve(p).then(ret => (thened = ret));
+    t.is(thened, false, `p2 is not yet resolved`);
+    t.equal(await p2, 'done', `p2 is resolved`);
+  } catch (e) {
+    t.isNot(e, e, 'unexpected exception');
+  } finally {
+    t.end();
+  }
+});