Skip to content

Commit

Permalink
feat(ses,pass-style): use non-trapping integrity level for safety
Browse files Browse the repository at this point in the history
  • Loading branch information
erights committed Jan 3, 2025
1 parent d4c5cfb commit 60157c5
Show file tree
Hide file tree
Showing 13 changed files with 87 additions and 33 deletions.
6 changes: 3 additions & 3 deletions packages/captp/src/captp.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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}
*/
Expand Down
7 changes: 4 additions & 3 deletions packages/marshal/src/encodeToCapData.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ const {
is,
entries,
fromEntries,
freeze,
// @ts-expect-error TS doesn't see this on ObjectConstructor
suppressTrapping,
} = Object;

/**
Expand Down Expand Up @@ -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;
}
Expand Down
7 changes: 6 additions & 1 deletion packages/pass-style/src/passStyle-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
35 changes: 24 additions & 11 deletions packages/pass-style/src/passStyleOf.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,13 @@ import { assertPassableString } from './string.js';
/** @typedef {Exclude<PassStyle, PrimitiveStyle | "promise">} 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
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
5 changes: 3 additions & 2 deletions packages/pass-style/src/remotable.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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}`
);
Expand Down
11 changes: 8 additions & 3 deletions packages/pass-style/src/safe-promise.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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(
Expand Down
6 changes: 3 additions & 3 deletions packages/pass-style/test/passStyleOf.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Check failure on line 16 in packages/pass-style/test/passStyleOf.test.js

View workflow job for this annotation

GitHub Actions / lint

'freeze' is assigned a value but never used. Allowed unused vars must match /^_/u
const { ownKeys } = Reflect;

test('passStyleOf basic success cases', t => {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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();

Expand Down
3 changes: 2 additions & 1 deletion packages/ses/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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:^",
Expand Down
10 changes: 10 additions & 0 deletions packages/ses/src/commons.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion packages/ses/src/error/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down
20 changes: 16 additions & 4 deletions packages/ses/src/make-hardener.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import {
apply,
arrayForEach,
defineProperty,
freeze,
getOwnPropertyDescriptor,
getOwnPropertyDescriptors,
getPrototypeOf,
Expand All @@ -49,6 +48,8 @@ import {
FERAL_STACK_GETTER,
FERAL_STACK_SETTER,
isError,
isFrozen,
suppressTrapping,
} from './commons.js';
import { assert } from './error/assert.js';

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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, []),
});
Expand Down
6 changes: 6 additions & 0 deletions packages/ses/src/permits.js
Original file line number Diff line number Diff line change
Expand Up @@ -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%': {
Expand Down Expand Up @@ -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: {
Expand Down
3 changes: 2 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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"
Expand Down

0 comments on commit 60157c5

Please sign in to comment.