From c14e7763a91759086aafe58b7a1fb54598a70495 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Thu, 2 Jan 2025 10:12:44 -0800 Subject: [PATCH 1/4] refactor: prepare for non-trapping integrity trait --- packages/captp/src/trap.js | 16 ++++- packages/eventual-send/src/E.js | 25 ++++--- packages/eventual-send/src/handled-promise.js | 3 + .../far/test/marshal-far-function.test.js | 2 +- packages/marshal/src/marshal-stringify.js | 14 +++- .../marshal/test/marshal-far-function.test.js | 2 +- packages/pass-style/test/passStyleOf.test.js | 65 +++++++++---------- packages/ses/docs/preparing-for-stabilize.md | 24 +++++++ packages/ses/src/commons.js | 6 -- .../src/sloppy-globals-scope-terminator.js | 10 ++- packages/ses/src/strict-scope-terminator.js | 12 +++- 11 files changed, 119 insertions(+), 60 deletions(-) create mode 100644 packages/ses/docs/preparing-for-stabilize.md diff --git a/packages/captp/src/trap.js b/packages/captp/src/trap.js index 5b7c81afda..8d95e54946 100644 --- a/packages/captp/src/trap.js +++ b/packages/captp/src/trap.js @@ -1,5 +1,7 @@ // Lifted mostly from `@endo/eventual-send/src/E.js`. +const { freeze } = Object; + /** * Default implementation of Trap for near objects. * @@ -62,11 +64,21 @@ const TrapProxyHandler = (x, trapImpl) => { */ export const makeTrap = trapImpl => { const Trap = x => { + /** + * `freeze` but not `harden` the proxy target so it remains trapping. + * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md + */ + const target = freeze(() => {}); const handler = TrapProxyHandler(x, trapImpl); - return harden(new Proxy(() => {}, handler)); + return new Proxy(target, handler); }; const makeTrapGetterProxy = x => { + /** + * `freeze` but not `harden` the proxy target so it remains trapping. + * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md + */ + const target = freeze(Object.create(null)); const handler = harden({ ...baseFreezableProxyHandler, has(_target, _prop) { @@ -77,7 +89,7 @@ export const makeTrap = trapImpl => { return trapImpl.get(x, prop); }, }); - return new Proxy(Object.create(null), handler); + return new Proxy(target, handler); }; Trap.get = makeTrapGetterProxy; diff --git a/packages/eventual-send/src/E.js b/packages/eventual-send/src/E.js index a114e3ccb2..b87727418c 100644 --- a/packages/eventual-send/src/E.js +++ b/packages/eventual-send/src/E.js @@ -2,7 +2,7 @@ import { trackTurns } from './track-turns.js'; import { makeMessageBreakpointTester } from './message-breakpoints.js'; const { details: X, quote: q, Fail, error: makeError } = assert; -const { assign, create } = Object; +const { assign, create, freeze } = Object; /** * @import { HandledPromiseConstructor } from './types.js'; @@ -171,6 +171,16 @@ const makeEGetProxyHandler = (x, HandledPromise) => * @param {HandledPromiseConstructor} HandledPromise */ const makeE = HandledPromise => { + /** + * `freeze` but not `harden` the proxy target so it remains trapping. + * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md + */ + const funcTarget = freeze(() => {}); + /** + * `freeze` but not `harden` the proxy target so it remains trapping. + * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md + */ + const objTarget = freeze(create(null)); return harden( assign( /** @@ -183,7 +193,7 @@ const makeE = HandledPromise => { * @returns {ECallableOrMethods>} method/function call proxy */ // @ts-expect-error XXX typedef - x => harden(new Proxy(() => {}, makeEProxyHandler(x, HandledPromise))), + x => new Proxy(funcTarget, makeEProxyHandler(x, HandledPromise)), { /** * E.get(x) returns a proxy on which you can get arbitrary properties. @@ -196,11 +206,8 @@ const makeE = HandledPromise => { * @returns {EGetters>} property get proxy * @readonly */ - get: x => - // @ts-expect-error XXX typedef - harden( - new Proxy(create(null), makeEGetProxyHandler(x, HandledPromise)), - ), + // @ts-expect-error XXX typedef + get: x => new Proxy(objTarget, makeEGetProxyHandler(x, HandledPromise)), /** * E.resolve(x) converts x to a handled promise. It is @@ -224,9 +231,7 @@ const makeE = HandledPromise => { */ sendOnly: x => // @ts-expect-error XXX typedef - harden( - new Proxy(() => {}, makeESendOnlyProxyHandler(x, HandledPromise)), - ), + new Proxy(funcTarget, makeESendOnlyProxyHandler(x, HandledPromise)), /** * E.when(x, res, rej) is equivalent to diff --git a/packages/eventual-send/src/handled-promise.js b/packages/eventual-send/src/handled-promise.js index 17785b3529..cef6a957a6 100644 --- a/packages/eventual-send/src/handled-promise.js +++ b/packages/eventual-send/src/handled-promise.js @@ -309,6 +309,9 @@ export const makeHandledPromise = () => { if (proxyOpts) { const { handler: proxyHandler, + // The proxy target can be frozen but should not be hardened + // so it remains trapping. + // See https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md target: proxyTarget, revokerCallback, } = proxyOpts; diff --git a/packages/far/test/marshal-far-function.test.js b/packages/far/test/marshal-far-function.test.js index c86c2b1fb6..28d544e6c6 100644 --- a/packages/far/test/marshal-far-function.test.js +++ b/packages/far/test/marshal-far-function.test.js @@ -58,7 +58,7 @@ test('Data can contain far functions', t => { const arrow = Far('arrow', a => a + 1); t.is(passStyleOf(harden({ x: 8, foo: arrow })), 'copyRecord'); const mightBeMethod = a => a + 1; - t.throws(() => passStyleOf(freeze({ x: 8, foo: mightBeMethod })), { + t.throws(() => passStyleOf(harden({ x: 8, foo: mightBeMethod })), { message: /Remotables with non-methods like "x" /, }); }); diff --git a/packages/marshal/src/marshal-stringify.js b/packages/marshal/src/marshal-stringify.js index e581cdefed..50d80779c5 100644 --- a/packages/marshal/src/marshal-stringify.js +++ b/packages/marshal/src/marshal-stringify.js @@ -5,6 +5,8 @@ import { makeMarshal } from './marshal.js'; /** @import {Passable} from '@endo/pass-style' */ +const { freeze } = Object; + /** @type {import('./types.js').ConvertValToSlot} */ const doNotConvertValToSlot = val => Fail`Marshal's stringify rejects presences and promises ${val}`; @@ -23,7 +25,12 @@ const badArrayHandler = harden({ }, }); -const badArray = harden(new Proxy(harden([]), badArrayHandler)); +/** + * `freeze` but not `harden` the proxy target so it remains trapping. + * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md + */ +const arrayTarget = freeze(/** @type {any[]} */ ([])); +const badArray = new Proxy(arrayTarget, badArrayHandler); const { serialize, unserialize } = makeMarshal( doNotConvertValToSlot, @@ -48,7 +55,10 @@ harden(stringify); */ const parse = str => unserialize( - harden({ + // `freeze` but not `harden` since the `badArray` proxy and its target + // must remain trapping. + // See https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md + freeze({ body: str, slots: badArray, }), diff --git a/packages/marshal/test/marshal-far-function.test.js b/packages/marshal/test/marshal-far-function.test.js index b546b11621..223fdd27e5 100644 --- a/packages/marshal/test/marshal-far-function.test.js +++ b/packages/marshal/test/marshal-far-function.test.js @@ -60,7 +60,7 @@ test('Data can contain far functions', t => { const arrow = Far('arrow', a => a + 1); t.is(passStyleOf(harden({ x: 8, foo: arrow })), 'copyRecord'); const mightBeMethod = a => a + 1; - t.throws(() => passStyleOf(freeze({ x: 8, foo: mightBeMethod })), { + t.throws(() => passStyleOf(harden({ x: 8, foo: mightBeMethod })), { message: /Remotables with non-methods like "x" /, }); }); diff --git a/packages/pass-style/test/passStyleOf.test.js b/packages/pass-style/test/passStyleOf.test.js index d09cd55260..f13d3f1fd7 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 } = Object; +const { getPrototypeOf, defineProperty, freeze } = Object; const { ownKeys } = Reflect; test('passStyleOf basic success cases', t => { @@ -193,16 +193,22 @@ test('passStyleOf testing remotables', t => { const tagRecord1 = harden(makeTagishRecord('Alleged: manually constructed')); /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ - const farObj1 = harden({ - __proto__: tagRecord1, - }); + const farObj1 = harden({ __proto__: tagRecord1 }); t.is(passStyleOf(farObj1), 'remotable'); const tagRecord2 = makeTagishRecord('Alleged: tagRecord not hardened'); - /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ - const farObj2 = Object.freeze({ - __proto__: tagRecord2, - }); + /** + * TODO In order to run this test before we have explicit support for a + * non-trapping integrity trait, we have to `freeze` here but not `harden`. + * However, once we do have that support, and `passStyleOf` checks that + * its argument is also non-trapping, we still need to avoid `harden` + * because that would also hardden `__proto__`. So we will need to + * 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 + * + * @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 + */ + const farObj2 = freeze({ __proto__: tagRecord2 }); if (harden.isFake) { t.is(passStyleOf(farObj2), 'remotable'); } else { @@ -212,39 +218,27 @@ test('passStyleOf testing remotables', t => { }); } - const tagRecord3 = Object.freeze( - makeTagishRecord('Alleged: both manually frozen'), - ); + const tagRecord3 = harden(makeTagishRecord('Alleged: both manually frozen')); /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ - const farObj3 = Object.freeze({ - __proto__: tagRecord3, - }); + const farObj3 = harden({ __proto__: tagRecord3 }); t.is(passStyleOf(farObj3), 'remotable'); const tagRecord4 = harden(makeTagishRecord('Remotable')); /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ - const farObj4 = harden({ - __proto__: tagRecord4, - }); + const farObj4 = harden({ __proto__: tagRecord4 }); t.is(passStyleOf(farObj4), 'remotable'); const tagRecord5 = harden(makeTagishRecord('Not alleging')); - const farObj5 = harden({ - __proto__: tagRecord5, - }); + const farObj5 = harden({ __proto__: tagRecord5 }); t.throws(() => passStyleOf(farObj5), { message: /For now, iface "Not alleging" must be "Remotable" or begin with "Alleged: " or "DebugName: "; unimplemented/, }); const tagRecord6 = harden(makeTagishRecord('Alleged: manually constructed')); - const farObjProto6 = harden({ - __proto__: tagRecord6, - }); + const farObjProto6 = harden({ __proto__: tagRecord6 }); /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ - const farObj6 = harden({ - __proto__: farObjProto6, - }); + const farObj6 = harden({ __proto__: farObjProto6 }); t.is(passStyleOf(farObj6), 'remotable', 'tagRecord grandproto is accepted'); // Our current agoric-sdk plans for far classes are to create a class-like @@ -323,12 +317,8 @@ test('passStyleOf testing remotables', t => { const fauxTagRecordB = harden( makeTagishRecord('Alleged: manually constructed', harden({})), ); - const farObjProtoB = harden({ - __proto__: fauxTagRecordB, - }); - const farObjB = harden({ - __proto__: farObjProtoB, - }); + const farObjProtoB = harden({ __proto__: fauxTagRecordB }); + const farObjB = harden({ __proto__: farObjProtoB }); t.throws(() => passStyleOf(farObjB), { message: 'cannot serialize Remotables with non-methods like "Symbol(passStyle)" in "[Alleged: manually constructed]"', @@ -387,7 +377,16 @@ test('remotables - safety from the gibson042 attack', t => { }, ); - const makeInput = () => Object.freeze({ __proto__: mercurialProto }); + /** + * TODO In order to run this test before we have explicit support for a + * non-trapping integrity trait, we have to `freeze` here but not `harden`. + * However, once we do have that support, and `passStyleOf` checks that + * its argument is also non-trapping, we still need to avoid `harden` + * because that would also hardden `__proto__`. So we will need to + * 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 input1 = makeInput(); const input2 = makeInput(); diff --git a/packages/ses/docs/preparing-for-stabilize.md b/packages/ses/docs/preparing-for-stabilize.md new file mode 100644 index 0000000000..63a432b46f --- /dev/null +++ b/packages/ses/docs/preparing-for-stabilize.md @@ -0,0 +1,24 @@ +# Preparing for the Non-trapping Integrity Trait + +The [Stabilize proposal](https://github.com/tc39/proposal-stabilize) is currently at stage 1 of the tc39 process. It proposes three distinct integrity traits whose current placeholder names are: +- ***fixed***: would mitigate the return-override mistake by preventing objects with this trait from being stamped with new class-private-fields. +- ***overridable***: would mitigate the assignment-override mistake by enabling non-writable properties inherited from an object with this trait to be overridden by property assignment on an inheriting object. +- ***non-trapping***: would mitigate proxy-based reentrancy hazards by having a proxy whose target carries this trait never trap to its handler, but rather just perform the default action directly on this non-trapping target. + +Draft PR [feat(non-trapping-shim): ponyfill and shim for the non-trapping integrity trait #2673](https://github.com/endojs/endo/pull/2673) is a ponyfill and shim for this non-trapping integrity trait. The names it introduces are placeholders, since the bikeshedding process for these names has not yet concluded. + +Draft PR [feat(ses,pass-style): use non-trapping integrity trait for safety #2675](https://github.com/endojs/endo/pull/2675) uses this support for the non-trapping integity trait to mitigate reentrancy attacks from hardened objects, expecially passable copy-data objects like copyLists, copyRecords, and taggeds. To do so, it makes two fundamental changes: +- Where `harden` made the object at every step frozen, that PR changes `harden` to also make those objects non-trapping. +- Where `passStyleOf` checked that objects are frozen, that PR changes `passStyleOf` to also check that those objects are non-trapping. + +## How proxy code should prepare + +[#2673](https://github.com/endojs/endo/pull/2673) will *by default* produce proxies that refuse to be made non-trapping. An explicit handler trap (whose name is TBD) will need to be explicitly provided to make a proxy that allows itself to be made non-trapping. This is the right default, because proxies on frozen almost-empty objects can still have useful trap behavior for their `get`, `set`, `has`, and `apply` traps. Even on a frozen target +- The `get`, `set`, and `has` traps applied to a non-own property name are still general traps that can have useful trapping behavior. +- The `apply` trap can ignore the target's call behavior and just do its own thing. + +However, to prepare for these changes, we need to avoid hardening both such proxies and their targets. We need to avoid hardening their target because this will bypass the traps. We need to avoid hardening the proxy because such proxies will *by default* refuse to be made non-trapping, and thus refuse to be hardened. + +## How passable objects should prepare + +Although we think of `passStyleOf` as requiring its input to be hardened, `passStyleOf` instead checked that each relevant object is frozen. Manually freezing all objects reachable from a root object had been equivalent to hardening that root object. With these changes, even such manual transitive freezing will not make an object passable. To prepare for these changes, use `harden` explicitly instead. diff --git a/packages/ses/src/commons.js b/packages/ses/src/commons.js index 5925c17eae..5211fa075d 100644 --- a/packages/ses/src/commons.js +++ b/packages/ses/src/commons.js @@ -269,12 +269,6 @@ export const finalizationRegistryUnregister = export const getConstructorOf = fn => reflectGet(getPrototypeOf(fn), 'constructor'); -/** - * immutableObject - * An immutable (frozen) empty object that is safe to share. - */ -export const immutableObject = freeze(create(null)); - /** * isObject tests whether a value is an object. * Today, this is equivalent to: diff --git a/packages/ses/src/sloppy-globals-scope-terminator.js b/packages/ses/src/sloppy-globals-scope-terminator.js index 01e3860856..2899e348de 100644 --- a/packages/ses/src/sloppy-globals-scope-terminator.js +++ b/packages/ses/src/sloppy-globals-scope-terminator.js @@ -3,7 +3,6 @@ import { create, freeze, getOwnPropertyDescriptors, - immutableObject, reflectSet, } from './commons.js'; import { @@ -11,6 +10,13 @@ import { alwaysThrowHandler, } from './strict-scope-terminator.js'; +/** + * `freeze` but not `harden` the proxy target so it remains trapping. + * Thus, it should not be shared outside this module. + * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md + */ +const onlyFrozenObject = freeze(create(null)); + /* * createSloppyGlobalsScopeTerminator() * strictScopeTerminatorHandler manages a scopeTerminator Proxy which serves as @@ -45,7 +51,7 @@ export const createSloppyGlobalsScopeTerminator = globalObject => { ); const sloppyGlobalsScopeTerminator = new Proxy( - immutableObject, + onlyFrozenObject, sloppyGlobalsScopeTerminatorHandler, ); diff --git a/packages/ses/src/strict-scope-terminator.js b/packages/ses/src/strict-scope-terminator.js index 7257afccd1..8680c8f21a 100644 --- a/packages/ses/src/strict-scope-terminator.js +++ b/packages/ses/src/strict-scope-terminator.js @@ -7,12 +7,18 @@ import { freeze, getOwnPropertyDescriptors, globalThis, - immutableObject, } from './commons.js'; import { assert } from './error/assert.js'; const { Fail, quote: q } = assert; +/** + * `freeze` but not `harden` the proxy target so it remains trapping. + * Thus, it should not be shared outside this module. + * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md + */ +const onlyFrozenObject = freeze(create(null)); + /** * alwaysThrowHandler * This is an object that throws if any property is called. It's used as @@ -21,7 +27,7 @@ const { Fail, quote: q } = assert; * create one and share it between all Proxy handlers. */ export const alwaysThrowHandler = new Proxy( - immutableObject, + onlyFrozenObject, freeze({ get(_shadow, prop) { Fail`Please report unexpected scope handler trap: ${q(String(prop))}`; @@ -88,6 +94,6 @@ export const strictScopeTerminatorHandler = freeze( ); export const strictScopeTerminator = new Proxy( - immutableObject, + onlyFrozenObject, strictScopeTerminatorHandler, ); From e8ac65becd64debf481d0adc84579dbb298fbc7e Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Mon, 13 Jan 2025 14:49:50 -0800 Subject: [PATCH 2/4] fixup! review suggestions --- packages/eventual-send/src/E.js | 42 ++++++++++++----- packages/pass-style/test/passStyleOf.test.js | 47 ++++++++++++++++++-- 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/packages/eventual-send/src/E.js b/packages/eventual-send/src/E.js index b87727418c..7634f267ce 100644 --- a/packages/eventual-send/src/E.js +++ b/packages/eventual-send/src/E.js @@ -167,20 +167,42 @@ const makeEGetProxyHandler = (x, HandledPromise) => get: (_target, prop) => HandledPromise.get(x, prop), }); +/** + * `freeze` but not `harden` the proxy target so it remains trapping. + * This is safe to share between proxy instances because they are encapsulated + * within the proxy. + * - Before stabilize/suppressTrapping, this is safe + * because they are already frozen, and so they cannot be damaged by the + * proxies that encapsulate them. + * - After stabilize/suppressTrapping, this is safe because the only damage + * that could be done would be by stabilize/suppressTrapping. These proxies + * do not explicitly provide such a trap, and thus will use the default + * behavior which is to refuse to be made non-trapping. + * + * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md + */ +const funcTarget = freeze(() => {}); + +/** + * `freeze` but not `harden` the proxy target so it remains trapping. + * This is safe to share between proxy instances because they are encapsulated + * within the proxy. + * - Before stabilize/suppressTrapping, this is safe + * because they are already frozen, and so they cannot be damaged by the + * proxies that encapsulate them. + * - After stabilize/suppressTrapping, this is safe because the only damage + * that could be done would be by stabilize/suppressTrapping. These proxies + * do not explicitly provide such a trap, and thus will use the default + * behavior which is to refuse to be made non-trapping. + * + * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md + */ +const objTarget = freeze(create(null)); + /** * @param {HandledPromiseConstructor} HandledPromise */ const makeE = HandledPromise => { - /** - * `freeze` but not `harden` the proxy target so it remains trapping. - * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md - */ - const funcTarget = freeze(() => {}); - /** - * `freeze` but not `harden` the proxy target so it remains trapping. - * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md - */ - const objTarget = freeze(create(null)); return harden( assign( /** diff --git a/packages/pass-style/test/passStyleOf.test.js b/packages/pass-style/test/passStyleOf.test.js index f13d3f1fd7..b33bfd79a0 100644 --- a/packages/pass-style/test/passStyleOf.test.js +++ b/packages/pass-style/test/passStyleOf.test.js @@ -110,13 +110,17 @@ test('some passStyleOf rejections', t => { }); /** - * For testing purposes, makes a TagRecord-like object with + * For testing purposes, makes a *non-frozen* TagRecord-like object with * non-enumerable PASS_STYLE and Symbol.toStringTag properties. * A valid Remotable must inherit from a valid TagRecord. + * - Before stabilize/suppressTrapping, a valid TagRecord must be frozen. + * - After stabilize/suppressTrapping, a valid TagRecord must also be + * stable/non-trapping, for example, because it was hardened. * * @param {string} [tag] * @param {object|null} [proto] * @returns {{ [PASS_STYLE]: 'remotable', [Symbol.toStringTag]: string }} + * @see https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md */ const makeTagishRecord = (tag = 'Remotable', proto = undefined) => { return Object.create(proto === undefined ? Object.prototype : proto, { @@ -193,16 +197,21 @@ test('passStyleOf testing remotables', t => { const tagRecord1 = harden(makeTagishRecord('Alleged: manually constructed')); /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObj1 = harden({ __proto__: tagRecord1 }); t.is(passStyleOf(farObj1), 'remotable'); const tagRecord2 = makeTagishRecord('Alleged: tagRecord not hardened'); /** + * Do not freeze `tagRecord2` in order to test that an object with + * a non-frozen __proto__ is not passable. + * * TODO In order to run this test before we have explicit support for a * non-trapping integrity trait, we have to `freeze` here but not `harden`. * However, once we do have that support, and `passStyleOf` checks that * its argument is also non-trapping, we still need to avoid `harden` - * because that would also hardden `__proto__`. So we will need to + * because that would also harden `__proto__`. So we will need to * 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 * @@ -220,15 +229,21 @@ test('passStyleOf testing remotables', t => { const tagRecord3 = harden(makeTagishRecord('Alleged: both manually frozen')); /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObj3 = harden({ __proto__: tagRecord3 }); t.is(passStyleOf(farObj3), 'remotable'); const tagRecord4 = harden(makeTagishRecord('Remotable')); /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObj4 = harden({ __proto__: tagRecord4 }); t.is(passStyleOf(farObj4), 'remotable'); const tagRecord5 = harden(makeTagishRecord('Not alleging')); + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObj5 = harden({ __proto__: tagRecord5 }); t.throws(() => passStyleOf(farObj5), { message: @@ -236,8 +251,12 @@ test('passStyleOf testing remotables', t => { }); const tagRecord6 = harden(makeTagishRecord('Alleged: manually constructed')); + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObjProto6 = harden({ __proto__: tagRecord6 }); /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObj6 = harden({ __proto__: farObjProto6 }); t.is(passStyleOf(farObj6), 'remotable', 'tagRecord grandproto is accepted'); @@ -292,6 +311,8 @@ test('passStyleOf testing remotables', t => { const tagRecordA1 = harden( makeTagishRecord('Alleged: null-proto tagRecord proto', null), ); + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObjA1 = harden({ __proto__: tagRecordA1 }); t.throws( () => passStyleOf(farObjA1), @@ -302,7 +323,11 @@ test('passStyleOf testing remotables', t => { const tagRecordA2 = harden( makeTagishRecord('Alleged: null-proto tagRecord grandproto', null), ); + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObjProtoA2 = harden({ __proto__: tagRecordA2 }); + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObjA2 = harden({ __proto__: farObjProtoA2 }); t.throws( () => passStyleOf(farObjA2), @@ -317,7 +342,11 @@ test('passStyleOf testing remotables', t => { const fauxTagRecordB = harden( makeTagishRecord('Alleged: manually constructed', harden({})), ); + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObjProtoB = harden({ __proto__: fauxTagRecordB }); + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const farObjB = harden({ __proto__: farObjProtoB }); t.throws(() => passStyleOf(farObjB), { message: @@ -329,6 +358,8 @@ test('passStyleOf testing remotables', t => { ); Object.defineProperty(farObjProtoWithExtra, 'extra', { value: () => {} }); harden(farObjProtoWithExtra); + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const badFarObjExtraProtoProp = harden({ __proto__: farObjProtoWithExtra }); t.throws(() => passStyleOf(badFarObjExtraProtoProp), { message: 'Unexpected properties on Remotable Proto ["extra"]', @@ -378,11 +409,17 @@ test('remotables - safety from the gibson042 attack', t => { ); /** + * Do not freeze `mercurialProto` in order to test that an object with + * a non-frozen __proto__ is not passable. Once we have support for + * non-trapping, we should generalize this test (or add a new one) where + * `mercurialProto` is frozen but still trapping. This test would then + * test that a valid TagRecord must be non-trapping. + * * TODO In order to run this test before we have explicit support for a * non-trapping integrity trait, we have to `freeze` here but not `harden`. * However, once we do have that support, and `passStyleOf` checks that * its argument is also non-trapping, we still need to avoid `harden` - * because that would also hardden `__proto__`. So we will need to + * because that would also harden `__proto__`. So we will need to * 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 */ @@ -448,12 +485,16 @@ test('Allow toStringTag overrides', t => { t.is(`${q(alice)}`, '"[DebugName: Allison]"'); /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const carol = harden({ __proto__: alice }); t.is(passStyleOf(carol), 'remotable'); t.is(`${carol}`, '[object DebugName: Allison]'); t.is(`${q(carol)}`, '"[DebugName: Allison]"'); /** @type {any} UNTIL https://github.com/microsoft/TypeScript/issues/38385 */ + // TODO Once we have support for explicit non-trapping, this should change + // from `harden` to `suppressTrapping` or `stabilize`. const bob = harden({ __proto__: carol, [Symbol.toStringTag]: 'DebugName: Robert', From bf416a218ffa65435bad546301d6a80f3bfcbf7a Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Sun, 29 Dec 2024 14:26:32 -0800 Subject: [PATCH 3/4] feat(ses): sham of non-trapping shim and ponyfill --- .../sham-non-trapping/non-trapping-pony.js | 292 ++++++++++++++++++ .../sham-non-trapping/non-trapping-shim.js | 11 + .../non-trapping-pony.test.js | 32 ++ .../non-trapping-shim.test.js | 29 ++ 4 files changed, 364 insertions(+) create mode 100644 packages/ses/src/sham-non-trapping/non-trapping-pony.js create mode 100644 packages/ses/src/sham-non-trapping/non-trapping-shim.js create mode 100644 packages/ses/test/sham-non-trapping/non-trapping-pony.test.js create mode 100644 packages/ses/test/sham-non-trapping/non-trapping-shim.test.js diff --git a/packages/ses/src/sham-non-trapping/non-trapping-pony.js b/packages/ses/src/sham-non-trapping/non-trapping-pony.js new file mode 100644 index 0000000000..1fbe3e4289 --- /dev/null +++ b/packages/ses/src/sham-non-trapping/non-trapping-pony.js @@ -0,0 +1,292 @@ +/* eslint-disable @endo/no-polymorphic-call */ +/* eslint-disable no-restricted-globals */ +const OriginalObject = Object; +const OriginalReflect = Reflect; +const OriginalProxy = Proxy; +const { freeze, defineProperty, hasOwn } = OriginalObject; +const { apply, construct, ownKeys } = OriginalReflect; + +const nonTrappingSet = new WeakSet(); + +const proxyHandlerMap = new WeakMap(); + +const isPrimitive = specimen => OriginalObject(specimen) !== specimen; + +/** + * Corresponds to the internal function shared by `Object.isNonTrapping` and + * `Reflect.isNonTrapping`. + * + * @param {any} specimen + * @param {boolean} shouldThrow + * @returns {boolean} + */ +const isNonTrappingInternal = (specimen, shouldThrow) => { + if (nonTrappingSet.has(specimen)) { + return true; + } + if (!proxyHandlerMap.has(specimen)) { + return false; + } + const [target, handler] = proxyHandlerMap.get(specimen); + if (isNonTrappingInternal(target, shouldThrow)) { + nonTrappingSet.add(specimen); + return true; + } + const trap = handler.isNonTrapping; + if (trap === undefined) { + return false; + } + const result = apply(trap, handler, [target]); + const ofTarget = isNonTrappingInternal(target, shouldThrow); + if (result !== ofTarget) { + if (shouldThrow) { + throw TypeError( + `'isNonTrapping' proxy trap does not reflect 'isNonTrapping' of proxy target (which is '${ofTarget}')`, + ); + } + return false; + } + if (result) { + nonTrappingSet.add(specimen); + } + return result; +}; + +/** + * Corresponds to the internal function shared by `Object.suppressTrapping` and + * `Reflect.suppressTrapping`. + * + * @param {any} specimen + * @param {boolean} shouldThrow + * @returns {boolean} + */ +const suppressTrappingInternal = (specimen, shouldThrow) => { + if (nonTrappingSet.has(specimen)) { + return true; + } + freeze(specimen); + if (!proxyHandlerMap.has(specimen)) { + nonTrappingSet.add(specimen); + return true; + } + const [target, handler] = proxyHandlerMap.get(specimen); + if (isNonTrappingInternal(target, shouldThrow)) { + nonTrappingSet.add(specimen); + return true; + } + const trap = handler.suppressTrapping; + if (trap === undefined) { + const result = suppressTrappingInternal(target, shouldThrow); + if (result) { + nonTrappingSet.add(specimen); + } + return result; + } + const result = apply(trap, handler, [target]); + const ofTarget = isNonTrappingInternal(target, shouldThrow); + if (result !== ofTarget) { + if (shouldThrow) { + throw TypeError( + `'suppressTrapping' proxy trap does not reflect 'isNonTrapping' of proxy target (which is '${ofTarget}')`, + ); + } + return false; + } + if (result) { + nonTrappingSet.add(specimen); + } + return result; +}; + +export const extraReflectMethods = freeze({ + isNonTrapping(target) { + if (isPrimitive(target)) { + throw TypeError('Reflect.isNonTrapping called on non-object'); + } + return isNonTrappingInternal(target, false); + }, + suppressTrapping(target) { + if (isPrimitive(target)) { + throw TypeError('Reflect.suppressTrapping called on non-object'); + } + return suppressTrappingInternal(target, false); + }, +}); + +export const extraObjectMethods = freeze({ + isNonTrapping(target) { + if (isPrimitive(target)) { + return true; + } + return isNonTrappingInternal(target, true); + }, + suppressTrapping(target) { + if (isPrimitive(target)) { + return target; + } + if (suppressTrappingInternal(target, true)) { + return target; + } + throw TypeError('suppressTrapping trap returned falsy'); + }, +}); + +const addExtras = (base, ...extrasArgs) => { + for (const extras of extrasArgs) { + for (const key of ownKeys(extras)) { + if (base[key] !== extras[key]) { + defineProperty(base, key, { + value: extras[key], + writable: true, + enumerable: false, + configurable: true, + }); + } + } + } +}; + +/** In the shim, `ReflectPlus` replaces the global `Reflect`. */ +const ReflectPlus = {}; +addExtras(ReflectPlus, OriginalReflect, extraReflectMethods); +export { ReflectPlus }; + +/** + * In the shim, `ObjectPlus` replaces the global `Object`. + * + * @type {ObjectConstructor} + */ +// @ts-expect-error TS does not know the rest of the type is added below +const ObjectPlus = function Object(...args) { + if (new.target) { + return construct(OriginalObject, args, new.target); + } else { + return apply(OriginalObject, this, args); + } +}; +// @ts-expect-error We actually can assign to its `.prototype`. +ObjectPlus.prototype = OriginalObject.prototype; +addExtras(ObjectPlus, OriginalObject, extraObjectMethods); +export { ObjectPlus }; + +/** + * A way to store the `originalHandler` on the `handlerPlus` without + * possible conflict with an future trap name. + * + * Normally, we'd use a WeakMap for this, so the property is also + * undiscoverable. But in this case, the `handlerPlus` objects are + * safely encapsulated within this module, so no one is in a position to + * discovery this property by inspection. + */ +const ORIGINAL_HANDLER = Symbol('OriginalHandler'); + +const metaHandler = freeze({ + get(_, trapName, handlerPlus) { + /** + * The `trapPlus` method is an enhanced version of + * `originalHandler[trapName]`. If the handlerPlus has no own `trapName` + * property, then the `get` of the metaHandler is called, which returns + * the `trapPlus`, which is then called as the trap of the returned + * proxyPlus. When so called, it installs an own `handlerPlus[trapName]` + * which is either `undefined` or this same `trapPlus`, to avoid further + * need to meta-handle that `handlerPlus[trapName]`. + * + * @param {any} target + * @param {any[]} rest + */ + const trapPlus = freeze((target, ...rest) => { + if (isNonTrappingInternal(target, true)) { + defineProperty(handlerPlus, trapName, { + value: undefined, + writable: false, + enumerable: true, + configurable: false, + }); + } else { + if (!hasOwn(handlerPlus, trapName)) { + defineProperty(handlerPlus, trapName, { + value: trapPlus, + writable: false, + enumerable: true, + configurable: true, + }); + } + const { [ORIGINAL_HANDLER]: originalHandler } = handlerPlus; + const trap = originalHandler[trapName]; + if (trap !== undefined) { + // Note that whether `trap === undefined` can change dynamically, + // so we do not install an own `handlerPlus[trapName] === undefined` + // for that case. We still install or preserve an own + // `handlerPlus[trapName] === trapPlus` until the target is + // seen to be non-trapping. + return apply(trap, originalHandler, [target, ...rest]); + } + } + return ReflectPlus[trapName](target, ...rest); + }); + return trapPlus; + }, +}); + +/** + * A handlerPlus starts as a fresh empty object that inherits from a proxy + * whose handler is the shared generic metaHandler. + * Thus, the metaHandler's `get` method is called only when the + * `handlerPlus` does not have a property overriding that `trapName`. + * In that case, the metaHandler's `get` is called with its `receiver` + * being the `handlerPlus`. + * + * @param {ProxyHandler} originalHandler + * @returns {ProxyHandler & { + * isNonTrapping: (target: any) => boolean, + * suppressTrapping: (target: any) => boolean, + * originalHandler: ProxyHandler + * }} + */ +const makeHandlerPlus = originalHandler => ({ + // @ts-expect-error TS does not know what this __proto__ is doing + __proto__: new OriginalProxy({}, metaHandler), + [ORIGINAL_HANDLER]: originalHandler, +}); + +const ProxyInternal = function Proxy(target, handler) { + if (new.target !== ProxyInternal) { + if (new.target === undefined) { + throw TypeError('Proxy constructor requires "new"'); + } + throw TypeError('Safe Proxy shim does not support subclassing'); + } + const handlerPlus = makeHandlerPlus(handler); + const proxy = new OriginalProxy(target, handlerPlus); + proxyHandlerMap.set(proxy, [target, handler]); + return proxy; +}; + +/** + * In the shim, `ProxyPlus` replaces the global `Proxy`. + * + * We use `bind` as the only way for user code to produce a + * constructible function (i.e., one that responds to `new`) without a + * `.prototype` property. + * + * @type {ProxyConstructor} + */ +const ProxyPlus = ProxyInternal.bind(undefined); +defineProperty(ProxyPlus, 'name', { value: 'Proxy' }); + +ProxyPlus.revocable = (target, handler) => { + const handlerPlus = makeHandlerPlus(handler); + const { proxy, revoke } = OriginalProxy.revocable(target, handlerPlus); + proxyHandlerMap.set(proxy, [target, handler]); + return { + proxy, + revoke() { + if (isNonTrappingInternal(target, true)) { + throw TypeError('Cannot revoke non-trapping proxy'); + } + revoke(); + }, + }; +}; + +export { ProxyPlus }; diff --git a/packages/ses/src/sham-non-trapping/non-trapping-shim.js b/packages/ses/src/sham-non-trapping/non-trapping-shim.js new file mode 100644 index 0000000000..7174b458a3 --- /dev/null +++ b/packages/ses/src/sham-non-trapping/non-trapping-shim.js @@ -0,0 +1,11 @@ +/* eslint-disable no-restricted-globals */ +/* global globalThis */ +import { ReflectPlus, ObjectPlus, ProxyPlus } from './non-trapping-pony.js'; + +globalThis.Reflect = ReflectPlus; + +globalThis.Object = ObjectPlus; +// eslint-disable-next-line no-extend-native +Object.prototype.constructor = ObjectPlus; + +globalThis.Proxy = ProxyPlus; diff --git a/packages/ses/test/sham-non-trapping/non-trapping-pony.test.js b/packages/ses/test/sham-non-trapping/non-trapping-pony.test.js new file mode 100644 index 0000000000..9fdf421149 --- /dev/null +++ b/packages/ses/test/sham-non-trapping/non-trapping-pony.test.js @@ -0,0 +1,32 @@ +// Uses 'ava' rather than @endo/ses-ava to avoid worries about cyclic +// dependencies. We will need similar tests is higher level packages, in order +// to test compat with ses and ses-ava. +import test from 'ava'; +import { + ReflectPlus, + ProxyPlus, +} from '../../src/sham-non-trapping/non-trapping-pony.js'; + +const { freeze, isFrozen } = Object; + +test('non-trapping-pony', async t => { + const specimen = { foo: 8 }; + + const sillyHandler = freeze({ + get(target, prop, receiver) { + return [target, prop, receiver]; + }, + }); + + const safeProxy = new ProxyPlus(specimen, sillyHandler); + + t.false(ReflectPlus.isNonTrapping(specimen)); + t.false(isFrozen(specimen)); + t.deepEqual(safeProxy.foo, [specimen, 'foo', safeProxy]); + + t.true(ReflectPlus.suppressTrapping(specimen)); + + t.true(ReflectPlus.isNonTrapping(specimen)); + t.true(isFrozen(specimen)); + t.deepEqual(safeProxy.foo, 8); +}); diff --git a/packages/ses/test/sham-non-trapping/non-trapping-shim.test.js b/packages/ses/test/sham-non-trapping/non-trapping-shim.test.js new file mode 100644 index 0000000000..7e5bccbe58 --- /dev/null +++ b/packages/ses/test/sham-non-trapping/non-trapping-shim.test.js @@ -0,0 +1,29 @@ +// Uses 'ava' rather than @endo/ses-ava to avoid worries about cyclic +// dependencies. We will need similar tests is higher level packages, in order +// to test compat with ses and ses-ava. +import test from 'ava'; +import '../../src/sham-non-trapping/non-trapping-shim.js'; + +const { freeze, isFrozen } = Object; + +test('non-trapping-pony', async t => { + const specimen = { foo: 8 }; + + const sillyHandler = freeze({ + get(target, prop, receiver) { + return [target, prop, receiver]; + }, + }); + + const safeProxy = new Proxy(specimen, sillyHandler); + + t.false(Reflect.isNonTrapping(specimen)); + t.false(isFrozen(specimen)); + t.deepEqual(safeProxy.foo, [specimen, 'foo', safeProxy]); + + t.true(Reflect.suppressTrapping(specimen)); + + t.true(Reflect.isNonTrapping(specimen)); + t.true(isFrozen(specimen)); + t.deepEqual(safeProxy.foo, 8); +}); From e3327b8b37f16e30c85043e0af28b8d72ac54368 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Sat, 4 Jan 2025 12:55:56 -0800 Subject: [PATCH 4/4] sham of non-trapping for cross-repo testing --- packages/marshal/src/encodeToCapData.js | 2 ++ packages/marshal/src/types.js | 2 ++ packages/pass-style/src/symbol.js | 3 ++- packages/ses/src/error/assert.js | 5 ++++- packages/ses/src/make-hardener.js | 4 +++- 5 files changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/marshal/src/encodeToCapData.js b/packages/marshal/src/encodeToCapData.js index 052b5795a1..945207922d 100644 --- a/packages/marshal/src/encodeToCapData.js +++ b/packages/marshal/src/encodeToCapData.js @@ -187,6 +187,8 @@ export const makeEncodeToCapData = (encodeOptions = {}) => { // work. If we allow sortable symbol keys, this will need to // become more interesting. const names = ownKeys(passable).sort(); + // TODO either delete or at-ts-expect-error + // @ts-ignore return fromEntries( names.map(name => [name, encodeToCapDataRecur(passable[name])]), ); diff --git a/packages/marshal/src/types.js b/packages/marshal/src/types.js index 8f73c6c936..0d6325b3b9 100644 --- a/packages/marshal/src/types.js +++ b/packages/marshal/src/types.js @@ -22,6 +22,8 @@ export {}; /** * @template T + * TODO either delete or at-ts-expect-error + * // @ts-ignore * @typedef {{ '@qclass': T }} EncodingClass */ diff --git a/packages/pass-style/src/symbol.js b/packages/pass-style/src/symbol.js index ca01808206..953063e844 100644 --- a/packages/pass-style/src/symbol.js +++ b/packages/pass-style/src/symbol.js @@ -11,7 +11,8 @@ const wellKnownSymbolNames = new Map( name => typeof name === 'string' && typeof Symbol[name] === 'symbol', ) .filter(name => { - // @ts-expect-error It doesn't know name cannot be a symbol + // TODO either delete or at-ts-expect-error + // @ts-ignore It doesn't know name cannot be a symbol !name.startsWith('@@') || Fail`Did not expect Symbol to have a symbol-valued property name starting with "@@" ${q( name, diff --git a/packages/ses/src/error/assert.js b/packages/ses/src/error/assert.js index f4a645c5b0..21844cc7e5 100644 --- a/packages/ses/src/error/assert.js +++ b/packages/ses/src/error/assert.js @@ -293,6 +293,8 @@ export const sanitizeError = error => { } = descs; const restNames = ownKeys(restDescs); + // TODO why did this start warning? + // eslint-disable-next-line @endo/restrict-comparison-operands if (restNames.length >= 1) { for (const name of restNames) { delete error[name]; @@ -305,7 +307,8 @@ export const sanitizeError = error => { ); } for (const name of ownKeys(error)) { - // @ts-expect-error TS still confused by symbols as property names + // TODO either delete or at-ts-expect-error + // @ts-ignore 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..f9ff87a4de 100644 --- a/packages/ses/src/make-hardener.js +++ b/packages/ses/src/make-hardener.js @@ -238,7 +238,9 @@ 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 + // + // TODO either delete or at-ts-expect-error + // @ts-ignore 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, []),