From 661f3918c874c5a5d6182cd71b33b239d1c290a8 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Sat, 20 Apr 2024 17:33:05 -0700 Subject: [PATCH] fix(ses): harden hacks v8 stack own accessor problem --- packages/pass-style/test/test-passStyleOf.js | 3 +- packages/ses/src/commons.js | 55 ++++++++++++++++++++ packages/ses/src/make-hardener.js | 50 +++++++++++++++--- 3 files changed, 98 insertions(+), 10 deletions(-) diff --git a/packages/pass-style/test/test-passStyleOf.js b/packages/pass-style/test/test-passStyleOf.js index baaa48dd2b..1588d46297 100644 --- a/packages/pass-style/test/test-passStyleOf.js +++ b/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 => { diff --git a/packages/ses/src/commons.js b/packages/ses/src/commons.js index 427330f8ea..2b0c346818 100644 --- a/packages/ses/src/commons.js +++ b/packages/ses/src/commons.js @@ -294,3 +294,58 @@ 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 = er1StackDesc.get; + feralStackSetter = er1StackDesc.set; + } else { + throw TypeError('Error own stack accessor functions not as expected'); + } +} + +/** + * 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; diff --git a/packages/ses/src/make-hardener.js b/packages/ses/src/make-hardener.js index 881404190d..ac5ba39585 100644 --- a/packages/ses/src/make-hardener.js +++ b/packages/ses/src/make-hardener.js @@ -49,6 +49,8 @@ import { weakmapSet, weaksetAdd, weaksetHas, + FERAL_STACK_GETTER, + isError, } from './commons.js'; import { assert } from './error/assert.js'; @@ -174,7 +176,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 +220,53 @@ export const makeHardener = () => { enqueue(desc.set, `${pathname}(set)`); } }); - } + }; + + const freezeAndTraverse = + FERAL_STACK_GETTER === 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. + 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();