From d0de868a5aa510d846e7da7f3d852d88ff0490ba Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Mon, 15 Jan 2024 21:55:05 -0800 Subject: [PATCH] fix(ses): handle properties already override protected --- packages/ses/src/enable-property-overrides.js | 72 +++++----- packages/ses/src/enablements.js | 13 ++ packages/ses/src/lockdown.js | 3 + packages/ses/src/tame-faux-data-properties.js | 131 ++++++++++++++++++ .../test/test-tame-faux-data-properties.js | 112 +++++++++++++++ 5 files changed, 295 insertions(+), 36 deletions(-) create mode 100644 packages/ses/src/tame-faux-data-properties.js create mode 100644 packages/ses/test/test-tame-faux-data-properties.js diff --git a/packages/ses/src/enable-property-overrides.js b/packages/ses/src/enable-property-overrides.js index 476a9d9b60..4f2af62c55 100644 --- a/packages/ses/src/enable-property-overrides.js +++ b/packages/ses/src/enable-property-overrides.js @@ -11,7 +11,6 @@ import { defineProperty, getOwnPropertyDescriptor, getOwnPropertyDescriptors, - getOwnPropertyNames, isObject, objectHasOwnProperty, ownKeys, @@ -88,9 +87,39 @@ export default function enablePropertyOverrides( if ('value' in desc && desc.configurable) { const { value } = desc; - function getter() { - return value; - } + const isDebug = setHas(debugProperties, prop); + + // We use concise method syntax to be `this` sensitive but still + // omit a prototype property of [[Construct]] behavior. + const { getter, setter } = { + getter() { + return value; + }, + setter(newValue) { + if (obj === this) { + throw TypeError( + `Cannot assign to read only property '${String( + prop, + )}' of '${path}'`, + ); + } + if (objectHasOwnProperty(this, prop)) { + this[prop] = newValue; + } else { + if (isDebug) { + // eslint-disable-next-line @endo/no-polymorphic-call + console.error(TypeError(`Override property ${prop}`)); + } + defineProperty(this, prop, { + value: newValue, + writable: true, + enumerable: true, + configurable: true, + }); + } + }, + }; + defineProperty(getter, 'originalValue', { value, writable: false, @@ -98,32 +127,6 @@ export default function enablePropertyOverrides( configurable: false, }); - const isDebug = setHas(debugProperties, prop); - - function setter(newValue) { - if (obj === this) { - throw TypeError( - `Cannot assign to read only property '${String( - prop, - )}' of '${path}'`, - ); - } - if (objectHasOwnProperty(this, prop)) { - this[prop] = newValue; - } else { - if (isDebug) { - // eslint-disable-next-line @endo/no-polymorphic-call - console.error(TypeError(`Override property ${prop}`)); - } - defineProperty(this, prop, { - value: newValue, - writable: true, - enumerable: true, - configurable: true, - }); - } - } - defineProperty(obj, prop, { get: getter, set: setter, @@ -148,12 +151,11 @@ export default function enablePropertyOverrides( } // TypeScript does not allow symbols to be used as indexes because it // cannot recokon types of symbolized properties. - // @ts-ignore arrayForEach(ownKeys(descs), prop => enable(path, obj, prop, descs[prop])); } function enableProperties(path, obj, plan) { - for (const prop of getOwnPropertyNames(plan)) { + for (const prop of ownKeys(plan)) { const desc = getOwnPropertyDescriptor(obj, prop); if (!desc || desc.get || desc.set) { // No not a value property, nothing to do. @@ -161,10 +163,8 @@ export default function enablePropertyOverrides( continue; } - // Plan has no symbol keys and we use getOwnPropertyNames() - // so `prop` cannot only be a string, not a symbol. We coerce it in place - // with `String(..)` anyway just as good hygiene, since these paths are just - // for diagnostic purposes. + // In case `prop` is a symbol, we first coerce it with `String`, + // purely for diagnostic purposes. const subPath = `${path}.${String(prop)}`; const subPlan = plan[prop]; diff --git a/packages/ses/src/enablements.js b/packages/ses/src/enablements.js index e45c45e1a1..222418121a 100644 --- a/packages/ses/src/enablements.js +++ b/packages/ses/src/enablements.js @@ -1,3 +1,5 @@ +import { toStringTagSymbol } from './commons.js'; + /** * @file Exports {@code enablements}, a recursively defined * JSON record defining the optimum set of intrinsics properties @@ -74,6 +76,13 @@ export const minEnablements = { '%ErrorPrototype%': { name: true, // set by "precond", "ava", "node-fetch" }, + '%IteratorPrototype%': { + toString: true, + // https://github.com/tc39/proposal-iterator-helpers + constructor: true, + // https://github.com/tc39/proposal-iterator-helpers + [toStringTagSymbol]: true, + }, }; /** @@ -152,6 +161,10 @@ export const moderateEnablements = { '%IteratorPrototype%': { toString: true, + // https://github.com/tc39/proposal-iterator-helpers + constructor: true, + // https://github.com/tc39/proposal-iterator-helpers + [toStringTagSymbol]: true, }, }; diff --git a/packages/ses/src/lockdown.js b/packages/ses/src/lockdown.js index 01babf187e..56f339664a 100644 --- a/packages/ses/src/lockdown.js +++ b/packages/ses/src/lockdown.js @@ -54,6 +54,7 @@ import { getAnonymousIntrinsics } from './get-anonymous-intrinsics.js'; import { makeCompartmentConstructor } from './compartment.js'; import { tameHarden } from './tame-harden.js'; import { tameSymbolConstructor } from './tame-symbol-constructor.js'; +import { tameFauxDataProperties } from './tame-faux-data-properties.js'; /** @typedef {import('../types.js').LockdownOptions} LockdownOptions */ @@ -335,6 +336,8 @@ export const repairIntrinsics = (options = {}) => { // Replace *Locale* methods with their non-locale equivalents tameLocaleMethods(intrinsics, localeTaming); + tameFauxDataProperties(intrinsics); + /** * 2. WHITELIST to standardize the environment. */ diff --git a/packages/ses/src/tame-faux-data-properties.js b/packages/ses/src/tame-faux-data-properties.js new file mode 100644 index 0000000000..741585f114 --- /dev/null +++ b/packages/ses/src/tame-faux-data-properties.js @@ -0,0 +1,131 @@ +import { + getOwnPropertyDescriptor, + apply, + defineProperty, + toStringTagSymbol, +} from './commons.js'; + +const throws = thunk => { + try { + thunk(); + } catch (er) { + return true; + } + return false; +}; + +/** + * Exported for convenience of unit testing. Harmless, but not expected + * to be useful by itself. + * + * @param {any} obj + * @param {string|symbol} prop + * @param {any} expectedValue + * @returns {boolean} + */ +export const tameFauxDataProperty = (obj, prop, expectedValue) => { + if (obj === undefined) { + return false; + } + const desc = getOwnPropertyDescriptor(obj, prop); + if (!desc || 'value' in desc) { + return false; + } + const { get, set } = desc; + if (typeof get !== 'function') { + return false; + } + if (typeof set !== 'function') { + return false; + } + const observedValue = get(); + if (observedValue !== expectedValue) { + return false; + } + if (apply(get, obj, []) !== expectedValue) { + return false; + } + const testValue = 'Seems to be a setter'; + const subject1 = { __proto__: null }; + apply(set, subject1, [testValue]); + if (subject1[prop] !== testValue) { + return false; + } + const subject2 = { __proto__: obj }; + apply(set, subject2, [testValue]); + if (subject2[prop] !== testValue) { + return false; + } + if (!throws(() => apply(set, obj, [testValue]))) { + return false; + } + if ('originalValue' in get) { + return false; + } + + // We assume that this code runs before any untrusted code runs, so + // we do not need to worry about the above conditions passing because of + // malicious intent. In fact, it runs even before vetted shims are supposed + // to run, between repair and hardening. Given that, after all these tests + // pass, we have adequately validated that the property in question is + // an accessor function whose purpose is suppressing the override mistake, + // i.e., enabling a non-writable property to be overridden by assignment. + // In that case, here we *temporarily* turn it into the data property + // it seems to emulate, but writable so that it does not trigger the + // override mistake while in this temporary state. + + // For those properties that are also listed in `enablements.js`, + // that phase will re-enable override for these properties, but + // via accessor functions that ses controls, so we know what they are + // doing. In addition, the getter functions installed by + // `enable-property-overrides.js` have an `originalValue` field + // enabling meta-traversal code like harden to visit the original value + // without calling the getter. + + if (desc.configurable === false) { + // Even though it seems to be an accessor, we're unable to fix it. + return false; + } + + // Many of the `return false;` cases above plausibly should be turned into + // errors, or an least generate warnings. However, for those, the checks + // following this phase are likely to signal an error anyway. + + defineProperty(obj, prop, { + value: expectedValue, + writable: true, + enumerable: desc.enumerable, + configurable: true, + }); + + return true; +}; + +/** + * For each, see it the property is an accessor property that + * seems to emulate a data property with this expectedValue, + * and seems to be an accessor whose purpose is to protect against + * the override mistake, i.e., enable these properties to be overridden + * by assignment. If all these expected conditions are met, then + * *temporarily* turn it into the data property it emulated. + * + * Coordinate with `enablements.js` so the appropriate ones are + * turned back to accessor that protect against override mistake, + * but accessors we know. + * + * @param {Record} intrinsics + */ +export const tameFauxDataProperties = intrinsics => { + // https://github.com/tc39/proposal-iterator-helpers + tameFauxDataProperty( + intrinsics['%IteratorPrototype%'], + 'constructor', + intrinsics.Iterator, + ); + // https://github.com/tc39/proposal-iterator-helpers + tameFauxDataProperty( + intrinsics['%IteratorPrototype%'], + toStringTagSymbol, + 'Iterator', + ); +}; diff --git a/packages/ses/test/test-tame-faux-data-properties.js b/packages/ses/test/test-tame-faux-data-properties.js new file mode 100644 index 0000000000..5d53cc36d0 --- /dev/null +++ b/packages/ses/test/test-tame-faux-data-properties.js @@ -0,0 +1,112 @@ +import test from 'ava'; +import '../index.js'; + +import { tameFauxDataProperty as tfdp } from '../src/tame-faux-data-properties.js'; + +const { freeze, defineProperty, getOwnPropertyDescriptor } = Object; + +test('unit test tameFauxDataProperty', t => { + t.is(tfdp(undefined, 'foo', 'bar'), false); + t.is(tfdp({}, 'foo', 'bar'), false); + t.is(tfdp({ foo: 'bar' }, 'foo', 'bar'), false); + + t.is( + tfdp( + { + get foo() { + return 'bar'; + }, + }, + 'foo', + 'bar', + ), + false, + ); + + t.is( + tfdp({ + get foo() { + return 'bar'; + }, + set foo(_newValue) { + throw TypeError('always throws'); + }, + }), + false, + ); + + t.is( + tfdp({ + get foo() { + return 'bar'; + }, + set foo(_newValue) { + // never throws + }, + }), + false, + ); + + const subject1 = { + get foo() { + return 'bar'; + }, + set foo(_newValue) { + if (this === subject1) { + throw TypeError('throws only when it should'); + } + }, + }; + t.is(tfdp(subject1, 'foo', 'bar'), false); + + const subject2 = { + get foo() { + return 'bar'; + }, + set foo(newValue) { + defineProperty(this, 'foo', { value: newValue }); + }, + }; + t.is(tfdp(subject2, 'foo', 'bar'), false); + + const subject3 = { + get foo() { + return 'bar'; + }, + set foo(newValue) { + if (this === subject3) { + throw TypeError('throws only when it should'); + } + defineProperty(this, 'foo', { value: newValue }); + }, + }; + t.is(tfdp(freeze(subject3), 'foo', 'bar'), false); + + const subject4 = { + get foo() { + return 'zip'; + }, + set foo(newValue) { + if (this === subject4) { + throw TypeError('throws only when it should'); + } + defineProperty(this, 'foo', { value: newValue }); + }, + }; + t.is(tfdp(subject4, 'foo', 'bar'), false); + + const desc4 = getOwnPropertyDescriptor(subject4, 'foo'); + t.deepEqual(desc4, { + get: desc4.get, + set: desc4.set, + enumerable: true, + configurable: true, + }); + t.is(tfdp(subject4, 'foo', 'zip'), true); + t.deepEqual(getOwnPropertyDescriptor(subject4, 'foo'), { + value: 'zip', + writable: true, + enumerable: true, + configurable: true, + }); +});