feat(ses): Revocable evalScope

packages/ses/src/eval-scope.js

@@ -1,6 +1,62 @@
-import { FERAL_EVAL, create, freeze, defineProperties } from './commons.js';
+import { FERAL_EVAL, create, defineProperties, freeze } from './commons.js';
 
-export const createEvalScope = () => {
+import { assert } from './error/assert.js';
+
+const { details: d } = assert;
+
+// We attempt to frustrate stack bumping attacks on the safe evaluator
+// (`make-safe-evaluator.js`).
+// A stack bumping attack forces an API call to throw a stack overflow
+// `RangeError` at an inopportune time.
+// The attacker arranges for the stack to be sufficiently deep that the API
+// consumes exactly enough stack frames to throw an exception.
+//
+// For the safe evaluator, an exception thrown between adding and then deleting
+// `eval` on `evalScope` could leak the real `eval` to an attacker's lexical
+// scope.
+// This would be sufficiently disastrous that we guard against it twice.
+// First, we delete `eval` from `evalScope` immediately before rendering it to
+// the guest program's lexical scope.
+//
+// If the attacker manages to arrange for `eval` to throw an exception after we
+// call `allowNextEvalToBeUnsafe` but before the guest program accesses `eval`,
+// it would be able to access `eval` once more in its own code.
+// Although they could do no harm with a direct `eval`, they would be able to
+// escape to the true global scope with an indirect `eval`.
+//
+//   prepareStack(depth, () => {
+//     (eval)('');
+//   });
+//   const unsafeEval = (eval);
+//   const safeEval = (eval);
+//   const realGlobal = unsafeEval('globalThis');
+//
+// To protect against that case, we also delete `eval` from the `evalScope` in
+// a `finally` block surrounding the call to the safe evaluator.
+// The only way to reach this case is if `eval` remains on `evalScope` due to
+// an attack, so we assume that attack would have have invalided our isolation
+// and revoke all future access to the evaluator.
+//
+// To defeat a stack bumping attack, we must use fewer stack frames to recover
+// in that `finally` block than we used in the `try` block.
+// We have no reliable guarantees about how many stack frames a block of
+// JavaScript will consume.
+// Function inlining, tail-call optimization, variations in the size of a stack
+// frame, and block scopes may affect the depth of the stack.
+// The only number of acceptable stack frames to use in the finally block is
+// zero.
+// We only use property assignment and deletion in the safe evaluator's
+// `finally` block.
+// We use `delete evalScope.eval` to withhold the evaluator.
+// We assign an envelope object over `evalScopeKit.revoked` to revoke the
+// evaluator.
+//
+// This is why we supply a meaningfully named function for
+// `allowNextEvalToBeUnsafe` but do not provide a corresponding
+// `revokeAccessToUnsafeEval` or even simply `revoke`.
+// These recovery routines are expressed inline in the safe evaluator.
+
+export const makeEvalScopeKit = () => {
   const evalScope = create(null);
   const oneTimeEvalProperties = freeze({
     eval: {
@@ -13,13 +69,22 @@ export const createEvalScope = () => {
     },
   });
 
-  return {
+  const evalScopeKit = {
     evalScope,
-    allowNextEvalToBeUnsafe: () => {
+    allowNextEvalToBeUnsafe() {
+      if (evalScopeKit.revoked !== null) {
+        // eslint-disable-next-line @endo/no-polymorphic-call
+        assert.fail(
+          d`a handler did not reset allowNextEvalToBeUnsafe ${this.revoked.err}`,
+        );
+      }
       // Allow next reference to eval produce the unsafe FERAL_EVAL.
       // We avoid defineProperty because it consumes an extra stack frame taming
       // its return value.
       defineProperties(evalScope, oneTimeEvalProperties);
     },
+    revoked: null,
   };
+
+  return evalScopeKit;
 };

packages/ses/src/make-safe-evaluator.js

@@ -4,7 +4,7 @@
 import { apply, freeze } from './commons.js';
 import { strictScopeTerminator } from './strict-scope-terminator.js';
 import { createSloppyGlobalsScopeTerminator } from './sloppy-globals-scope-terminator.js';
-import { createEvalScope } from './eval-scope.js';
+import { makeEvalScopeKit } from './eval-scope.js';
 import { applyTransforms, mandatoryTransforms } from './transforms.js';
 import { makeEvaluate } from './make-evaluate.js';
 import { assert } from './error/assert.js';
@@ -31,7 +31,8 @@ export const makeSafeEvaluator = ({
   const scopeTerminator = sloppyGlobalsMode
     ? createSloppyGlobalsScopeTerminator(globalObject)
     : strictScopeTerminator;
-  const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope();
+  const evalScopeKit = makeEvalScopeKit();
+  const { evalScope } = evalScopeKit;
 
   const evaluateContext = freeze({
     evalScope,
@@ -67,7 +68,8 @@ export const makeSafeEvaluator = ({
     ]);
 
     // Allow next reference to eval produce the unsafe FERAL_EVAL.
-    allowNextEvalToBeUnsafe();
+    // eslint-disable-next-line @endo/no-polymorphic-call
+    evalScopeKit.allowNextEvalToBeUnsafe();
 
     let err;
     try {
@@ -95,6 +97,7 @@ export const makeSafeEvaluator = ({
         // variable resolution via the scopeHandler, and throw an error with
         // diagnostic info including the thrown error if any from evaluating the
         // source code.
+        evalScopeKit.revoked = { err };
         // TODO A GOOD PLACE TO PANIC(), i.e., kill the vat incarnation.
         // See https://github.com/Agoric/SES-shim/issues/490
         // eslint-disable-next-line @endo/no-polymorphic-call

packages/ses/test/test-eval-scope.js

@@ -1,5 +1,5 @@
 import test from 'ava';
-import { createEvalScope } from '../src/eval-scope.js';
+import { makeEvalScopeKit } from '../src/eval-scope.js';
 
 // The original unsafe untamed eval function, which must not escape.
 // Sample at module initialization time, which is before lockdown can
@@ -10,23 +10,25 @@ const FERAL_EVAL = eval;
 test('evalScope - is not created with eval exposed', t => {
   t.plan(2);
 
-  const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope();
+  const evalScopeKit = makeEvalScopeKit();
+  const { evalScope } = evalScopeKit;
 
   t.is(Reflect.has(evalScope, 'eval'), false);
 
-  allowNextEvalToBeUnsafe();
+  evalScopeKit.allowNextEvalToBeUnsafe();
 
   t.is(Reflect.has(evalScope, 'eval'), true);
 });
 
 test('evalScope - getting eval removes it from evalScope', t => {
   t.plan(5);
 
-  const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope();
+  const evalScopeKit = makeEvalScopeKit();
+  const { evalScope } = evalScopeKit;
 
   t.is(Reflect.has(evalScope, 'eval'), false);
 
-  allowNextEvalToBeUnsafe();
+  evalScopeKit.allowNextEvalToBeUnsafe();
 
   t.is(Reflect.has(evalScope, 'eval'), true);
 

packages/ses/test/test-make-evaluate.js

@@ -2,7 +2,7 @@ import test from 'ava';
 import { apply, freeze } from '../src/commons.js';
 import { makeEvaluate } from '../src/make-evaluate.js';
 import { strictScopeTerminator } from '../src/strict-scope-terminator.js';
-import { createEvalScope } from '../src/eval-scope.js';
+import { makeEvalScopeKit } from '../src/eval-scope.js';
 
 const makeObservingProxy = target => {
   const ops = [];
@@ -36,7 +36,8 @@ test('makeEvaluate - optimizer', t => {
   );
 
   const scopeTerminator = strictScopeTerminator;
-  const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope();
+  const evalScopeKit = makeEvalScopeKit();
+  const { evalScope } = evalScopeKit;
 
   const evaluate = makeEvaluate(
     freeze({ scopeTerminator, globalObject, globalLexicals, evalScope }),
@@ -48,7 +49,7 @@ test('makeEvaluate - optimizer', t => {
   globalObjectOps.length = 0;
   globalLexicalsOps.length = 0;
 
-  allowNextEvalToBeUnsafe();
+  evalScopeKit.allowNextEvalToBeUnsafe();
 
   const result = apply(evaluate, globalObject, [`!foo && bar && baz`]);
 
@@ -64,15 +65,16 @@ test('makeEvaluate - strict-mode', t => {
   const globalLexicals = Object.create(null);
 
   const scopeTerminator = strictScopeTerminator;
-  const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope();
+  const evalScopeKit = makeEvalScopeKit();
+  const { evalScope } = evalScopeKit;
 
   const evaluate = makeEvaluate(
     freeze({ scopeTerminator, globalObject, globalLexicals, evalScope }),
   );
 
-  allowNextEvalToBeUnsafe();
+  evalScopeKit.allowNextEvalToBeUnsafe();
   t.throws(() => apply(evaluate, globalObject, [`foo = 42`]));
 
-  allowNextEvalToBeUnsafe();
+  evalScopeKit.allowNextEvalToBeUnsafe();
   t.throws(() => apply(evaluate, globalObject, [`with({}) {}`]));
 });