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

Terminate CapTP in non-hardened Realm without SES shim #1686

Open
kriskowal opened this issue Jul 19, 2023 · 8 comments
Open

Terminate CapTP in non-hardened Realm without SES shim #1686

kriskowal opened this issue Jul 19, 2023 · 8 comments
Assignees
Labels
kriskowal-review-2024-01 Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024

Comments

@kriskowal
Copy link
Member

@danfinlay raises an issue we have struggled with: it’s currently not possible to terminate CapTP in a realm that does not also use the SES shim, because these layers depend upon harden. We have yet to find a nice compromise that allows us to write these libraries in a way that works well in both hardened and unhardened JavaScript without compromising the readability or audit-ability of that code in the hardened case. Some of the difficulty lies in the dichotomy:

  • harden walks up prototype chains, so hardening an object before lockdown would cause the repair phase to throw errors
  • harden that does not walk up the prototype chain does not provide assurance that the object is transitively hardened

However, it is clearly the responsibility of the creator of instances to harden the instance and also clearly the responsibility of a library to harden its classes, constructors, prototypes, return values, and so on. Currently, harden does both as a safeguard against a library that failed to harden itself.

So, I propose we can break this logjam with the following strategy:

  • We recreate an @endo/harden user-space implementation of harden that is a ponyfill for globalThis.harden and otherwise provides an implementation that freeze an object by walking transitive properties and not prototype chains. This is a version of harden that is suitable for use in both unhardened JavaScript and hardened JavaScript.
  • We also change globalThis.harden so that it freezes exactly the same objects as the ponyfill, and asserts that every hardened object is transitively frozen over both property and prototype, throwing an error that indicates that the hardened instance must stand on top of only hardened prototypes.

This would allow us to refactor CapTP and all its dependencies to depend on the @endo/harden ponyfill, making CapTP usable without shims, equally safe with shims, and otherwise identical verbatim to the current implementation.

@kriskowal
Copy link
Member Author

For review by @erights and @michaelfig

@mhofman
Copy link
Contributor

mhofman commented Jul 20, 2023

So there is the question of getters and setters in this split world: who is responsible for hardening them?

I believe that in most cases these are not shared with other objects, and that a "surface" harden should thus freeze them as part of the "walking transitive properties" step.

Btw in this world, it's not really the harden implementation that changes , but whether isHardened() recursively checks the prototype chain or not. In fact isHardened() could skip the prototype walk before lockdown(), but enforce it after. Taken an other way, lockdown() replaces isHardened() to add a recursive hardening check for the prototype.

@gibson042
Copy link
Contributor

gibson042 commented Jul 20, 2023

It looks to me like initialization of the marshal package and use for marshalling/unmarshalling CapData currently has three firm dependencies that are satisfied by @endo/init:

  • a global harden function (which must perform a shallow freeze but need not go further than that)
  • a global assert function with details, quote, note, typeof, fail, and Fail properties, all roughly as defined in ses/src/error/assert.js (assert should throw an exception when its argument is not truthy, assert.details should be usable as tagged template function, assert.quote should quote its argument, etc.)
  • a deconstructable global HandledPromise (e.g., even globalThis.HandledPromise = false suffices)

The first two dependencies seem relevant, although they could in principle be parameterized, and the third can be addressed by #1687.

The bar is actually rather low:

run with node --input-type=module -e '…'
// STUB DEPENDENCIES
import "data:text/javascript, globalThis.harden ||= Object.freeze;";
import "data:text/javascript, const q = JSON.stringify, { raw } = String, X = (T, ...args) => raw(T, ...args.map(v => q(v) || String(v))); const Fail = (...args) => { throw Error(X(...args)); }; const assert = (globalThis.assert ||= (ok, msg) => ok || Fail([msg])); assert.Fail ||= Fail; assert.fail ||= msg => Fail([msg]); assert.typeof ||= (v, t) => typeof v === t || Fail`not a ${t}: ${v}`; assert.details ||= X; assert.quote ||= q; assert.note ||= () => {};";
import "data:text/javascript, globalThis.HandledPromise ||= false;";

import { makeMarshal } from "@endo/marshal";
import { Far } from "@endo/pass-style";

const { defineProperties, freeze } = Object;

const slotAssignments = new Map();
const getSlotAssignment = slotAssignments.get.bind(slotAssignments);
const setSlotAssignment = slotAssignments.set.bind(slotAssignments);
let nextSlotAssignment = 10;
const convertValToSlot = obj => {
  let slot = getSlotAssignment(obj);
  if (slot === undefined) {
    setSlotAssignment(obj, slot = `id1:${nextSlotAssignment++}`);
  }
  return slot;
};
const convertSlotToVal = (id, iface = "") => Far(`${iface}<${id}>`);
const m = makeMarshal(convertValToSlot, convertSlotToVal);


// Demonstrate successful unmarshalling and marshalling.
const o = m.fromCapData({ body: `#{"bigint":"+42","remotable":"$0.Foo","repeat":"$0"}`, slots: ["id0:1"] });
console.log(o);
// => {
//   bigint: 42n,
//   remotable: Object [Alleged: Foo<id0:1>] {},
//   repeat: Object [Alleged: Foo<id0:1>] {}
// }
console.log(m.toCapData(o));
// => {
//   body: '{"bigint":{"@qclass":"bigint","digits":"42"},"remotable":{"@qclass":"slot","iface":"Alleged: Foo<id0:1>","index":0},"repeat":{"@qclass":"slot","index":0}}',
//   slots: [ 'id1:10' ]
// }

// Demonstrate unhardened environment.
Object.prototype.expando = "Object.prototype is mutable";
console.log({}.expando);
// => Object.prototype is mutable

// Demonstrate input validation.
const badError = freeze(defineProperties(Error(), { message: { value: freeze(["not", "a", "string"]) } }));
console.log(m.toCapData(freeze({ badError })));
// => Error: Passed Error "message" {"value":["not","a","string"],"writable":false,"enumerable":false,"configurable":false} must be a string-valued data property.

@michaelfig
Copy link
Member

a deconstructable global HandledPromise (e.g., even globalThis.HandledPromise = false suffices)

I'm confused as to what @endo/marshal buys us in isolation. For "terminating CapTP", all we need are functioning versions of E() and Far() and implementing their correct behaviour currently relies pretty deeply on HandledPromise, which is not so easy to stub.

I'm in wholehearted agreement that the @endo/eventual-send layers can be structured better, but I don't see any cheap way to make do in their total absence.

@kriskowal
Copy link
Member Author

Ah, yes. We can’t avoid shimming HandledPromise. I assume that’s not possible to ponyfill (is it?). I do assume that HandledPromise can be used before lockdown (can it?). If not in its current form, I’m expecting things to get easier when SES supports vetted shims.

@michaelfig
Copy link
Member

I assume that's not possible to ponyfill (is it?).

The problem with not installing HandledPromise (or its spiritual successors) somewhere in globalThis is that it really needs a few per-Realm WeakMaps to work correctly. Making it a shim is how we avoid its eval twins.

@kriskowal
Copy link
Member Author

kriskowal commented Jul 21, 2023

That’ll be even more true with CapTP when we support 3-party-handoff. That will take a dependency on a shared nonce locator, and that in turn will require WeakRef and FinalizationRegistry to track the retention of objects that were transported between sessions (if I grok right).

@mhofman
Copy link
Contributor

mhofman commented Dec 28, 2023

I have been giving more thoughts about the different harden semantics before and after lockdown. I had been uneasy about the possibility of an object to be considered hardened before lockdown, but not satisfy the prototype checks post lockdown. I think this can be remediated by a weak list of prototype objects to check during lockdown.

Assuming that harden is implemented as a flag on the object (not as a transitive frozen check), here would be the suggested semantics:

  • isHardened() checks whether the object has the flag. Nothing else.
  • harden()
    • Checks that the object is not in "CheckIsHardenedAtLockdownList", fails otherwise
    • Transitively walks and freezes the object and its properties
      • The object / function in data properties and the accessor functions are frozen, and added to a "ToHardenSet"
    • The prototype of each of these objects / functions is checked for a hardened flag. If no flag:
      • if post-lockdown, fails
      • if pre-lockdown, adds the prototype object to the "CheckIsHardenedAtLockdownList"
    • The hardened flag is applied on all entries in the "ToHardenSet"
  • lockdown() does the following:
    • transitively freezes all intrinsics (including all prototypes), and adds them to a "ToHardenSet"
    • checks that all entries in the "CheckIsHardenedAtLockdownList" are in the "ToHardenSet", fails otherwise
    • empty the "CheckIsHardenedAtLockdownList"
    • Applies the hardened flag to all entries in the "ToHardenSet"

There is a single remaining pre/post lockdown discrepancy with this scheme: pre lockdown, an object can be hardened without its prototype ever being hardened. If the prototype is hardened after the object, hardening of the prototype fails. If lockdown is called without the prototype being an intrinsic, or having been manually hardened (before the objects using it as prototype), lockdown fails. As such the possibility of code working pre lockdown but not post lockdown is reduced, and can easily be fixed by adding the harden() call for the missing prototypes (and only at the right place).

Regarding the cost of maintaining the "CheckIsHardenedAtLockdownList". For user code correctly using hardening, only the intrinsics should ever be present in this list. Even if lockdown is never called, the cost to maintain this list should be minimal. As a specification construct, the content of the list is never observable, and its entries can be garbage collected. As a shim, to avoid leaks it may have to be implemented using WeakRef and FinalizationRegistry.

A shim implementing harden() without lockdown() should probably provide a lockdown() stub that only throws. In that case the "CheckIsHardenedAtLockdownList" can be implemented as a simple WeakSet as it never needs to be iterated. Technically a lockdown+harden shim does not strictly need to iterate "CheckIsHardenedAtLockdownList" unless it wants to provide information about which prototypes were not hardened.

@kriskowal kriskowal added the kriskowal-review-2024-01 Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024 label Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kriskowal-review-2024-01 Issues that kriskowal wants to bring to the attention of the team for review as of January, 2024
Projects
None yet
Development

No branches or pull requests

5 participants