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

feat(ses,pass-style): use non-trapping integrity trait for safety #2675

Open
wants to merge 4 commits into
base: markm-no-trapping-shim
Choose a base branch
from

Conversation

erights
Copy link
Contributor

@erights erights commented Dec 31, 2024

Staged on #2673

Closes: #XXXX
Refs: #XXXX

Description

Use of the non-trapping integrity level, as provided by the ponyfill and shim at #2673 , for additional safety of both the ses-shim and @endo/pass-style.

Security Considerations

The point. By having passStyleOf ensure that copyData (copyList, copyRecord, tagged) is non-trapping, we enable programming patterns that check this pass-style early on suspect parameters. Once those checks pass, we now know we can operate on such copy-data within the function body without the possibility that these operations cause interleaving with foreign code, and thus without vulnerability to reentrancy hazards or attacks.

Scaling Considerations

In addition to those documented at #2673:

  • Until we get the native harden of XS to do suppressTrapping at each step, we need to remove the shortcut to use the native harden if there is one. This will be expensive. Likely too expensive to use in production.
    • Update: We now make the harden bypass conditional on whether the opt-in shim has been enabled. By default, it is disabled, avoiding the cost for the default case.
  • In fact, avoiding the native hardener is so expensive that for this PR to proceed at all, the main changes will need to be switched by a new config option, likely starting with a default of off.
    • Update: The shim has indeed been made opt-in
  • fix(ses): dont use native harden #2677 doesn't use the native harden, but its CI shows a bunch of resulting errors to be understood and fixed.

Documentation Considerations

Other than those documented at #2673, none

Testing Considerations

  • Because our checks for acyclic package dependencies does not distinguish between dependencies and devDependencies, if we want to continue to use ses-ava to test @endo/non-trapping-shim, but also have ses depend on @endo/non-trapping-shim for enhancing harden, we will need to move the @endo/non-trapping-shim tests somewhere else. Or find another way to avoid the package-dependency-cycle diagnostic.

    • Update: Avoiding this problem rather than fixing it. The shim now just uses plain ava.
  • We should also write tests that fail with the prior freeze-only harden behavior, especially tests that demonstrate reentrancy attacks, and see that those test newly pass with this PR.

  • Because this PR's CI is for testing in the shim's default opt-out state, we need to do something else to run all these CI tests when the shim is opted into. We use DO NOT MERGE flip non trapping shim default #2699 as a temporary placeholder to do so. It flips the shim to opt-out, so that it defaults to 'enabled'. If CI passes in that PR, we take that as confirmation that this PR is compat with both states. However, this duality is not itself something we can automatically retest for future PRs.

Compatibility Considerations

In addition to those documented at #2673:

Proxies on frozen almost-empty targets often still do useful work in their trap handlers. This is because get and has, for example, if about non-own property names, are still general traps. In addition, apply can ignore the call behavior of its target and just do its own thing. For these cases, we need to be careful not to harden the target or proxy. We need to find and convert these cases to explicitly freeze.

Going the other way, some tests would freeze some inputs to passStyleOf, depending on these freeze calls to make the known-input-structure hardened, or hardened enough. But now that passStyleOf requires non-trapping, this should usually be changed to harden.

Upgrade Considerations

Other than those documented at #2673, none

@erights erights self-assigned this Dec 31, 2024
@erights erights force-pushed the markm-use-no-trapping-shim branch from c0354eb to 6bbdc1a Compare December 31, 2024 22:54
@erights erights force-pushed the markm-no-trapping-shim branch from aed8d00 to cb1e1f6 Compare December 31, 2024 23:03
@erights erights force-pushed the markm-use-no-trapping-shim branch from 6bbdc1a to dcf739c Compare December 31, 2024 23:04
@erights erights force-pushed the markm-no-trapping-shim branch from cb1e1f6 to 5c38d8c Compare December 31, 2024 23:11
@erights erights force-pushed the markm-use-no-trapping-shim branch from dcf739c to 128ef21 Compare December 31, 2024 23:11
@erights erights force-pushed the markm-no-trapping-shim branch from 98e6396 to f7d527c Compare January 1, 2025 00:05
@erights erights force-pushed the markm-use-no-trapping-shim branch 2 times, most recently from 7f44397 to dde0022 Compare January 1, 2025 01:26
@erights erights force-pushed the markm-use-no-trapping-shim branch 7 times, most recently from 49d9dfb to 5bc9589 Compare January 1, 2025 03:09
@erights erights marked this pull request as ready for review January 1, 2025 03:31
@erights erights requested a review from michaelfig January 1, 2025 03:57
@erights
Copy link
Contributor Author

erights commented Jan 1, 2025

@michaelfig Adding you as an additional reviewer. Please look at least at the changes to eventual-send and captp. Thanks!

@erights erights force-pushed the markm-use-no-trapping-shim branch from c0d1ae2 to 7c0edb8 Compare January 1, 2025 04:29
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your changes to eventual-send and captp LGTM.

Is it too late to change all occurrences of "no-trapping" to "non-trapping"? That would be more consistent with other terms in the integrity levels, like "non-frozen".

packages/captp/src/captp.js Outdated Show resolved Hide resolved
packages/captp/src/captp.js Outdated Show resolved Hide resolved
packages/eventual-send/src/E.js Outdated Show resolved Hide resolved
@erights erights changed the title feat(ses,pass-style): use no-trapping integrity level for safety feat(ses,pass-style): use non-trapping integrity trait for safety Jan 2, 2025
@erights erights force-pushed the markm-no-trapping-shim branch from f7d527c to 29223ef Compare January 2, 2025 21:41
@erights erights force-pushed the markm-no-trapping-shim branch from 85d55f0 to 0c6aa83 Compare January 2, 2025 23:28
@erights erights force-pushed the markm-use-no-trapping-shim branch 2 times, most recently from 78c5b8b to 6d3c6f2 Compare January 20, 2025 23:39
@erights erights force-pushed the markm-no-trapping-shim branch from afe438b to 7ab2643 Compare January 20, 2025 23:44
@erights erights force-pushed the markm-use-no-trapping-shim branch from 6d3c6f2 to bb949c1 Compare January 20, 2025 23:46
@erights erights force-pushed the markm-no-trapping-shim branch from 7ab2643 to 9b20c1b Compare January 21, 2025 00:01
@erights erights force-pushed the markm-use-no-trapping-shim branch from bb949c1 to 103c1bb Compare January 21, 2025 00:02
@erights erights force-pushed the markm-no-trapping-shim branch from 9b20c1b to c9b61ee Compare January 24, 2025 20:20
@erights erights force-pushed the markm-use-no-trapping-shim branch from 103c1bb to 5b48cc6 Compare January 25, 2025 00:30
@erights erights force-pushed the markm-no-trapping-shim branch from 642154e to 2b82131 Compare January 25, 2025 03:23
@erights erights force-pushed the markm-use-no-trapping-shim branch 2 times, most recently from 3ceaac9 to 721c7e1 Compare January 25, 2025 18:55
@erights erights force-pushed the markm-no-trapping-shim branch from bcc292a to 2277afa Compare January 25, 2025 20:12
@erights erights force-pushed the markm-use-no-trapping-shim branch from 721c7e1 to 00efef2 Compare January 25, 2025 20:13
@erights erights force-pushed the markm-no-trapping-shim branch from 2277afa to b5a9f15 Compare January 25, 2025 20:48
@erights erights force-pushed the markm-use-no-trapping-shim branch from 00efef2 to 1b36221 Compare January 25, 2025 20:49
@erights erights force-pushed the markm-no-trapping-shim branch from b5a9f15 to 4646e3d Compare January 27, 2025 03:28
@erights erights force-pushed the markm-use-no-trapping-shim branch from 0098db1 to a11fc2e Compare January 27, 2025 03:40
@erights
Copy link
Contributor Author

erights commented Jan 27, 2025

So I have 2 concerns after looking at this PR and its adoption in agoric-sdk (Agoric/agoric-sdk#10795):

  • Given the early nature of the non-trapping proposal, I would like SES and endo packages to avoid taking a hard dependency on it. Can we instead feature detect and use non-trapping only if available? Then we can move the shim application to @endo/init?
  • The changes to harden (and to some extend passStyleOf) are presenting a potential upgrade hazard. We generally consider harden to be part of the platform, which means it may get upgraded independently of the code using it. The problem is that we have some code with harden usages that are no longer valid after this change: any code that was hardening proxies or their target. That means we will not be able to use the same user code with updated XS and/or lockdown bundles that include this change. I am really not sure how we work around this one, besides accepting this breaking change / incompatibility, and require that platforms like agoric-sdk use "repaired" lockdown bundles to skip/undo this change if it needs to load incompatible user code.

The shim is now opt-in, i.e., disabled by default. It has no effect unless explicitly opting in to the new behavior via env-options SES_NON_TRAPPING_SHIM=enabled

@erights erights marked this pull request as ready for review January 27, 2025 04:04
@erights
Copy link
Contributor Author

erights commented Jan 27, 2025

Converting to draft while we wait for draft #2684 to settle down

No longer any need to wait on that. This PR is again Ready for Review. PTAL

@erights erights requested review from michaelfig and mhofman January 27, 2025 04:05
@erights erights force-pushed the markm-use-no-trapping-shim branch from a11fc2e to e40dcb3 Compare January 27, 2025 04:50
@erights erights force-pushed the markm-no-trapping-shim branch from 2c3def1 to 95526ee Compare January 27, 2025 20:05
@erights erights force-pushed the markm-use-no-trapping-shim branch from e40dcb3 to 8173b85 Compare January 27, 2025 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants