refactor(ses)!: Divide safe evaluator scope stack into four layers

packages/ses/NEWS.md

@@ -1,5 +1,15 @@
 User-visible changes in SES:
 
+# Next release
+
+- Previous versions of SES would leak the proxy used to isolate evaluated
+  code to functions added to the global object by guest code.
+  The value of `this` in such functions should be `undefined`, but that is not
+  possible to emulate in this shim.
+  This version changes the value of `this` in such functions to be the same as
+  `globalThis` of the compartment, as would be correct in sloppy mode.
+- Removes experimental support for "known scope proxies".
+
 # v0.16.0 (2022-10-19)
 
 - When hardening a typed array, detects and locks down properties named as

packages/ses/src/commons.js

@@ -74,6 +74,7 @@ export const {
   toStringTag: toStringTagSymbol,
   iterator: iteratorSymbol,
   matchAll: matchAllSymbol,
+  unscopables: unscopablesSymbol,
   keyFor: symbolKeyFor,
   for: symbolFor,
 } = Symbol;

packages/ses/src/compartment-evaluate.js

@@ -28,11 +28,7 @@ export const provideCompartmentEvaluator = (compartmentFields, options) => {
     // shared evaluator so we need to build a new one
 
     let { globalTransforms } = compartmentFields;
-    const {
-      globalObject,
-      globalLexicals,
-      knownScopeProxies,
-    } = compartmentFields;
+    const { globalObject, globalLexicals } = compartmentFields;
 
     let localObject = globalLexicals;
     if (__moduleShimLexicals__ !== undefined) {
@@ -58,15 +54,14 @@ export const provideCompartmentEvaluator = (compartmentFields, options) => {
       globalLexicals: localObject,
       globalTransforms,
       sloppyGlobalsMode,
-      knownScopeProxies,
     }));
   }
 
   return { safeEvaluate };
 };
 
 export const compartmentEvaluate = (compartmentFields, source, options) => {
-  // Perform this check first to avoid unecessary sanitizing.
+  // Perform this check first to avoid unnecessary sanitizing.
   // TODO Maybe relax string check and coerce instead:
   // /~https://github.com/tc39/proposal-dynamic-code-brand-checks
   if (typeof source !== 'string') {

packages/ses/src/compartment-shim.js

@@ -7,7 +7,6 @@ import {
   ReferenceError,
   TypeError,
   WeakMap,
-  WeakSet,
   arrayFilter,
   arrayJoin,
   assign,
@@ -18,9 +17,9 @@ import {
   promiseThen,
   weakmapGet,
   weakmapSet,
-  weaksetHas,
 } from './commons.js';
 import {
+  setGlobalObjectSymbolUnscopables,
   setGlobalObjectConstantProperties,
   setGlobalObjectMutableProperties,
   setGlobalObjectEvaluators,
@@ -119,12 +118,6 @@ export const CompartmentPrototype = {
     return '[object Compartment]';
   },
 
-  /* eslint-disable-next-line no-underscore-dangle */
-  __isKnownScopeProxy__(value) {
-    const { knownScopeProxies } = weakmapGet(privateFields, this);
-    return weaksetHas(knownScopeProxies, value);
-  },
-
   module(specifier) {
     if (typeof specifier !== 'string') {
       throw new TypeError('first argument of module() must be a string');
@@ -277,20 +270,20 @@ export const makeCompartmentConstructor = (
 
     const globalObject = {};
 
+    setGlobalObjectSymbolUnscopables(globalObject);
+
     // We must initialize all constant properties first because
     // `makeSafeEvaluator` may use them to create optimized bindings
     // in the evaluator.
     // TODO: consider merging into a single initialization if internal
     // evaluator is no longer eagerly created
     setGlobalObjectConstantProperties(globalObject);
 
-    const knownScopeProxies = new WeakSet();
     const { safeEvaluate } = makeSafeEvaluator({
       globalObject,
       globalLexicals,
       globalTransforms,
       sloppyGlobalsMode: false,
-      knownScopeProxies,
     });
 
     setGlobalObjectMutableProperties(globalObject, {
@@ -313,7 +306,6 @@ export const makeCompartmentConstructor = (
       name: `${name}`,
       globalTransforms,
       globalObject,
-      knownScopeProxies,
       globalLexicals,
       safeEvaluate,
       resolveHook,

packages/ses/src/eval-scope.js

@@ -0,0 +1,90 @@
+import { FERAL_EVAL, create, defineProperties, freeze } from './commons.js';
+
+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: {
+      get() {
+        delete evalScope.eval;
+        return FERAL_EVAL;
+      },
+      enumerable: false,
+      configurable: true,
+    },
+  });
+
+  const evalScopeKit = {
+    evalScope,
+    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/global-object.js

@@ -1,8 +1,47 @@
-import { defineProperty, objectHasOwnProperty, entries } from './commons.js';
+import {
+  TypeError,
+  assign,
+  create,
+  defineProperty,
+  entries,
+  freeze,
+  objectHasOwnProperty,
+  unscopablesSymbol,
+} from './commons.js';
 import { makeEvalFunction } from './make-eval-function.js';
 import { makeFunctionConstructor } from './make-function-constructor.js';
 import { constantProperties, universalPropertyNames } from './whitelist.js';
 
+/**
+ * The host's ordinary global object is not provided by a `with` block, so
+ * assigning to Symbol.unscopables has no effect.
+ * Since this shim uses `with` blocks to create a confined lexical scope for
+ * guest programs, we cannot emulate the proper behavior.
+ * With this shim, assigning Symbol.unscopables causes the given lexical
+ * names to fall through to the terminal scope proxy.
+ * But, we can install this setter to prevent a program from proceding on
+ * this false assumption.
+ *
+ * @param {Object} globalObject
+ */
+export const setGlobalObjectSymbolUnscopables = globalObject => {
+  defineProperty(
+    globalObject,
+    unscopablesSymbol,
+    freeze(
+      assign(create(null), {
+        set: freeze(() => {
+          throw new TypeError(
+            `Cannot set Symbol.unscopables of a Compartment's globalThis`,
+          );
+        }),
+        enumerable: false,
+        configurable: false,
+      }),
+    ),
+  );
+};
+
 /**
  * setGlobalObjectConstantProperties()
  * Initializes a new global object using a process similar to ECMA specifications