From 82e1233c2892c58a8d2dce3e531c168254781ff0 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Tue, 31 Dec 2024 13:28:17 -0800 Subject: [PATCH] feat(ses,pass-style): use non-trapping integrity level for safety --- packages/captp/src/captp.js | 6 ++-- packages/marshal/src/encodeToCapData.js | 7 ++-- packages/pass-style/src/passStyle-helpers.js | 7 +++- packages/pass-style/src/passStyleOf.js | 35 ++++++++++++++------ packages/pass-style/src/remotable.js | 5 +-- packages/pass-style/src/safe-promise.js | 11 ++++-- packages/pass-style/test/passStyleOf.test.js | 6 ++-- packages/ses/package.json | 3 +- packages/ses/src/commons.js | 10 ++++++ packages/ses/src/error/assert.js | 1 - packages/ses/src/make-hardener.js | 20 ++++++++--- packages/ses/src/permits.js | 6 ++++ yarn.lock | 3 +- 13 files changed, 87 insertions(+), 33 deletions(-) diff --git a/packages/captp/src/captp.js b/packages/captp/src/captp.js index abdfdd4966..55b966291f 100644 --- a/packages/captp/src/captp.js +++ b/packages/captp/src/captp.js @@ -47,7 +47,7 @@ const reverseSlot = slot => { }; /** - * @typedef {object} CapTPImportExportTables + * @typedef {object} CapTPImportExportTables * @property {(value: any) => CapTPSlot} makeSlotForValue * @property {(slot: CapTPSlot, iface: string | undefined) => any} makeValueForSlot * @property {(slot: CapTPSlot) => boolean} hasImport @@ -58,12 +58,12 @@ const reverseSlot = slot => { * @property {(slot: CapTPSlot, value: any) => void} markAsExported * @property {(slot: CapTPSlot) => void} deleteExport * @property {() => void} didDisconnect - + * * @typedef {object} MakeCapTPImportExportTablesOptions * @property {boolean} gcImports * @property {(slot: CapTPSlot) => void} releaseSlot * @property {(slot: CapTPSlot) => RemoteKit} makeRemoteKit - + * * @param {MakeCapTPImportExportTablesOptions} options * @returns {CapTPImportExportTables} */ diff --git a/packages/marshal/src/encodeToCapData.js b/packages/marshal/src/encodeToCapData.js index 052b5795a1..fda5e06dc7 100644 --- a/packages/marshal/src/encodeToCapData.js +++ b/packages/marshal/src/encodeToCapData.js @@ -30,7 +30,8 @@ const { is, entries, fromEntries, - freeze, + // @ts-expect-error TS doesn't see this on ObjectConstructor + suppressTrapping, } = Object; /** @@ -176,10 +177,10 @@ export const makeEncodeToCapData = (encodeOptions = {}) => { // We harden the entire capData encoding before we return it. // `encodeToCapData` requires that its input be Passable, and // therefore hardened. - // The `freeze` here is needed anyway, because the `rest` is + // The `suppressTrapping` here is needed anyway, because the `rest` is // freshly constructed by the `...` above, and we're using it // as imput in another call to `encodeToCapData`. - result.rest = encodeToCapDataRecur(freeze(rest)); + result.rest = encodeToCapDataRecur(suppressTrapping(rest)); } return result; } diff --git a/packages/pass-style/src/passStyle-helpers.js b/packages/pass-style/src/passStyle-helpers.js index 7107e11b89..ff698a0086 100644 --- a/packages/pass-style/src/passStyle-helpers.js +++ b/packages/pass-style/src/passStyle-helpers.js @@ -11,8 +11,10 @@ const { getOwnPropertyDescriptor, getPrototypeOf, hasOwnProperty: objectHasOwnProperty, - isFrozen, prototype: objectPrototype, + isFrozen, + // @ts-expect-error TS does not yet have `isNonTrapping` on ObjectConstructor + isNonTrapping, } = Object; const { apply } = Reflect; const { toStringTag: toStringTagSymbol } = Symbol; @@ -167,6 +169,9 @@ const makeCheckTagRecord = checkProto => { CX(check)`A non-object cannot be a tagRecord: ${tagRecord}`)) && (isFrozen(tagRecord) || (!!check && CX(check)`A tagRecord must be frozen: ${tagRecord}`)) && + (isNonTrapping(tagRecord) || + (!!check && + CX(check)`A tagRecord must be non-trapping: ${tagRecord}`)) && (!isArray(tagRecord) || (!!check && CX(check)`An array cannot be a tagRecord: ${tagRecord}`)) && checkPassStyle( diff --git a/packages/pass-style/src/passStyleOf.js b/packages/pass-style/src/passStyleOf.js index 7c4dd78b2e..2c7cdeb820 100644 --- a/packages/pass-style/src/passStyleOf.js +++ b/packages/pass-style/src/passStyleOf.js @@ -31,7 +31,13 @@ import { assertPassableString } from './string.js'; /** @typedef {Exclude} HelperPassStyle */ const { ownKeys } = Reflect; -const { isFrozen, getOwnPropertyDescriptors, values } = Object; +const { + getOwnPropertyDescriptors, + values, + isFrozen, + // @ts-expect-error TS does not yet have `isNonTrapping` on ObjectConstructor + isNonTrapping, +} = Object; /** * @param {PassStyleHelper[]} passStyleHelpers @@ -143,14 +149,17 @@ const makePassStyleOf = passStyleHelpers => { if (inner === null) { return 'null'; } - if (!isFrozen(inner)) { - assert.fail( - // TypedArrays get special treatment in harden() - // and a corresponding special error message here. - isTypedArray(inner) - ? X`Cannot pass mutable typed arrays like ${inner}.` - : X`Cannot pass non-frozen objects like ${inner}. Use harden()`, - ); + if (!isNonTrapping(inner)) { + if (!isFrozen(inner)) { + throw assert.fail( + // TypedArrays get special treatment in harden() + // and a corresponding special error message here. + isTypedArray(inner) + ? X`Cannot pass mutable typed arrays like ${inner}.` + : X`Cannot pass non-frozen objects like ${inner}. Use harden()`, + ); + } + throw Fail`Cannot pass non-trapping objects like ${inner}`; } if (isPromise(inner)) { assertSafePromise(inner); @@ -177,8 +186,12 @@ const makePassStyleOf = passStyleHelpers => { return 'remotable'; } case 'function': { - isFrozen(inner) || - Fail`Cannot pass non-frozen objects like ${inner}. Use harden()`; + if (!isNonTrapping(inner)) { + if (!isFrozen(inner)) { + throw Fail`Cannot pass non-frozen objects like ${inner}. Use harden()`; + } + throw Fail`Cannot pass trapping objects like ${inner}. Use harden()`; + } typeof inner.then !== 'function' || Fail`Cannot pass non-promise thenables`; remotableHelper.assertValid(inner, passStyleOfRecur); diff --git a/packages/pass-style/src/remotable.js b/packages/pass-style/src/remotable.js index af681c2335..30440efdaf 100644 --- a/packages/pass-style/src/remotable.js +++ b/packages/pass-style/src/remotable.js @@ -24,9 +24,10 @@ const { ownKeys } = Reflect; const { isArray } = Array; const { getPrototypeOf, - isFrozen, prototype: objectPrototype, getOwnPropertyDescriptors, + // @ts-expect-error TS does not yet have `isNonTrapping` on ObjectConstructor + isNonTrapping, } = Object; /** @@ -154,7 +155,7 @@ const checkRemotable = (val, check) => { if (confirmedRemotables.has(val)) { return true; } - if (!isFrozen(val)) { + if (!isNonTrapping(val)) { return ( !!check && CX(check)`cannot serialize non-frozen objects like ${val}` ); diff --git a/packages/pass-style/src/safe-promise.js b/packages/pass-style/src/safe-promise.js index 407e2aab6a..b474ee54d2 100644 --- a/packages/pass-style/src/safe-promise.js +++ b/packages/pass-style/src/safe-promise.js @@ -6,7 +6,12 @@ import { assertChecker, hasOwnPropertyOf, CX } from './passStyle-helpers.js'; /** @import {Checker} from './types.js' */ -const { isFrozen, getPrototypeOf, getOwnPropertyDescriptor } = Object; +const { + getPrototypeOf, + getOwnPropertyDescriptor, + // @ts-expect-error TS does not yet have `isNonTrapping` on ObjectConstructor + isNonTrapping, +} = Object; const { ownKeys } = Reflect; const { toStringTag } = Symbol; @@ -88,7 +93,7 @@ const checkPromiseOwnKeys = (pr, check) => { if ( typeof val === 'object' && val !== null && - isFrozen(val) && + isNonTrapping(val) && getPrototypeOf(val) === Object.prototype ) { const subKeys = ownKeys(val); @@ -132,7 +137,7 @@ const checkPromiseOwnKeys = (pr, check) => { */ const checkSafePromise = (pr, check) => { return ( - (isFrozen(pr) || CX(check)`${pr} - Must be frozen`) && + (isNonTrapping(pr) || CX(check)`${pr} - Must be frozen`) && (isPromise(pr) || CX(check)`${pr} - Must be a promise`) && (getPrototypeOf(pr) === Promise.prototype || CX(check)`${pr} - Must inherit from Promise.prototype: ${q( diff --git a/packages/pass-style/test/passStyleOf.test.js b/packages/pass-style/test/passStyleOf.test.js index f13d3f1fd7..b94361aebd 100644 --- a/packages/pass-style/test/passStyleOf.test.js +++ b/packages/pass-style/test/passStyleOf.test.js @@ -13,7 +13,7 @@ const harden = /** @type {import('ses').Harden & { isFake?: boolean }} */ ( global.harden ); -const { getPrototypeOf, defineProperty, freeze } = Object; +const { getPrototypeOf, defineProperty, freeze, suppressTrapping } = Object; const { ownKeys } = Reflect; test('passStyleOf basic success cases', t => { @@ -208,7 +208,7 @@ test('passStyleOf testing remotables', t => { * * @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ - const farObj2 = freeze({ __proto__: tagRecord2 }); + const farObj2 = suppressTrapping({ __proto__: tagRecord2 }); if (harden.isFake) { t.is(passStyleOf(farObj2), 'remotable'); } else { @@ -386,7 +386,7 @@ test('remotables - safety from the gibson042 attack', t => { * explicitly make this non-trapping, which we cannot yet express. * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md */ - const makeInput = () => freeze({ __proto__: mercurialProto }); + const makeInput = () => suppressTrapping({ __proto__: mercurialProto }); const input1 = makeInput(); const input2 = makeInput(); diff --git a/packages/ses/package.json b/packages/ses/package.json index e552bda739..1ddcd66b57 100644 --- a/packages/ses/package.json +++ b/packages/ses/package.json @@ -85,7 +85,8 @@ "postpack": "git clean -f '*.d.ts*' '*.tsbuildinfo'" }, "dependencies": { - "@endo/env-options": "workspace:^" + "@endo/env-options": "workspace:^", + "@endo/non-trapping-shim": "^0.1.0" }, "devDependencies": { "@endo/compartment-mapper": "workspace:^", diff --git a/packages/ses/src/commons.js b/packages/ses/src/commons.js index 5211fa075d..d0e580be58 100644 --- a/packages/ses/src/commons.js +++ b/packages/ses/src/commons.js @@ -14,6 +14,8 @@ /* global globalThis */ /* eslint-disable no-restricted-globals */ +import '@endo/non-trapping-shim/shim.js'; + // We cannot use globalThis as the local name since it would capture the // lexical name. const universalThis = globalThis; @@ -75,6 +77,11 @@ export const { setPrototypeOf, values, fromEntries, + // https://github.com/endojs/endo/pull/2673 + // @ts-expect-error TS does not yet have this on ObjectConstructor. + isNonTrapping, + // @ts-expect-error TS does not yet have this on ObjectConstructor. + suppressTrapping, } = Object; export const { @@ -125,6 +132,9 @@ export const { ownKeys, preventExtensions: reflectPreventExtensions, set: reflectSet, + // https://github.com/endojs/endo/pull/2673 + isNonTrapping: reflectIsNonTrapping, + suppressTrapping: reflectSuppressTrapping, } = Reflect; export const { isArray, prototype: arrayPrototype } = Array; diff --git a/packages/ses/src/error/assert.js b/packages/ses/src/error/assert.js index f4a645c5b0..d5b9d222cc 100644 --- a/packages/ses/src/error/assert.js +++ b/packages/ses/src/error/assert.js @@ -305,7 +305,6 @@ export const sanitizeError = error => { ); } for (const name of ownKeys(error)) { - // @ts-expect-error TS still confused by symbols as property names const desc = descs[name]; if (desc && objectHasOwnProperty(desc, 'get')) { defineProperty(error, name, { diff --git a/packages/ses/src/make-hardener.js b/packages/ses/src/make-hardener.js index d377fd8793..749367a140 100644 --- a/packages/ses/src/make-hardener.js +++ b/packages/ses/src/make-hardener.js @@ -30,7 +30,6 @@ import { apply, arrayForEach, defineProperty, - freeze, getOwnPropertyDescriptor, getOwnPropertyDescriptors, getPrototypeOf, @@ -49,6 +48,8 @@ import { FERAL_STACK_GETTER, FERAL_STACK_SETTER, isError, + isFrozen, + suppressTrapping, } from './commons.js'; import { assert } from './error/assert.js'; @@ -128,6 +129,10 @@ const freezeTypedArray = array => { * @returns {Harden} */ export const makeHardener = () => { + // TODO Get the native hardener to suppressTrapping at each step, + // rather than freeze. Until then, we cannot use it, which is *expensive*! + // TODO Comment out the following to skip the native hardener. + // // Use a native hardener if possible. if (typeof globalThis.harden === 'function') { const safeHarden = globalThis.harden; @@ -182,8 +187,17 @@ export const makeHardener = () => { // Also throws if the object is an ArrayBuffer or any TypedArray. if (isTypedArray(obj)) { freezeTypedArray(obj); + if (isFrozen(obj)) { + // After `freezeTypedArray`, the typed array might actually be + // frozen if + // - it has no indexed properties + // - it is backed by an Immutable ArrayBuffer as proposed. + // In either case, this makes it a candidate to be made + // non-trapping. + suppressTrapping(obj); + } } else { - freeze(obj); + suppressTrapping(obj); } // we rely upon certain commitments of Object.freeze and proxies here @@ -238,8 +252,6 @@ export const makeHardener = () => { // 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, []), }); diff --git a/packages/ses/src/permits.js b/packages/ses/src/permits.js index 283d861c34..5bbdd646ab 100644 --- a/packages/ses/src/permits.js +++ b/packages/ses/src/permits.js @@ -488,6 +488,9 @@ export const permitted = { groupBy: fn, // Seen on QuickJS __getClass: false, + // https://github.com/endojs/endo/pull/2673 + isNonTrapping: fn, + suppressTrapping: fn, }, '%ObjectPrototype%': { @@ -1624,6 +1627,9 @@ export const permitted = { set: fn, setPrototypeOf: fn, '@@toStringTag': 'string', + // https://github.com/endojs/endo/pull/2673 + isNonTrapping: fn, + suppressTrapping: fn, }, Proxy: { diff --git a/yarn.lock b/yarn.lock index 98e3a54cd0..4b305d03e2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -702,7 +702,7 @@ __metadata: languageName: unknown linkType: soft -"@endo/non-trapping-shim@workspace:packages/non-trapping-shim": +"@endo/non-trapping-shim@npm:^0.1.0, @endo/non-trapping-shim@workspace:packages/non-trapping-shim": version: 0.0.0-use.local resolution: "@endo/non-trapping-shim@workspace:packages/non-trapping-shim" dependencies: @@ -8960,6 +8960,7 @@ __metadata: "@endo/compartment-mapper": "workspace:^" "@endo/env-options": "workspace:^" "@endo/module-source": "workspace:^" + "@endo/non-trapping-shim": "npm:^0.1.0" "@endo/test262-runner": "workspace:^" ava: "npm:^6.1.3" babel-eslint: "npm:^10.1.0"