From 74ce9e89995fa61f5e53eec82cc124af891c8ed7 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Sat, 20 Apr 2024 17:33:05 -0700 Subject: [PATCH 1/7] 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(); From b11bebae29f97e934c27dbb1d5f99cfa5d0a7627 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Mon, 22 Apr 2024 11:42:05 -0700 Subject: [PATCH 2/7] test: switch CI from 22.x to 21.x --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7f23ed1565..ed021a3c1d 100644 --- a/.github/workflows/ci.yml +++ b/.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: From 770d56183c7e0328a492853a506d7e4d6c8e98dd Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Mon, 22 Apr 2024 12:09:06 -0700 Subject: [PATCH 3/7] fixup! adding link to review conversation --- packages/ses/src/make-hardener.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/ses/src/make-hardener.js b/packages/ses/src/make-hardener.js index ac5ba39585..9117322cb0 100644 --- a/packages/ses/src/make-hardener.js +++ b/packages/ses/src/make-hardener.js @@ -247,6 +247,7 @@ export const makeHardener = () => { // 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, []), }); } From e5f724019fea2630e2fc883a2fb5ddd1bed1e6e1 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Mon, 22 Apr 2024 12:13:01 -0700 Subject: [PATCH 4/7] fixup! review suggestions --- packages/ses/src/make-hardener.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/ses/src/make-hardener.js b/packages/ses/src/make-hardener.js index 9117322cb0..e9d255b40b 100644 --- a/packages/ses/src/make-hardener.js +++ b/packages/ses/src/make-hardener.js @@ -50,6 +50,7 @@ import { weaksetAdd, weaksetHas, FERAL_STACK_GETTER, + FERAL_STACK_SETTER, isError, } from './commons.js'; import { assert } from './error/assert.js'; @@ -223,7 +224,7 @@ export const makeHardener = () => { }; const freezeAndTraverse = - FERAL_STACK_GETTER === undefined + (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 From a3e1f0d57c31f1f49ddee7279fcc37423fb69cef Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Mon, 22 Apr 2024 12:48:44 -0700 Subject: [PATCH 5/7] fixup! format --- packages/ses/src/make-hardener.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ses/src/make-hardener.js b/packages/ses/src/make-hardener.js index e9d255b40b..626540e4c5 100644 --- a/packages/ses/src/make-hardener.js +++ b/packages/ses/src/make-hardener.js @@ -224,7 +224,7 @@ export const makeHardener = () => { }; const freezeAndTraverse = - (FERAL_STACK_GETTER === undefined && FERAL_STACK_SETTER === undefined) + 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 From 07d6d6b5eaa122117e638045b21cc490c9d872e8 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Mon, 22 Apr 2024 13:28:45 -0700 Subject: [PATCH 6/7] fixup! review suggestions --- .../SES_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR.md | 17 +++++++++++++++++ packages/ses/src/commons.js | 5 ++++- 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 packages/ses/error-codes/SES_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR.md diff --git a/packages/ses/error-codes/SES_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR.md b/packages/ses/error-codes/SES_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR.md new file mode 100644 index 0000000000..d053d322b5 --- /dev/null +++ b/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! diff --git a/packages/ses/src/commons.js b/packages/ses/src/commons.js index 2b0c346818..65893e430f 100644 --- a/packages/ses/src/commons.js +++ b/packages/ses/src/commons.js @@ -324,7 +324,10 @@ if (er1StackDesc && er2StackDesc && er1StackDesc.get) { feralStackGetter = er1StackDesc.get; feralStackSetter = er1StackDesc.set; } else { - throw TypeError('Error own stack accessor functions not as expected'); + // 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)', + ); } } From 1d5f361b629bb5b55d516e06066e7ab9c3e0df59 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Mon, 22 Apr 2024 16:09:42 -0700 Subject: [PATCH 7/7] fixup! review suggestions --- packages/ses/src/commons.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ses/src/commons.js b/packages/ses/src/commons.js index 65893e430f..57d3683753 100644 --- a/packages/ses/src/commons.js +++ b/packages/ses/src/commons.js @@ -321,8 +321,8 @@ if (er1StackDesc && er2StackDesc && er1StackDesc.get) { // 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; + 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(