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

Stamp proxies to prevent reentrancy / interleaving #3905

Open
mhofman opened this issue Sep 28, 2021 · 5 comments
Open

Stamp proxies to prevent reentrancy / interleaving #3905

mhofman opened this issue Sep 28, 2021 · 5 comments
Assignees
Labels
audit-zestival Vulnerability assessment of ERTP + Zoe endo enhancement New feature or request security SwingSet package: SwingSet tc39 Related to TC39 standards work

Comments

@mhofman
Copy link
Member

mhofman commented Sep 28, 2021

What is the Problem Being Solved?

Untrusted code executed in a liveslots Compartment may create a proxy for a hardened object, which would behave like a hardened object (thanks to the language invariants), but be able to observe when properties are being accessed. That would allow attacker code to interleave at that point which could perform reentrancy type attacks.

This is a problem only for intra-vat code, with potential attack surface in at least 2 cases:

  • Liveslot to vat. E.g. marshal's serializer is used in trusted vat code, such as the virtual object manager which doesn't expect an untrusted interleaving point when handling a user provided object.
  • ZCF to contract. passStyleOf of copyRecord or copyArray objects may share a reference within a vat, and traversing such objects would not be expected to cause interleaving.

Description of the Design

The idea is to be able to identify proxies where plain objects are expected while allowing proxies to be used normally in user code. This can be accomplished by overriding the Proxy constructor in the liveslots compartment with a version that stamps the instances it creates (e.g. adding them to a WeakSet), and creating a power that checks if a given object is in that set.

The original Proxy constructor needs to be completely denied, which can be accomplished by the inescapableGlobalProperties of liveSlots. In the future such propagated globals may become a built-in feature of the Compartment API.

In the ZCF to contract case, it all executes within the liveSlot compartment, which would thus need the proxy stamp checking power. Since the power to detect any proxy may be considered sensitive and denied to non privileged code, we may need to restrict the power to "privileged" code running inside the compartment. For example only the @agoric/marshal module, and in particular its passStyleOf helper may need this power.

Two high level options may be possible to restrict a power inside a Compartment:

  • pass the power as a globalLexical and deny it from unprivileged code. The problem is the denial part since it's not possible to differentiate privileged from unprivileged code inside a bundle. One workaround might be a static checker which allows exactly a single lexical use of the power, then verify that the bundle includes the module which is supposed to exercise the power.
  • Have privileged modules marked as exit points in the bundle, instantiate the privileged modules separately with the power, and provide the module instance to the loading hooks. This wouldn't be able to use any definition of pure modules as by definition the module instance would be impure when it checks with the power.
    • A variation of this approach is to load the privileged modules as a bootstrap bundle, initialize the privileged modules (either implicit init which grabs the sensitive powers from the global, or explicit init where the bootstrap installs an init function accepting powers on the global) which leaves its initialized features on the global, then load the unprivileged bundle which can now use privileged features without having access to the raw powers itself.

Security Considerations

Escaping the stamping Proxy constructor would allow a complete bypass of the mechanism this is proposing, so the global must be inescapable.

The power to read a proxy stamp may be sensitive and denied to unprivileged code.

Test Plan

TBD, but verify that user constructed proxies can be identified in the different locations where we believe they could cause the most harm.

@mhofman
Copy link
Member Author

mhofman commented Aug 15, 2023

Another motivation: robust console.log code which avoids executing any user code. See #8191

@erights
Copy link
Member

erights commented Aug 15, 2023

How?

@erights
Copy link
Member

erights commented Aug 15, 2023

IIUC, to avoid executing any user code during console.log, console.log would first need to brand check for non-proxy before interacting with the argument, or anything it reaches from the argument. Once brand-checked to be a non-proxy, it would then need to get property descriptors to avoid triggering getters.

Note that passStyleOf itself cannot be used to pre-filter no-proxy Passables, because passStyleOf itself may trigger user code via proxies or getters.

Is this what you have in mind?

@erights
Copy link
Member

erights commented Aug 15, 2023

(In my head, I just started hearing the Wizard of Oz movie scarecrow singing "If I only had a write barrier". Ironically, the meter doesn't quite work.)

@mhofman
Copy link
Member Author

mhofman commented Aug 15, 2023

Correct, object-inspect.js which is backing our console.log would need to do that dance, as suggested by @gibson042. Both object-inspect.js and passStyleOf need an IsProxy predicate to accomplish this.

The interesting bit is the layering between ses-shim / lockdown, object-inspect.js, liveslots, and the replacement of the Proxy constructor with a stamping one. We'll likely need this replacement to take the form of a vetted shim loaded before hardenIntrinsics() in the lockdown bundle, and with the proxy recognition power endowed to object-inspect.js and liveslots (which can then further delegate it to makePassStyleOf, before leaving that shared passStyleOf behind on something like VatData).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-zestival Vulnerability assessment of ERTP + Zoe endo enhancement New feature or request security SwingSet package: SwingSet tc39 Related to TC39 standards work
Projects
None yet
Development

No branches or pull requests

5 participants