fix(ses): harden hacks v8 stack own accessor problem (#2232)

closes: #2198 
refs: #2230 #2200
https://chromium-review.googlesource.com/c/v8/v8/+/4459251 #2229 #2231


## Description

Alternative to #2229 that just hacks `harden` to directly repair a
problematic error own stack accessor property, replacing it with a data
property.

### Security Considerations

Both before and after this PR, `passStyleOf` will reject errors with the
v8 problematic error own stack accessor property, preventing the
unsafety at stake here. However, this would mean that much existing code
that used to be correct will break when run on a v8 with this problem.

### Scaling Considerations

Avoids any extra overhead on platforms without this problem, including
all platforms other than v8.

### Documentation Considerations

probably none. This PR essentially avoids the need to document the v8
problem that it masks.

### Testing Considerations

Only needed to repair one test to use `harden` rather than `freeze`, in
a case where `harden` was more natural anyway.

### Compatibility Considerations

This PR enables more errors to pass that check without further changes
to user code. #2229 had similar goals, but would still require more
changes to user code than this PR. This is demonstrated by all the test
code in #2229 that needed to be fixed that does not need to be fixed in
this PR.

### Upgrade Considerations

none

- ~[ ] Includes `*BREAKING*:` in the commit message with migration
instructions for any breaking change.~
- ~[ ] Updates `NEWS.md` for user-facing changes.~

.github/workflows/ci.yml

@@ -77,7 +77,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        node-version: [18.x, 20.x, 22.x]
+        node-version: [18.x, 20.x, 21.x]
         platform: [ubuntu-latest, windows-latest]
 
     steps:

packages/pass-style/test/test-passStyleOf.js

@@ -416,12 +416,11 @@ test('Unexpected stack on errors', t => {
 
   const carrierStack = {};
   err.stack = carrierStack;
-  Object.freeze(err);
+  harden(err);
 
   t.throws(() => passStyleOf(err), {
     message: 'Passable Error "stack" own property must be a string: {}',
   });
-  err.stack.foo = 42;
 });
 
 test('Allow toStringTag overrides', t => {

packages/ses/error-codes/SES_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR.md

@@ -0,0 +1,17 @@
+# Unexpected `Error` own `stack` accessor property (`SES_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR`)
+
+## Background
+
+Some non-standard implementations of errors have idiosyncratic unsafety problems that need idiosyncratic solutions, so the ses-shim can only repair the safety problems that fit into the categories it knows about.
+
+Firefox/SpiderMonkey, Moddable/XS, and the [Error Stack proposal](https://github.com/tc39/proposal-error-stacks/issues/26) all agree on the safest behavior, to have an `Error.prototype.stack` accessor property that is inherited by error instances, enabling an initial-load library like the ses-shim to virtualize this behavior across all errors.
+
+Safari/JSC and v8 up through Node 20 both had this appear as an own data property on error instances. This was safe enough for integrity purposes. In addition, v8 has magic error-stack initialization APIs that enabled us to hide the stack for confidentiality and determinism purposes.
+
+Starting with the v8 of Node 21, v8 makes a per-instance `stack` own accessor property, as first reported at https://github.com/tc39/proposal-error-stacks/issues/26#issuecomment-1675512619 . Fortunately, for all errors in the same realm, all their `stack` own properties use the same getter, and they all use the same setter. This enables the [ses-shim to repair](https://github.com/endojs/endo/pull/2232) some of their safety problems.
+
+## What this diagnostic means
+
+Before doing the v8 repair described above, we first do a sanity check that we're on a platform that misbehaves in precisely this way. If we see that error instances are both with own accessor `stack` properties that fail this sanity check, then we have encountered another idiosyncratic that we're not yet prepared for and do not yet know how to secure. In that case, ses-shim initialization should fail with this diagnostic.
+
+If you see this diagnostic, PLEASE let us know, and let us know what platform (JavaScript engine and version) you saw this on. Thanks!

packages/ses/src/commons.js

@@ -294,3 +294,61 @@ export const noEvalEvaluate = () => {
   // See https://github.com/endojs/endo/blob/master/packages/ses/error-codes/SES_NO_EVAL.md
   throw TypeError('Cannot eval with evalTaming set to "noEval" (SES_NO_EVAL)');
 };
+
+// ////////////////// FERAL_STACK_GETTER FERAL_STACK_SETTER ////////////////////
+
+const er1StackDesc = getOwnPropertyDescriptor(Error('er1'), 'stack');
+const er2StackDesc = getOwnPropertyDescriptor(TypeError('er2'), 'stack');
+
+let feralStackGetter;
+let feralStackSetter;
+if (er1StackDesc && er2StackDesc && er1StackDesc.get) {
+  // We should only encounter this case on v8 because of its problematic
+  // error own stack accessor behavior.
+  // Note that FF/SpiderMonkey, Moddable/XS, and the error stack proposal
+  // all inherit a stack accessor property from Error.prototype, which is
+  // great. That case needs no heroics to secure.
+  if (
+    // In the v8 case as we understand it, all errors have an own stack
+    // accessor property, but within the same realm, all these accessor
+    // properties have the same getter and have the same setter.
+    // This is therefore the case that we repair.
+    typeof er1StackDesc.get === 'function' &&
+    er1StackDesc.get === er2StackDesc.get &&
+    typeof er1StackDesc.set === 'function' &&
+    er1StackDesc.set === er2StackDesc.set
+  ) {
+    // Otherwise, we have own stack accessor properties that are outside
+    // our expectations, that therefore need to be understood better
+    // before we know how to repair them.
+    feralStackGetter = freeze(er1StackDesc.get);
+    feralStackSetter = freeze(er1StackDesc.set);
+  } else {
+    // See https://github.com/endojs/endo/blob/master/packages/ses/error-codes/SES_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR.md
+    throw TypeError(
+      'Unexpected Error own stack accessor functions (SES_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR)',
+    );
+  }
+}
+
+/**
+ * If on a v8 with the problematic error own stack accessor behavior,
+ * `FERAL_STACK_GETTER` will be the shared getter of all those accessors
+ * and `FERAL_STACK_SETTER` will be the shared setter. On any platform
+ * without this problem, `FERAL_STACK_GETTER` and `FERAL_STACK_SETTER` are
+ * both `undefined`.
+ *
+ * @type {(() => any) | undefined}
+ */
+export const FERAL_STACK_GETTER = feralStackGetter;
+
+/**
+ * If on a v8 with the problematic error own stack accessor behavior,
+ * `FERAL_STACK_GETTER` will be the shared getter of all those accessors
+ * and `FERAL_STACK_SETTER` will be the shared setter. On any platform
+ * without this problem, `FERAL_STACK_GETTER` and `FERAL_STACK_SETTER` are
+ * both `undefined`.
+ *
+ * @type {((newValue: any) => void) | undefined}
+ */
+export const FERAL_STACK_SETTER = feralStackSetter;

packages/ses/src/make-hardener.js

@@ -49,6 +49,9 @@ import {
   weakmapSet,
   weaksetAdd,
   weaksetHas,
+  FERAL_STACK_GETTER,
+  FERAL_STACK_SETTER,
+  isError,
 } from './commons.js';
 import { assert } from './error/assert.js';
 
@@ -174,7 +177,7 @@ export const makeHardener = () => {
       /**
        * @param {any} obj
        */
-      function freezeAndTraverse(obj) {
+      const baseFreezeAndTraverse = obj => {
         // Now freeze the object to ensure reactive
         // objects such as proxies won't add properties
         // during traversal, before they get frozen.
@@ -218,21 +221,54 @@ export const makeHardener = () => {
             enqueue(desc.set, `${pathname}(set)`);
           }
         });
-      }
+      };
+
+      const freezeAndTraverse =
+        FERAL_STACK_GETTER === undefined && FERAL_STACK_SETTER === undefined
+          ? // On platforms without v8's error own stack accessor problem,
+            // don't pay for any extra overhead.
+            baseFreezeAndTraverse
+          : obj => {
+              if (isError(obj)) {
+                // Only pay the overhead if it first passes this cheap isError
+                // check. Otherwise, it will be unrepaired, but won't be judged
+                // to be a passable error anyway, so will not be unsafe.
+                const stackDesc = getOwnPropertyDescriptor(obj, 'stack');
+                if (
+                  stackDesc &&
+                  stackDesc.get === FERAL_STACK_GETTER &&
+                  stackDesc.configurable
+                ) {
+                  // Can only repair if it is configurable. Otherwise, leave
+                  // unrepaired, in which case it will not be judged passable,
+                  // avoiding a safety problem.
+                  defineProperty(obj, 'stack', {
+                    // NOTE: Calls getter during harden, which seems dangerous.
+                    // But we're only calling the problematic getter whose
+                    // hazards we think we understand.
+                    // @ts-expect-error TS should know FERAL_STACK_GETTER
+                    // cannot be `undefined` here.
+                    // See https://github.com/endojs/endo/pull/2232#discussion_r1575179471
+                    value: apply(FERAL_STACK_GETTER, obj, []),
+                  });
+                }
+              }
+              return baseFreezeAndTraverse(obj);
+            };
 
-      function dequeue() {
+      const dequeue = () => {
         // New values added before forEach() has finished will be visited.
         setForEach(toFreeze, freezeAndTraverse);
-      }
+      };
 
       /** @param {any} value */
-      function markHardened(value) {
+      const markHardened = value => {
         weaksetAdd(hardened, value);
-      }
+      };
 
-      function commit() {
+      const commit = () => {
         setForEach(toFreeze, markHardened);
-      }
+      };
 
       enqueue(root);
       dequeue();