From d13bf9c6b083f04fa7f5983fb4f4a07cd263abe9 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Wed, 13 Nov 2024 11:55:52 -0800 Subject: [PATCH] fix(ses): fix #2598 with cauterizeProperty reuse (#2624) Closes: #2598 Refs: /~https://github.com/endojs/endo/pull/2563 /~https://github.com/endojs/endo/pull/2334 /~https://github.com/endojs/endo/pull/1221 ## Description #1221 was supposed to make ses tolerate undeletable `func.prototype` properties that should be absent, so long as they could be set to `undefined` instead, making them harmless. This tolerance came with a warning to flag the remaining non-conformance. However #2598 explains why #1221 sometimes fails to do this. #1221 did come with a test, but it fell into the case where #1221 works, which is a non-toplevel function. #2563 (and #2334 ?) fell into the trap explained by #2598 and untested by #1221, which is an undeletable `func.prototype` on a top-level instrinsic. As a result, #2563 currently contains a workaround for #2598 which this PR would make unnecessary. This PR fixes the problem by factoring out the `func.prototype`-tolerant property deletion into a separate `cauterizeProperty` function which it calls from both places. This PR also adds the test that was missing from #1221 , having first checked that the test detects #2598 when run without the rest of this PR. If this PR gets merged before #2563, then #2563's workaround for #2598 can first be removed before it is merged. - [ ] TODO should pass a genuine reporter in to all calls to `cauterizeProperty`. @kriskowal , please advise how intrinsics.js should arrange to do so. ### Security Considerations Allowing a `func.prototype` property that really shouldn't be there seems safe, so long as it is safely set to `undefined` first, which this PR does, and then checks that it has done so. ### Scaling Considerations none ### Documentation Considerations generally, this would be one less thing to worry about, and thus one less thing that needs to be documented for most users. ### Testing Considerations Adds the test that was missing from #1221 that let #2598 go unnoticed until #2563 ### Compatibility Considerations Should be none. ### Upgrade Considerations Should be none. --- packages/ses/src/cauterize-property.js | 69 +++++++++++++++++++ packages/ses/src/compartment-shim.js | 8 ++- packages/ses/src/intrinsics.js | 26 +++++-- packages/ses/src/lockdown.js | 2 +- packages/ses/src/permits-intrinsics.js | 33 +-------- .../tolerate-empty-prototype-toplevel.test.js | 24 +++++++ .../ses/test/tolerate-empty-prototype.test.js | 3 + 7 files changed, 129 insertions(+), 36 deletions(-) create mode 100644 packages/ses/src/cauterize-property.js create mode 100644 packages/ses/test/tolerate-empty-prototype-toplevel.test.js diff --git a/packages/ses/src/cauterize-property.js b/packages/ses/src/cauterize-property.js new file mode 100644 index 0000000000..b7bd4929a4 --- /dev/null +++ b/packages/ses/src/cauterize-property.js @@ -0,0 +1,69 @@ +import { objectHasOwnProperty } from './commons.js'; + +/** + * @import {Reporter} from './reporting-types.js' + */ + +/** + * Delete `obj[prop]` or at least make it harmless. + * + * If the property was not expected, then emit a reporter-dependent warning + * to bring attention to this case, so someone can determine what to do with it. + * + * If the property to be deleted is a function's `.prototype` property, this + * will normally be because the function was supposed to be a + * - builtin method or non-constructor function + * - arrow function + * - concise method + * + * all of whom are not supposed to have a `.prototype` property. Nevertheless, + * on some platforms (like older versions of Hermes), or as a result of + * some shim-based mods to the primordials (like core-js?), some of these + * functions may accidentally be more like `function` functions with + * an undeletable `.prototype` property. In these cases, if we can + * set the value of that bogus `.prototype` property to `undefined`, + * we do so, issuing a warning, rather than failing to initialize ses. + * + * @param {object} obj + * @param {PropertyKey} prop + * @param {boolean} known If deletion is expected, don't warn + * @param {string} subPath Used for warning messages + * @param {Reporter} reporter Where to issue warning or error. + * @returns {void} + */ +export const cauterizeProperty = ( + obj, + prop, + known, + subPath, + { warn, error }, +) => { + // Either the object lacks a permit or the object doesn't match the + // permit. + // If the permit is specifically false, not merely undefined, + // this is a property we expect to see because we know it exists in + // some environments and we have expressly decided to exclude it. + // Any other disallowed property is one we have not audited and we log + // that we are removing it so we know to look into it, as happens when + // the language evolves new features to existing intrinsics. + if (!known) { + warn(`Removing ${subPath}`); + } + try { + delete obj[prop]; + } catch (err) { + if (objectHasOwnProperty(obj, prop)) { + if (typeof obj === 'function' && prop === 'prototype') { + obj.prototype = undefined; + if (obj.prototype === undefined) { + warn(`Tolerating undeletable ${subPath} === undefined`); + return; + } + } + error(`failed to delete ${subPath}`, err); + } else { + error(`deleting ${subPath} threw`, err); + } + throw err; + } +}; diff --git a/packages/ses/src/compartment-shim.js b/packages/ses/src/compartment-shim.js index 8ad47f5046..ee3ddface4 100644 --- a/packages/ses/src/compartment-shim.js +++ b/packages/ses/src/compartment-shim.js @@ -2,12 +2,18 @@ import { globalThis } from './commons.js'; import { makeCompartmentConstructor } from './compartment.js'; import { tameFunctionToString } from './tame-function-tostring.js'; import { getGlobalIntrinsics } from './intrinsics.js'; +import { chooseReporter } from './reporting.js'; const markVirtualizedNativeFunction = tameFunctionToString(); +const muteReporter = chooseReporter('none'); + // @ts-ignore Compartment is definitely on globalThis. globalThis.Compartment = makeCompartmentConstructor( makeCompartmentConstructor, - getGlobalIntrinsics(globalThis), + // Any reporting that would need to be done should have already been done + // during `lockdown()`. + // See /~https://github.com/endojs/endo/pull/2624#discussion_r1840979770 + getGlobalIntrinsics(globalThis, muteReporter), markVirtualizedNativeFunction, ); diff --git a/packages/ses/src/intrinsics.js b/packages/ses/src/intrinsics.js index 3393d64880..6dd17b58ee 100644 --- a/packages/ses/src/intrinsics.js +++ b/packages/ses/src/intrinsics.js @@ -1,3 +1,4 @@ +import { cauterizeProperty } from './cauterize-property.js'; import { TypeError, WeakSet, @@ -23,6 +24,10 @@ import { permitted, } from './permits.js'; +/** + * @import {Reporter} from './reporting-types.js' + */ + const isFunction = obj => typeof obj === 'function'; // Like defineProperty, but throws if it would modify an existing property. @@ -71,7 +76,10 @@ function sampleGlobals(globalObject, newPropertyNames) { return newIntrinsics; } -export const makeIntrinsicsCollector = () => { +/** + * @param {Reporter} reporter + */ +export const makeIntrinsicsCollector = reporter => { /** @type {Record} */ const intrinsics = create(null); let pseudoNatives; @@ -100,7 +108,15 @@ export const makeIntrinsicsCollector = () => { } const namePrototype = permit.prototype; if (!namePrototype) { - throw TypeError(`${name}.prototype property not permitted`); + cauterizeProperty( + intrinsic, + 'prototype', + false, + `${name}.prototype`, + reporter, + ); + // eslint-disable-next-line no-continue + continue; } if ( typeof namePrototype !== 'string' || @@ -164,9 +180,11 @@ export const makeIntrinsicsCollector = () => { * *original* unsafe (feral, untamed) bindings of these global variables. * * @param {object} globalObject + * @param {Reporter} reporter */ -export const getGlobalIntrinsics = globalObject => { - const { addIntrinsics, finalIntrinsics } = makeIntrinsicsCollector(); +export const getGlobalIntrinsics = (globalObject, reporter) => { + // TODO pass a proper reporter to `makeIntrinsicsCollector` + const { addIntrinsics, finalIntrinsics } = makeIntrinsicsCollector(reporter); addIntrinsics(sampleGlobals(globalObject, sharedGlobalPropertyNames)); diff --git a/packages/ses/src/lockdown.js b/packages/ses/src/lockdown.js index 4150b3d544..97fdd84f95 100644 --- a/packages/ses/src/lockdown.js +++ b/packages/ses/src/lockdown.js @@ -281,7 +281,7 @@ export const repairIntrinsics = (options = {}) => { const markVirtualizedNativeFunction = tameFunctionToString(); const { addIntrinsics, completePrototypes, finalIntrinsics } = - makeIntrinsicsCollector(); + makeIntrinsicsCollector(reporter); // @ts-expect-error __hardenTaming__ could be any string const tamedHarden = tameHarden(safeHarden, __hardenTaming__); diff --git a/packages/ses/src/permits-intrinsics.js b/packages/ses/src/permits-intrinsics.js index a629dae8ea..fb0090476a 100644 --- a/packages/ses/src/permits-intrinsics.js +++ b/packages/ses/src/permits-intrinsics.js @@ -61,6 +61,7 @@ import { ownKeys, symbolKeyFor, } from './commons.js'; +import { cauterizeProperty } from './cauterize-property.js'; /** * @import {Reporter} from './reporting-types.js' @@ -77,7 +78,7 @@ import { export default function removeUnpermittedIntrinsics( intrinsics, markVirtualizedNativeFunction, - { warn, error }, + reporter, ) { // These primitives are allowed for permits. const primitives = ['undefined', 'boolean', 'number', 'string', 'symbol']; @@ -279,35 +280,7 @@ export default function removeUnpermittedIntrinsics( const subPermit = getSubPermit(obj, permit, propString); if (!subPermit || !isAllowedProperty(subPath, obj, prop, subPermit)) { - // Either the object lacks a permit or the object doesn't match the - // permit. - // If the permit is specifically false, not merely undefined, - // this is a property we expect to see because we know it exists in - // some environments and we have expressly decided to exclude it. - // Any other disallowed property is one we have not audited and we log - // that we are removing it so we know to look into it, as happens when - // the language evolves new features to existing intrinsics. - if (subPermit !== false) { - warn(`Removing ${subPath}`); - } - try { - delete obj[prop]; - } catch (err) { - if (prop in obj) { - if (typeof obj === 'function' && prop === 'prototype') { - obj.prototype = undefined; - if (obj.prototype === undefined) { - warn(`Tolerating undeletable ${subPath} === undefined`); - // eslint-disable-next-line no-continue - continue; - } - } - error(`failed to delete ${subPath}`, err); - } else { - error(`deleting ${subPath} threw`, err); - } - throw err; - } + cauterizeProperty(obj, prop, subPermit === false, subPath, reporter); } } } diff --git a/packages/ses/test/tolerate-empty-prototype-toplevel.test.js b/packages/ses/test/tolerate-empty-prototype-toplevel.test.js new file mode 100644 index 0000000000..34582a3777 --- /dev/null +++ b/packages/ses/test/tolerate-empty-prototype-toplevel.test.js @@ -0,0 +1,24 @@ +/* global globalThis */ +import test from 'ava'; +import '../index.js'; + +// See /~https://github.com/zloirock/core-js/issues/1092 +// See /~https://github.com/endojs/endo/issues/2598 +const originalEscape = globalThis.escape; +globalThis.escape = function escape(...args) { + return Reflect.apply(originalEscape, this, args); +}; + +lockdown(); + +test('tolerate empty escape.prototype', t => { + t.is(globalThis.escape, escape); + t.assert('prototype' in escape); + t.is(escape.prototype, undefined); + t.deepEqual(Object.getOwnPropertyDescriptor(escape, 'prototype'), { + value: undefined, + writable: !!harden.isFake, + enumerable: false, + configurable: false, + }); +}); diff --git a/packages/ses/test/tolerate-empty-prototype.test.js b/packages/ses/test/tolerate-empty-prototype.test.js index 5ad78e018a..055f60340b 100644 --- a/packages/ses/test/tolerate-empty-prototype.test.js +++ b/packages/ses/test/tolerate-empty-prototype.test.js @@ -2,6 +2,9 @@ import test from 'ava'; import '../index.js'; // See /~https://github.com/zloirock/core-js/issues/1092 +// Does not detect /~https://github.com/endojs/endo/issues/2598 because +// `push` is not toplevel. +// See tolerate-empty-prototype-toplevel.test.js const originalPush = Array.prototype.push; // eslint-disable-next-line no-extend-native Array.prototype.push = function push(...args) {