GC sensing possible with custom WeakMap/Set

[mhofman]

Describe the bug

The current approach to avoid contract code from sensing if the representative of a Virtual Object was garbage collected and transparently re-created (https://github.com/Agoric/agoric-sdk/issues/2724) is to replace the WeakMap and WeakSet available in the contract's compartment by a vref aware version (https://github.com/Agoric/agoric-sdk/issues/2993).

However nothing prevents user code from rolling out their own WeakMap/WeakSet implementation based on class private fields and a return override, akin to the AltWeakMapSet that was once proposed to work around the WeakMap performance issues of XS (https://github.com/endojs/endo/pull/703).

To Reproduce

Conceptual for now, but I can build a POC if needed

Expected behavior

Contract code unable to sense GC.

Platform Environment

N/A

Additional context

The object being frozen does not prevent private fields from being attached, so that is not a possible avenue for mitigating this.

The only options I can think of right now are:

• custom JS engine option to disallowing the ability to do a return override
  • This would only work until private declarations make it further in the spec
• a new integrity "level" preventing attachment of private fields
• adding a compartment transform to completely deny private fields usage

[erights]

Even though it is an information leak, I labeled it with security-availability rather than security-confidentiality because the threat here is breaking consensus by acting differently on different validators, based only on changes in spontaneous GC schedule. This could be caused, for example, by different memory budgets. Breaking consensus prevents progress, i.e., availability.

Although no state was directly corrupted, I am also labeling it security-integrity, because correct code may operate incorrectly when it uses return override to add private fields to VDOs (virtual/durable objects).

[erights]

The coupling here is interesting. Naively this should not have been possible because

• WeakMap and WeakSet were carefully designed so they do not reveal the non-determinacy of GC. By themselves, they don’t.
• WeakRef and FinalizationRegistry do, but liveSlots has been carefully designed to reserve these only for its own use; to deny them to the code running under the supervision of each instance of liveslots (where each instance is a separate vat).
• And liveSlots has been carefully designed to use WeakRef and FinalizationRegistry in a constrained manner, so that its observation of GC is not observable by the code running under each instance of liveSlots.

It is only this last step which falls into this trap, where the consequences of liveSlots observing GC inadvertently bled through into observability by the code running under supervision by that liveSlots instance.

I don’t see any way to repair this without either

• A virtual machine change (which would need a corresponding spec change :disappointed: )
• An invasive syntactic restriction preventing return override. Any syntactic restriction adequate for safety is also likely to break existing code that would otherwise be compat with Hardened JS. I don’t have a sense for how much code that is.
• Requiring all validators to run the same virtual machine with the same memory budget, etc, such that the full GC behavior is deterministically reproduced across validators, and is therefore in consensus. That would fix the availability issue, but not the integrity issue.

Fortunately, this is not relevant to MN1. It is relevant to MN3.

[mhofman]

Regarding a possible VM change on the private fields side, I've been trying to think about what that could look like. I've also been reading discussions around similar concerns when attaching private fields to the WindowProxy.

Conceptually there is no difference between attaching a private field to an object, and using that object as a WeakMap key. Any mechanism that prevents private fields attachment should similarly prevent addition to Weak collections. WeakRef / FinalizationRegistry may be sufficiently orthogonal to not participate in the same mechanism, but that seems improbable.

The fact we deny the WeakMap/WeakSet intrinsics to the contract's Compartment does not negate that liveslots needs the ability to add VDO and Presence representatives to a native WeakMap, as well as WeakRef and FinalizationRegistry. Thus any mechanism we come up with must not be absolute. One approach may be to have a temporal aspect to the mechanism: you can add the object to Weak collections or WeakRef/FR until it is "flagged as unweakable".

At first I imagined the "unweakable" mechanism could be a flag to Object.seal, but that wouldn't be compatible with the probable usage in the Web platform for WindowProxy and Location as these objects need to stay extensible. Similarly, these web objects should be usable with weak intrinsics, so throwing on adding to weak collections for example is not an option either for those. While in the web case, these are effectively roots of the object graph and thus their collection never observable, that's not the case for VDO and Presence representatives. I don't see a way to reconcile our requirements with any potential WindowProxy requirements.

It's also unclear whether the stage 1 private declarations proposal would enable adding private fields to objects after their creation without using the return override trick. I opened an issue to clarify: https://github.com/tc39/proposal-private-declarations/issues/12

[mhofman]

Maybe an alternative would be to somehow have the ability to flag objects so they cannot be used in a return override construction?

[warner]

For folks (like me) trying to understand the mechanics around this, I found https://hacks.mozilla.org/2021/06/implementing-private-fields-for-javascript/ to be a useful primer on private fields and the way a constructor can "override" the object being created by returning a value (instead of omitting the return statement as usual).

[mhofman]

Maybe an alternative would be to somehow have the ability to flag objects so they cannot be used in a return override construction?

An rough idea of how to accomplish this would be:

• Add a new slot on ordinary objects [[AllowConstructResult]] which is initially true.
• Add Object.preventConstructExtensions(obj) which sets the [[AllowConstructResult]] slot to false.
• Modify Step 10.a of Ordinary Object [[Construct]] to check for result.[[Value]].[[AllowConstructResult]], and throw a TypeError if false
• Add a Step 11 to Proxy Object [[Construct]] to check for newObj.[[AllowConstructResult]], and throw a TypeError if false
  • This one is potentially problematic if the target constructor calls Object.preventConstructExtensions(this), as the proxy would no longer be able to pass through construction. We could work around this issue by preventing constructors from doing that in the first place by adding a similar check of thisBinding.[[AllowConstructResult]] in the Ordinary Object [[Construct]] (Step 14), but that would prevent classes from "self sealing" in this manner.

[mhofman]

For folks (like me) trying to understand the mechanics around this, I found https://hacks.mozilla.org/2021/06/implementing-private-fields-for-javascript/ to be a useful primer on private fields and the way a constructor can "override" the object being created by returning a value (instead of omitting the return statement as usual).

Neat! The linked article on the origins of the return override is also enlightening: https://www.mgaudet.ca/technical/2020/7/24/investigating-the-return-behaviour-of-js-constructors

[FUDCo]

There's an important piece of this story that I'm completely failing to grok from the various comments and links folks have given, namely: how does this particular combination of features or misfeatures enable the creation of a weak-keyed collection in user space? Because I'm just not seeing it.

[mhofman]

I think the most descriptive is the actual implementation: https://github.com/endojs/endo/blob/9600194abc53bf28ca877f9b8eeb6ef73c52b214/packages/make-hardener/src/AltWeakMapSet.js

[FUDCo]

That is whacky. How is this not just considered a spec bug?

[mhofman]

Because unfortunately return override was added on purpose for backwards compatibility in the ES6 days. And its implications on private fields must have been considered since there are spec algorithms that explicitly check for privates being already present when installing them, which can only happen in the return override case.

What this means is you can create WeakMap like semantics out of regular class and private fields syntax. Aka the spec made WeakMap semantics undeniable. I would love to plug the return override hole, but I don't know how doable that will be.

[mhofman]

If we wanted to tackle this by preventing specifically private fields addition, I think the following approach would work:

• Extend Object.create() to accept an options bag. One such option would privateSealed
  • This would set a slot [[PrivateSealed]] to true on the newly created object
• Change PrivateMethodOrAccessorAdd to check for that the target object O does not have a [[PrivateSealed]] slot with a true value.

const facet = Object.create(
  Object.prototype,
  boundBehaviorPropertyDescriptors,
  {
    privateSealed: true,
  },
);

The advantage of this approach is that it doesn't create the ability for code to prevent extensions through the return override trick on 3rd party objects, only the creator can decide that.

It's not clear if a similar concept could apply to classes. It feels like the conceptual equivalent would be a final class modifier that prevents derived classes, which is arguably more far reaching than simply sealing the privates.

An options bag might also allow private declarations to be set on a regular object at construction without requiring the object literal syntax (as long as there is some kind of reification allowing expression usage) :

private #x as x;
const obj = Object.create(null, {}, { privateEntries: [x, 'privateValue'] });
assert(obj.#x === 'privateValue');

And finally it would allow an "integrity level caching" creation option as well.

[mhofman]

This came up on the html side with people stamping the window proxy object: https://github.com/whatwg/html/issues/7952