fix(eventual-send): E.resolve safer against constructor attack
MODIFY
packages/eventual-send/src/handled-promise.js
packages/eventual-send/src/handled-promise.js
@@ -365,12 +365,33 @@ export const makeHandledPromise = () => {
return handledP;
}
- const isFrozenPromiseThen = p => {
+ /**
+ * If the promise `p` is safe, then during the evaluation of the
+ * expressopns `p.then` and `await p`, `p` cannot mount a reentrancy attack.
+ * Unfortunately, due to limitations of the current JavaScript standard,
+ * it seems impossible to prevent `p` from mounting a reentrancy attack
+ * during the evaluation of `isSafePromise(p)`, and therefore during
+ * operations like `HandledPromise.resolve(p)` that call
+ * `isSafePromise(p)` synchronously.
+ *
+ * The `@endo/marshal` package defines a related notion of a passable
+ * promise, i.e., one for which which `passStyleOf(p) === 'promise'`. All
+ * passable promises are also safe. But not vice versa because the
+ * requirements for a promise to be passable are slightly greater. A safe
+ * promise must not override `then` or `constructor`. A passable promise
+ * must not have any own properties. The requirements are otherwise
+ * identical.
+ *
+ * @param {Promise} p
+ * @returns {boolean}
+ */
+ const isSafePromise = p => {
return (
isFrozen(p) &&
getPrototypeOf(p) === Promise.prototype &&
Promise.resolve(p) === p &&
- getOwnPropertyDescriptor(p, 'then') === undefined
+ getOwnPropertyDescriptor(p, 'then') === undefined &&
+ getOwnPropertyDescriptor(p, 'constructor') === undefined
);
};
@@ -414,7 +435,7 @@ export const makeHandledPromise = () => {
}
// Prevent any proxy trickery.
harden(resolvedPromise);
- if (isFrozenPromiseThen(resolvedPromise)) {
+ if (isSafePromise(resolvedPromise)) {
// We can use the `resolvedPromise` directly, since it is guaranteed to
// have a `then` which is actually `Promise.prototype.then`.
return resolvedPromise;

MODIFY
packages/eventual-send/test/test-thenable.js
packages/eventual-send/test/test-thenable.js
@@ -3,22 +3,75 @@ import { test } from './prepare-test-env-ava.js';
import { E, HandledPromise } from './get-hp.js';
-test('E.resolve is always asynchronous', async t => {
+const verifySimplePromise = async (t, resolve) => {
const p = new Promise(_ => {});
+ const p2 = resolve(p);
+ t.is(p, p2, 'simple promise should not coerce');
+};
+test(
+ 'E.resolve does not coerce simple promise',
+ verifySimplePromise,
+ E.resolve,
+);
+test(
+ 'HandledPromise.resolve does not coerce simple promise',
+ verifySimplePromise,
+ HandledPromise.resolve,
+);
+
+const verifyThenAttack = async (t, resolve) => {
+ const p = new Promise(_ => {});
+ let getHappened = false;
+ let callHappened = false;
// 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.is(await p2, 'done', `p2 is resolved`);
+ Object.defineProperty(p, 'then', {
+ get() {
+ getHappened = true;
+ return (res, _rej) => {
+ callHappened = true;
+ res('done');
+ };
+ },
});
+ const p2 = resolve(p).then(ret => ret);
+ t.not(p, p2, 'an own "then" should cause coercion');
+ t.is(getHappened, false, `getter not called too early`);
+ t.is(callHappened, false, `then not called too early`);
+ t.is(await p2, 'done', `p2 is resolved`);
+ t.is(getHappened, true, `getter not called too early`);
+ t.is(callHappened, true, `then not called too early`);
+};
+test('E.resolve protects against "then" attack', verifyThenAttack, E.resolve);
+test(
+ 'HandledPromise.resolve protects against "then" attack',
+ verifyThenAttack,
+ HandledPromise.resolve,
+);
-test('HandledPromise.resolve is always asynchronous', async t => {
+const verifyConstructorAttack = async (t, resolve) => {
const p = new Promise(_ => {});
+ let getHappened = false;
// 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.is(await p2, 'done', `p2 is resolved`);
+ Object.defineProperty(p, 'constructor', {
+ get() {
+ getHappened = true;
+ return Promise;
+ },
});
+ const p2 = resolve(p);
+ t.not(p, p2, 'an own "constructor" should cause coercion');
+ // This 'true' demonstrates our remaining vulnerability to reentrancy.
+ // But the fact that p coerced to a fresh p2 means that p2 cannot
+ // cause a reentrancy attack
+ t.is(getHappened, true, `same turn`);
+};
+test(
+ 'E.resolve protects against "constructor" attack',
+ verifyConstructorAttack,
+ E.resolve,
+);
+test(
+ 'HandledPromise.resolve protects against "constructor" attack',
+ verifyConstructorAttack,
+ HandledPromise.resolve,
+);

fix(eventual-send): E.resolve safer against constructor attack (#1390)