From c22476f4209513a07b73503cc07fda15d2799676 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Mon, 22 Aug 2022 21:11:58 -0700 Subject: [PATCH] fix: prepare for far classes (#1251) --- packages/marshal/src/helpers/remotable.js | 37 +++- packages/marshal/test/test-passStyleOf.js | 212 +++++++++++++++++++--- 2 files changed, 212 insertions(+), 37 deletions(-) diff --git a/packages/marshal/src/helpers/remotable.js b/packages/marshal/src/helpers/remotable.js index c19adca1c3..f1f59da6ff 100644 --- a/packages/marshal/src/helpers/remotable.js +++ b/packages/marshal/src/helpers/remotable.js @@ -68,15 +68,34 @@ harden(assertIface); * @returns {boolean} */ const checkRemotableProtoOf = (original, check = x => x) => { - /** - * TODO: It would be nice to typedef this shape, but we can't declare a type - * with PASS_STYLE from JSDoc. - * - * @type {{ [PASS_STYLE]: string, - * [Symbol.toStringTag]: string, - * }} - */ + // A valid remotable object must inherit from a "tag record" -- a + // plain-object prototype consisting of only + // a suitable `PASS_STYLE` property and a suitable `Symbol.toStringTag` + // property. The remotable could inherit directly from such a tag record, or + // it could inherit from another valid remotable, that therefore itself + // inherits directly or indirectly from such a tag record. + // + // TODO: It would be nice to typedef this shape, but we can't declare a type + // with PASS_STYLE from JSDoc. + // + // @type {{ [PASS_STYLE]: string, + // [Symbol.toStringTag]: string, + // }} + // const proto = getPrototypeOf(original); + const protoProto = proto === null ? null : getPrototypeOf(proto); + if ( + typeof original === 'object' && + proto !== objectPrototype && + protoProto !== objectPrototype && + protoProto !== null + ) { + return ( + // eslint-disable-next-line no-use-before-define + RemotableHelper.canBeValid(proto, check) && checkRemotable(proto, check) + ); + } + if ( !( check( @@ -95,8 +114,6 @@ const checkRemotableProtoOf = (original, check = x => x) => { return false; } - const protoProto = getPrototypeOf(proto); - if (typeof original === 'object') { if ( !check( diff --git a/packages/marshal/test/test-passStyleOf.js b/packages/marshal/test/test-passStyleOf.js index 802b043ebe..6efbb8576a 100644 --- a/packages/marshal/test/test-passStyleOf.js +++ b/packages/marshal/test/test-passStyleOf.js @@ -1,9 +1,12 @@ +/* eslint-disable max-classes-per-file */ import { test } from './prepare-test-env-ava.js'; import { passStyleOf } from '../src/passStyleOf.js'; import { Far } from '../src/make-far.js'; import { makeTagged } from '../src/makeTagged.js'; import { PASS_STYLE } from '../src/helpers/passStyle-helpers.js'; +const { getPrototypeOf } = Object; + test('passStyleOf basic success cases', t => { // Test in same order as `passStyleOf` for easier maintenance. // Remotables tested separately below. @@ -63,23 +66,32 @@ test('some passStyleOf rejections', t => { }); }); +/** + * For testing purposes, makes a TagRecond-like object with + * non-enumerable PASS_STYLE and Symbol.toStringTag properties. + * A valid Remotable must inherit from a valid TagRecord + * + * @param {string} [tag] + * @param {object|null} [proto] + */ +const makeTagishRecord = (tag = 'Remotable', proto = undefined) => { + return Object.create(proto === undefined ? Object.prototype : proto, { + [PASS_STYLE]: { value: 'remotable' }, + [Symbol.toStringTag]: { value: tag }, + }); +}; + test('passStyleOf testing remotables', t => { t.is(passStyleOf(Far('foo', {})), 'remotable'); t.is(passStyleOf(Far('foo', () => 'far function')), 'remotable'); - const tagRecord1 = Object.create(Object.prototype, { - [PASS_STYLE]: { value: 'remotable' }, - [Symbol.toStringTag]: { value: 'Alleged: manually constructed' }, - }); + const tagRecord1 = makeTagishRecord('Alleged: manually constructed'); const farObj1 = harden({ __proto__: tagRecord1, }); t.is(passStyleOf(farObj1), 'remotable'); - const tagRecord2 = Object.create(Object.prototype, { - [PASS_STYLE]: { value: 'remotable' }, - [Symbol.toStringTag]: { value: 'Alleged: tagRecord not hardened' }, - }); + const tagRecord2 = makeTagishRecord('Alleged: tagRecord not hardened'); const farObj2 = Object.freeze({ __proto__: tagRecord2, }); @@ -88,29 +100,20 @@ test('passStyleOf testing remotables', t => { }); const tagRecord3 = Object.freeze( - Object.create(Object.prototype, { - [PASS_STYLE]: { value: 'remotable' }, - [Symbol.toStringTag]: { value: 'Alleged: both manually frozen' }, - }), + makeTagishRecord('Alleged: both manually frozen'), ); const farObj3 = Object.freeze({ __proto__: tagRecord3, }); t.is(passStyleOf(farObj3), 'remotable'); - const tagRecord4 = Object.create(Object.prototype, { - [PASS_STYLE]: { value: 'remotable' }, - [Symbol.toStringTag]: { value: 'Remotable' }, - }); + const tagRecord4 = makeTagishRecord('Remotable'); const farObj4 = harden({ __proto__: tagRecord4, }); t.is(passStyleOf(farObj4), 'remotable'); - const tagRecord5 = Object.create(Object.prototype, { - [PASS_STYLE]: { value: 'remotable' }, - [Symbol.toStringTag]: { value: 'Not alleging' }, - }); + const tagRecord5 = makeTagishRecord('Not alleging'); const farObj5 = harden({ __proto__: tagRecord5, }); @@ -118,18 +121,173 @@ test('passStyleOf testing remotables', t => { message: /For now, iface "Not alleging" must be "Remotable" or begin with "Alleged: "; unimplemented/, }); - // We need this to succeed to enable far classes - const tagRecord6 = Object.create(Object.prototype, { - [PASS_STYLE]: { value: 'remotable' }, - [Symbol.toStringTag]: { value: 'Alleged: manually constructed' }, - }); + const tagRecord6 = makeTagishRecord('Alleged: manually constructed'); const farObjProto6 = harden({ __proto__: tagRecord6, }); const farObj6 = harden({ __proto__: farObjProto6, }); - t.throws(() => passStyleOf(farObj6), { - message: /"\[Symbol\(passStyle\)\]" property expected: "\[Alleged: manually constructed\]"/, + t.is(passStyleOf(farObj6), 'remotable'); + + // Our current agoric-sdk plans for far classes are to create a class-like + // abstraction, but not to actually use the JavaScript class syntax. + // Nevertheless, the representation passStyleOf recognizes is flexible + // enough to allow certain stylized use of syntactic classes. + class FarBaseClass7 { + #x; + + constructor(x) { + this.#x = x; + harden(this); + } + + add(y) { + return this.#x + y; + } + } + const farBaseProto7 = FarBaseClass7.prototype; + t.is(getPrototypeOf(farBaseProto7), Object.prototype); + Far('FarType7', farBaseProto7); + const farTagRecord7 = getPrototypeOf(farBaseProto7); + t.is(farTagRecord7[PASS_STYLE], 'remotable'); + t.is(getPrototypeOf(farTagRecord7), Object.prototype); + const farObj7 = new FarBaseClass7(3); + t.is(passStyleOf(farObj7), 'remotable'); + t.is(farObj7.add(7), 10); + t.is(`${farObj7}`, '[object Alleged: FarType7]'); + + class FarSubclass8 extends FarBaseClass7 { + twice() { + return this.add(4) + this.add(4); + } + } + const farObj8 = new FarSubclass8(3); + t.is(passStyleOf(farObj8), 'remotable'); + t.is(farObj8.twice(), 14); + + class NonFarBaseClass9 {} + class Subclass9 extends NonFarBaseClass9 {} + t.throws(() => Far('FarType9', Subclass9.prototype), { + message: 'For now, remotables cannot inherit from anything unusual, in {}', + }); + + const tagRecordA1 = Object.create(null, { + [PASS_STYLE]: { value: 'remotable' }, + [Symbol.toStringTag]: { value: 'Alleged: null grandproto is fine' }, + }); + const farObjProtoA1 = harden({ + __proto__: tagRecordA1, + }); + const farObjA1 = harden({ + __proto__: farObjProtoA1, + }); + t.is(passStyleOf(farObjA1), 'remotable'); + + const tagRecordA2 = Object.create(null, { + [PASS_STYLE]: { value: 'remotable' }, + [Symbol.toStringTag]: { value: 'Alleged: null grandproto is fine' }, + }); + const farObjA2 = harden({ + __proto__: tagRecordA2, + }); + t.is(passStyleOf(farObjA2), 'remotable'); + + const tagRecordA3 = makeTagishRecord( + 'Alleged: null grandproto is fine', + null, + ); + const farObjA3 = harden({ + __proto__: tagRecordA3, + }); + t.is(passStyleOf(farObjA3), 'remotable'); + + t.throws(() => passStyleOf(Object.prototype), { + message: 'cannot serialize Remotables with accessors like "toString" in {}', + }); + + const fauxTagRecordB = makeTagishRecord('Alleged: manually constructed', {}); + 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]"', + }); + + passStyleOf(harden({ __proto__: Object.prototype }), 'copyRecord'); + + const farObjC = harden({ + __proto__: Object.prototype, + method() { + return 'foo'; + }, + }); + t.throws(() => passStyleOf(farObjC), { + message: + 'Remotables must be explicitly declared: {"method":"[Function method]"}', + }); +}); + +test('remotables - safety from the gibson042 attack', t => { + // Tests the attack explained at + // https://github.com/endojs/endo/pull/1251#pullrequestreview-1077936894 + const nonEnumerable = { + configurable: true, + writable: true, + enumerable: false, + }; + const mercurialProto = new Proxy( + Object.defineProperties( + {}, + { + [PASS_STYLE]: { ...nonEnumerable, value: 'remotable' }, + [Symbol.toStringTag]: { ...nonEnumerable, value: 'Remotable' }, + }, + ), + { + getPrototypeOf(obj) { + // Self-mutate after returning the original prototype one time + // (to checkRemotableProtoOf). + if (obj[PASS_STYLE] !== 'error') { + obj[PASS_STYLE] = 'error'; + return Object.prototype; + } + return Error.prototype; + }, + }, + ); + + const makeInput = () => Object.freeze({ __proto__: mercurialProto }); + const input1 = makeInput(); + const input2 = makeInput(); + + // Further original attack text in comments. The attacks depends on + // `passStyleOf` succeeding on `input1`. Since `passStyleOf` now throws, + // that seems to stop the attack: + // console.log('# passStyleOf(input1)'); + // console.log(passStyleOf(input1)); // => "remotable" + t.throws(() => passStyleOf(input1), { + message: 'A tagRecord must be frozen: "[undefined: undefined]"', + }); + + // same because of passStyleMemo WeakMap + // console.log(`# passStyleOf(input1) again (cached "Purely for performance")`); + // console.log(passStyleOf(input1)); // => "remotable" + t.throws(() => passStyleOf(input1), { + message: + 'Errors must inherit from an error class .prototype "[undefined: undefined]"', + }); + + // different because of changes in the prototype + // Error (Errors must inherit from an error class .prototype) + // console.log('# passStyleOf(input2)'); + // console.log(passStyleOf(input2)); // => Error (Errors must inherit from an error class .prototype) + t.throws(() => passStyleOf(input2), { + message: + 'Errors must inherit from an error class .prototype "[undefined: undefined]"', }); });