From 0c137333060cab9e43fafed913fdd3b63aa47bd8 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Thu, 2 Jan 2025 17:40:48 -0800 Subject: [PATCH] feat(ses,pass-style): use non-trapping integrity trait ponyfill for safety --- packages/captp/src/captp.js | 6 ++-- packages/marshal/package.json | 1 + packages/marshal/src/encodeToCapData.js | 9 +++--- packages/pass-style/package.json | 1 + packages/pass-style/src/passStyle-helpers.js | 7 ++++- packages/pass-style/src/passStyleOf.js | 33 +++++++++++++------- packages/pass-style/src/remotable.js | 5 +-- packages/pass-style/src/safe-promise.js | 8 +++-- packages/pass-style/test/passStyleOf.test.js | 8 +++-- packages/ses/package.json | 3 +- packages/ses/src/make-hardener.js | 20 ++++++++++-- yarn.lock | 5 ++- 12 files changed, 74 insertions(+), 32 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/package.json b/packages/marshal/package.json index 8dc8b5bd1a..25cce0f142 100644 --- a/packages/marshal/package.json +++ b/packages/marshal/package.json @@ -45,6 +45,7 @@ "@endo/errors": "workspace:^", "@endo/eventual-send": "workspace:^", "@endo/nat": "workspace:^", + "@endo/non-trapping-shim": "^0.1.0", "@endo/pass-style": "workspace:^", "@endo/promise-kit": "workspace:^" }, diff --git a/packages/marshal/src/encodeToCapData.js b/packages/marshal/src/encodeToCapData.js index 052b5795a1..1af1cd1393 100644 --- a/packages/marshal/src/encodeToCapData.js +++ b/packages/marshal/src/encodeToCapData.js @@ -6,6 +6,8 @@ // encodes to CapData, a JSON-representable data structure, and leaves it to // the caller (`marshal.js`) to stringify it. +import { X, Fail, q } from '@endo/errors'; +import { extraObjectMethods } from '@endo/non-trapping-shim'; import { passStyleOf, isErrorLike, @@ -17,7 +19,6 @@ import { nameForPassableSymbol, passableSymbolForName, } from '@endo/pass-style'; -import { X, Fail, q } from '@endo/errors'; /** @import {Passable, RemotableObject} from '@endo/pass-style' */ /** @import {Encoding, EncodingUnion} from './types.js' */ @@ -30,8 +31,8 @@ const { is, entries, fromEntries, - freeze, } = Object; +const { suppressTrapping } = extraObjectMethods; /** * Special property name that indicates an encoding that needs special @@ -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/package.json b/packages/pass-style/package.json index 62f8494297..1a1d557e3a 100644 --- a/packages/pass-style/package.json +++ b/packages/pass-style/package.json @@ -37,6 +37,7 @@ "@endo/env-options": "workspace:^", "@endo/errors": "workspace:^", "@endo/eventual-send": "workspace:^", + "@endo/non-trapping-shim": "^0.1.0", "@endo/promise-kit": "workspace:^" }, "devDependencies": { diff --git a/packages/pass-style/src/passStyle-helpers.js b/packages/pass-style/src/passStyle-helpers.js index 7107e11b89..a6eb4212f0 100644 --- a/packages/pass-style/src/passStyle-helpers.js +++ b/packages/pass-style/src/passStyle-helpers.js @@ -4,6 +4,7 @@ /** @import {PassStyle} from './types.js' */ import { X, q } from '@endo/errors'; +import { extraObjectMethods } from '@endo/non-trapping-shim'; const { isArray } = Array; const { prototype: functionPrototype } = Function; @@ -11,11 +12,12 @@ const { getOwnPropertyDescriptor, getPrototypeOf, hasOwnProperty: objectHasOwnProperty, - isFrozen, prototype: objectPrototype, + isFrozen, } = Object; const { apply } = Reflect; const { toStringTag: toStringTagSymbol } = Symbol; +const { isNonTrapping } = extraObjectMethods; const typedArrayPrototype = getPrototypeOf(Uint8Array.prototype); const typedArrayToStringTagDesc = getOwnPropertyDescriptor( @@ -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..42b9408e18 100644 --- a/packages/pass-style/src/passStyleOf.js +++ b/packages/pass-style/src/passStyleOf.js @@ -4,8 +4,9 @@ import { isPromise } from '@endo/promise-kit'; import { X, Fail, q, annotateError, makeError } from '@endo/errors'; -import { isObject, isTypedArray, PASS_STYLE } from './passStyle-helpers.js'; +import { extraObjectMethods } from '@endo/non-trapping-shim'; +import { isObject, isTypedArray, PASS_STYLE } from './passStyle-helpers.js'; import { CopyArrayHelper } from './copyArray.js'; import { CopyRecordHelper } from './copyRecord.js'; import { TaggedHelper } from './tagged.js'; @@ -31,7 +32,8 @@ import { assertPassableString } from './string.js'; /** @typedef {Exclude} HelperPassStyle */ const { ownKeys } = Reflect; -const { isFrozen, getOwnPropertyDescriptors, values } = Object; +const { getOwnPropertyDescriptors, values, isFrozen } = Object; +const { isNonTrapping } = extraObjectMethods; /** * @param {PassStyleHelper[]} passStyleHelpers @@ -143,14 +145,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 +182,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..774f6e2ce0 100644 --- a/packages/pass-style/src/remotable.js +++ b/packages/pass-style/src/remotable.js @@ -1,6 +1,7 @@ /// import { Fail, q } from '@endo/errors'; +import { extraObjectMethods } from '@endo/non-trapping-shim'; import { assertChecker, canBeMethod, @@ -24,10 +25,10 @@ const { ownKeys } = Reflect; const { isArray } = Array; const { getPrototypeOf, - isFrozen, prototype: objectPrototype, getOwnPropertyDescriptors, } = Object; +const { isNonTrapping } = extraObjectMethods; /** * @param {InterfaceSpec} iface @@ -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..dd8e035d86 100644 --- a/packages/pass-style/src/safe-promise.js +++ b/packages/pass-style/src/safe-promise.js @@ -2,13 +2,15 @@ import { isPromise } from '@endo/promise-kit'; import { q } from '@endo/errors'; +import { extraObjectMethods } from '@endo/non-trapping-shim'; import { assertChecker, hasOwnPropertyOf, CX } from './passStyle-helpers.js'; /** @import {Checker} from './types.js' */ -const { isFrozen, getPrototypeOf, getOwnPropertyDescriptor } = Object; +const { getPrototypeOf, getOwnPropertyDescriptor } = Object; const { ownKeys } = Reflect; const { toStringTag } = Symbol; +const { isNonTrapping } = extraObjectMethods; /** * @param {Promise} pr The value to examine @@ -88,7 +90,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 +134,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..581ee2e8c8 100644 --- a/packages/pass-style/test/passStyleOf.test.js +++ b/packages/pass-style/test/passStyleOf.test.js @@ -2,6 +2,7 @@ import test from '@endo/ses-ava/prepare-endo.js'; import { q } from '@endo/errors'; +import { extraObjectMethods } from '@endo/non-trapping-shim'; import { passStyleOf } from '../src/passStyleOf.js'; import { Far, ToFarFunction } from '../src/make-far.js'; @@ -13,8 +14,9 @@ const harden = /** @type {import('ses').Harden & { isFake?: boolean }} */ ( global.harden ); -const { getPrototypeOf, defineProperty, freeze } = Object; +const { getPrototypeOf, defineProperty } = Object; const { ownKeys } = Reflect; +const { suppressTrapping } = extraObjectMethods; test('passStyleOf basic success cases', t => { // Test in same order as `passStyleOf` for easier maintenance. @@ -208,7 +210,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 +388,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/make-hardener.js b/packages/ses/src/make-hardener.js index d377fd8793..84df99c9cb 100644 --- a/packages/ses/src/make-hardener.js +++ b/packages/ses/src/make-hardener.js @@ -21,6 +21,7 @@ // @ts-check +import { extraObjectMethods } from '@endo/non-trapping-shim'; import { Set, String, @@ -30,7 +31,6 @@ import { apply, arrayForEach, defineProperty, - freeze, getOwnPropertyDescriptor, getOwnPropertyDescriptors, getPrototypeOf, @@ -49,9 +49,12 @@ import { FERAL_STACK_GETTER, FERAL_STACK_SETTER, isError, + isFrozen, } from './commons.js'; import { assert } from './error/assert.js'; +const { suppressTrapping } = extraObjectMethods; + /** * @import {Harden} from '../types.js' */ @@ -128,6 +131,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 +189,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 diff --git a/yarn.lock b/yarn.lock index 98e3a54cd0..95cbc2de2b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -607,6 +607,7 @@ __metadata: "@endo/init": "workspace:^" "@endo/lockdown": "workspace:^" "@endo/nat": "workspace:^" + "@endo/non-trapping-shim": "npm:^0.1.0" "@endo/pass-style": "workspace:^" "@endo/promise-kit": "workspace:^" "@endo/ses-ava": "workspace:^" @@ -702,7 +703,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: @@ -721,6 +722,7 @@ __metadata: "@endo/errors": "workspace:^" "@endo/eventual-send": "workspace:^" "@endo/init": "workspace:^" + "@endo/non-trapping-shim": "npm:^0.1.0" "@endo/promise-kit": "workspace:^" "@endo/ses-ava": "workspace:^" "@fast-check/ava": "npm:^1.1.5" @@ -8960,6 +8962,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"