Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ses,pass-style): use non-trapping integrity trait for safety #2675

Draft
wants to merge 2 commits into
base: markm-no-trapping-shim
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 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, suppressTrapping } = Object;
const { ownKeys } = Reflect;

test('passStyleOf basic success cases', t => {
Expand Down Expand Up @@ -217,7 +217,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 @@ -423,7 +423,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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"@endo/non-trapping-shim": "^0.1.0"
"@endo/non-trapping-shim": "workspace:^"

This might also explain the issues you're trying to work around in #2684

},
"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';

Comment on lines +17 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda wish we could make SES and other endo packages compatible with the shim having been loaded or not, and then, we could move this from ses to @endo/init, at least until we get more clarity on where the proposal is moving.

// 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 @@ -293,7 +293,7 @@
} = descs;

const restNames = ownKeys(restDescs);
if (restNames.length >= 1) {

Check failure on line 296 in packages/ses/src/error/assert.js

View workflow job for this annotation

GitHub Actions / lint

Comparison against unknown type
for (const name of restNames) {
delete error[name];
}
Expand All @@ -305,7 +305,6 @@
);
}
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.
Comment on lines +132 to +134
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah IMO this is another reason to optionally support non-trapping, as it's currently incompatible with a 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)) {
Comment on lines 189 to +190
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we modify freezeTypedArray to make it return whether the typedArray got fully frozen ?

// 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.
Comment on lines -241 to -242
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very confused why this is no longer necessary.

// 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
Loading