From 63142a0c5012c6d4e3522d4f88990260cf10e31b Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Thu, 3 Feb 2022 22:30:18 -0800 Subject: [PATCH] fix: remove pureCopy deleted from endo 1061 --- docs/env.md | 12 --------- packages/ERTP/docs/INPUT_VALIDATION.md | 30 +++++++-------------- packages/ERTP/src/displayInfo.js | 29 ++++++++++++++++---- packages/SwingSet/src/devices/bridge-src.js | 2 -- packages/SwingSet/src/devices/bridge.js | 2 -- packages/store/src/patterns/rankOrder.js | 24 ++++++++++++----- packages/store/test/test-store.js | 11 +++----- 7 files changed, 54 insertions(+), 56 deletions(-) diff --git a/docs/env.md b/docs/env.md index 3a6b43aac4c..52bc67d5cad 100644 --- a/docs/env.md +++ b/docs/env.md @@ -22,18 +22,6 @@ Lifetime: --> -## ALLOW_IMPLICIT_REMOTABLES - -Affects: marshal - -Purpose: to control whether the marshal system demands `Far` - -Description: set to `true` if you need to debug problems with missing `Far` -declarations. - -Lifetime: until all sources (including dapps) conform to using `Far` -declarations for all remote objects - ## CHAIN_BOOTSTRAP_VAT_CONFIG Affects: `ag-chain-cosmos`, `ag-solo` diff --git a/packages/ERTP/docs/INPUT_VALIDATION.md b/packages/ERTP/docs/INPUT_VALIDATION.md index 126bd843634..179c839b7a3 100644 --- a/packages/ERTP/docs/INPUT_VALIDATION.md +++ b/packages/ERTP/docs/INPUT_VALIDATION.md @@ -2,7 +2,7 @@ ## Requirements -We want ERTP to be robust against malicious code in the same vat, so the ERTP must be robust without the kind of checks that we get for free in intervat communication going through `@endo/marshal` and `@agoric/swingset-vat`. In other words, the only tools at our disposal are being in a Hardened JS environment, and helpers that we import and call directly, such as `passStyleOf` from `@endo/marshal`. +We want ERTP to be robust against malicious code in the same vat, so the ERTP must be robust without the kind of checks that we get for free in intervat communication going through `@endo/marshal` and `@agoric/swingset-vat`. In other words, the only tools at our disposal are being in a Hardened JS environment, and helpers that we import and call directly, such as `passStyleOf` from `@endo/marshal`. ## Malicious Behavior @@ -34,7 +34,7 @@ to the `brand` would currently require an asynchronous call on the brand.) For `coerce` to work according to spec, if the user passes in an -invalid brand or an invalid amount, the function should throw. +invalid brand or an invalid amount, the function should throw. ## Valid amounts @@ -45,7 +45,7 @@ Remotable and do not call any of these methods (we would have to call them asynchronously in order to check them, since the `brand` might be a remote object, so we do not check them.) -The value is either a bigint Nat in the case of AssetKind.NAT (we formerly allowed numbers for backwards compatibility, but now only bigints are allowed) or an array of Structures in the case of AssetKind.SET. The array of Structures might include any combination of other Structures, “undefined”, “null”, booleans, symbols, strings, pass-by-copy records, pass-by-copy arrays, numbers, bigints, Remotables, and errors. Importantly, a Structure cannot contain promises. +The value is either a bigint Nat in the case of AssetKind.NAT (we formerly allowed numbers for backwards compatibility, but now only bigints are allowed) or an array of Structures in the case of AssetKind.SET. The array of Structures might include any combination of other Structures, “undefined”, “null”, booleans, symbols, strings, pass-by-copy records, pass-by-copy arrays, numbers, bigints, Remotables, and errors. Importantly, a Structure cannot contain promises. ## Invalid Amounts @@ -82,7 +82,7 @@ Object.defineProperty(object1, 'value', { > object1.value 1000000n ``` - + `Object.freeze(object1)` is not helpful here, as the `getter` is not changing, the value that is being returned is. `harden` also does not prevent `value` from changing. ​​When harden does encounter an accessor property during its traversal, it does not "read" the property, and thereby does not call the getter. Rather, it proceeds to recursively harden the getter and setter themselves. That's because, for an accessor property, its current value isn't, for this purpose, considered part of its API surface, but rather part of the behavior of that API. Thus, we need an additional way to ensure that the value of `checked` cannot change. `passStyleOf` throws on objects with accessor properties like the @@ -92,22 +92,16 @@ change happens, `passStyleOf` would not throw in that case.) ### Is a CopyRecord and is a proxy -* **The dangers**: +* **The dangers**: 1) A proxy can throw on any property access. Any time it does *not* throw, it *must* return the same - value as before. + value as before. 2) A proxy can mount a reentrancy attack. When the property is accessed, the proxy handler is called, and the handler can reenter the code while the original is still waiting for that - property access to return. - -To mitigate the effects of proxies: -1. Create a new object with a spread operator so that the proxy isn't - used further. -2. Destructure and then never use the original object again. -3. Use `pureCopy`. `pureCopy` ensures the object is not a proxy, but `pureCopy` only works for copyRecords that are pure data, meaning no remotables and no promises. + property access to return. -We aim to address proxy-based reentrancy by other, lower-level means. -Specifically, `passStyleOf(x) === 'copyRecord'` or +We *assume* we will address proxy-based reentrancy by other, lower-level means. +Specifically, `passStyleOf(x) === 'copyRecord'` or `passStyleOf(x) === 'copyArray'` would guarantee that `x` is not a proxy, protecting against dangers #1 and #2. We should note that proxy-based reentrancy is not a threat across a vat boundary because it requires synchronous @@ -135,7 +129,7 @@ In `AmountMath.coerce(brand, allegedAmount)`, we do the following: 2. assert that the `allegedAmount` is a `copyRecord` 3. destructure the `allegedAmount` into `allegedBrand` and `allegedValue` -4. Assert that the `brand` is identical to the `allegedBrand` +4. Assert that the `brand` is identical to the `allegedBrand` 5. Call `AmountMath.make(brand, allegedValue)`, which: * Asserts that the `brand` is a remotable, again. * Asserts that the `allegedValue` is a `copyArray` or a `bigint` @@ -154,7 +148,3 @@ destructuring), or we successfully get both the `allegedBrand` and `allegedValue` and never touch the proxy again. Currently, we do not attempt to prevent proxy-based reentrancy, so defending against danger #1 is the full extent of our defense. - - - - diff --git a/packages/ERTP/src/displayInfo.js b/packages/ERTP/src/displayInfo.js index fda6dadb792..a909bd14125 100644 --- a/packages/ERTP/src/displayInfo.js +++ b/packages/ERTP/src/displayInfo.js @@ -1,7 +1,29 @@ // @ts-check import { assert, details as X, q } from '@agoric/assert'; -import { pureCopy, assertRecord } from '@endo/marshal'; +import { assertRecord, isObject, passStyleOf } from '@endo/marshal'; + +// TODO Once https://github.com/endojs/endo/pull/1061 is merged, then we +// should add `assertPure` to the imports from `@endo/marshal` and remove +// the redundant definition here. +const assertPure = (pureData, optNameOfData = 'Allegedly pure data') => { + const passStyle = passStyleOf(pureData); + switch (passStyle) { + case 'copyArray': + case 'copyRecord': + case 'tagged': { + return true; + } + default: { + if (!isObject(pureData)) { + return true; + } + assert.fail( + X`${q(optNameOfData)} ${pureData} must be pure, not a ${q(passStyle)}`, + ); + } + } +}; // TODO: assertSubset is copied from Zoe. Move this code to a location // where it can be used by ERTP and Zoe easily. Perhaps another @@ -32,11 +54,8 @@ const displayInfoKeys = harden(['decimalPlaces', 'assetKind']); export const coerceDisplayInfo = (allegedDisplayInfo, assetKind) => { // We include this check for a better error message assertRecord(allegedDisplayInfo, 'displayInfo'); + assertPure(allegedDisplayInfo, 'displayInfo'); - // `pureCopy` ensures the resulting object is not a proxy. Note that - // `pureCopy` works in this case because displayInfo is a copyRecord - // that is pure data, meaning no remotables and no promises. - allegedDisplayInfo = pureCopy(allegedDisplayInfo); if (allegedDisplayInfo.assetKind !== undefined) { assert( allegedDisplayInfo.assetKind === assetKind, diff --git a/packages/SwingSet/src/devices/bridge-src.js b/packages/SwingSet/src/devices/bridge-src.js index 3881ec6aed2..5b1dfce64c7 100644 --- a/packages/SwingSet/src/devices/bridge-src.js +++ b/packages/SwingSet/src/devices/bridge-src.js @@ -2,8 +2,6 @@ import { assert, details as X } from '@agoric/assert'; import { Far } from '@endo/marshal'; function sanitize(data) { - // TODO: Use @endo/marshal:pureCopy when it exists. - // Note: It exists. if (data === undefined) { return undefined; } diff --git a/packages/SwingSet/src/devices/bridge.js b/packages/SwingSet/src/devices/bridge.js index 4cf87f4991d..67b09a288dc 100644 --- a/packages/SwingSet/src/devices/bridge.js +++ b/packages/SwingSet/src/devices/bridge.js @@ -80,8 +80,6 @@ */ function sanitize(data) { - // TODO: use @endo/marshal:pureCopy when it exists. - // Note: It exists. if (data === undefined) { return undefined; } diff --git a/packages/store/src/patterns/rankOrder.js b/packages/store/src/patterns/rankOrder.js index 89a6c2e04e1..47366ce3969 100644 --- a/packages/store/src/patterns/rankOrder.js +++ b/packages/store/src/patterns/rankOrder.js @@ -1,17 +1,27 @@ // @ts-check import { assert, details as X, q } from '@agoric/assert'; -import { - getTag, - nameForPassableSymbol, - passStyleOf, - sameValueZero, -} from '@endo/marshal'; +import { getTag, nameForPassableSymbol, passStyleOf } from '@endo/marshal'; -const { fromEntries, entries, setPrototypeOf } = Object; +const { fromEntries, entries, setPrototypeOf, is } = Object; const { ownKeys } = Reflect; +/** + * This is the equality comparison used by JavaScript's Map and Set + * abstractions, where NaN is the same as NaN and -0 is the same as + * 0. Marshal serializes -0 as zero, so the semantics of our distributed + * object system does not distinguish 0 from -0. + * + * `sameValueZero` is the EcmaScript spec name for this equality comparison, + * but TODO we need a better name for the API. + * + * @param {any} x + * @param {any} y + * @returns {boolean} + */ +const sameValueZero = (x, y) => x === y || is(x, y); + /** * @type {[PassStyle, RankCover][]} */ diff --git a/packages/store/test/test-store.js b/packages/store/test/test-store.js index dec0d328e35..12caef6a370 100644 --- a/packages/store/test/test-store.js +++ b/packages/store/test/test-store.js @@ -3,7 +3,7 @@ // eslint-disable-next-line import/no-extraneous-dependencies import { test } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js'; -import { ALLOW_IMPLICIT_REMOTABLES, Far, passStyleOf } from '@endo/marshal'; +import { Far, passStyleOf } from '@endo/marshal'; import { makeLegacyMap } from '../src/legacy/legacyMap.js'; import { makeLegacyWeakMap } from '../src/legacy/legacyWeakMap.js'; import { makeScalarMapStore } from '../src/stores/scalarMapStore.js'; @@ -121,13 +121,8 @@ test('reject promise keys', t => { test('passability of stores', t => { t.is(passStyleOf(makeScalarMapStore('foo')), 'remotable'); t.is(passStyleOf(makeScalarWeakMapStore('foo')), 'remotable'); - if (ALLOW_IMPLICIT_REMOTABLES) { - t.is(passStyleOf(makeLegacyMap('foo')), 'remotable'); - t.is(passStyleOf(makeLegacyWeakMap('foo')), 'remotable'); - } else { - t.throws(() => passStyleOf(makeLegacyMap('foo')), { message: /x/ }); - t.throws(() => passStyleOf(makeLegacyWeakMap('foo')), { message: /x/ }); - } + t.throws(() => passStyleOf(makeLegacyMap('foo')), { message: /x/ }); + t.throws(() => passStyleOf(makeLegacyWeakMap('foo')), { message: /x/ }); }); test('passability of store iters', t => {