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): option to fake harden unsafely #1528

Merged
merged 1 commit into from
Mar 30, 2023
Merged

Conversation

erights
Copy link
Contributor

@erights erights commented Mar 28, 2023

Adds an option to make harden a do-nothing identity function, specifically for speed of the SwingSet kernel, though it could be used for other highly vetted, style restricted, security-critical, and speed-critical code. This would be safe to turn on in the SwingSet kernel or other such specialized code once we're confident that they are free of the kinds of bugs that a working harden would have protected them from.

Adds a __hardenTaming__ option to lockdown, with values 'safe' (the default) in which harden still works, and 'unsafe', in which harden is a do-nothing identity function.

There are various tests for whether something is frozen, sealed, non-extensible, non-configurable, non-writable, that could all be broken by this fake harden. However, in all cases in our non-test, non-demo code that we are aware of so far, for each such branch, one side of the branch reports an error and only the other side is the happy path. If we're confident we have no bugs that harden would have caught, then we need only ensure we go down the happy paths for such tests.

Object.isFrozen, Object.isSealed, Object.isExtensible, Reflect.isExtensible are patched to claim that everything is frozen, since that is typically the happy path. But not always. For those rare occasions where not being frozen is the happy path, we have added a harden.isFake = true property. When this unsafe option is not turned on, there is no isFake property, so harden.isFake is falsy. This lets code test harden.isFake to ensure it still goes down the happy path. This PR fixes the one case of that we know of in the non-test, non-demo code of the endo repository. To my surprise, there do not seem to be any in the non-test, non-demo code of the agoric-sdk repository.

At this time, we do not patch any of the builtins for reflecting on property attributes, such as Object.getOwnPropertyDescriptor. We will soon see if we need to.


Reviewers, please review by commit. The second commit, fix: tolerate LOCKDOWN_HARDEN_TAMING=unsafe, is not needed to pass under CI. But it is needed to pass locally if I first set export LOCKDOWN_HARDEN_TAMING=unsafe. The fact that it only modifies test files so that the first commit made all the needed changes to the non-test,non-demo files.

@erights erights self-assigned this Mar 28, 2023
@erights erights force-pushed the markm-fake-harden branch from d9e7322 to a59ba4a Compare March 28, 2023 02:55
@erights erights force-pushed the markm-fake-harden branch from a59ba4a to 4f540b7 Compare March 28, 2023 03:06
@erights erights requested a review from mhofman March 28, 2023 03:07
@erights erights force-pushed the markm-fake-harden branch 3 times, most recently from 1416999 to b5d666a Compare March 28, 2023 03:30
@mhofman
Copy link
Contributor

mhofman commented Mar 28, 2023

I am quite surprised tests pass without further mitigations for property descriptors. I would have expected some parts of compartments to complain, but I suppose since lockdown is optional for compartment, maybe not. I suppose some things like optimizers may not behave as expected though.

@erights
Copy link
Contributor Author

erights commented Mar 28, 2023

The only thing running with the option set to 'unsafe' is the one tiny new test. Now other test should see any difference.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

These changes are internally consistent, but I'm concerned about the boundary of a trusted environment, e.g. between the SwingSet kernel and a local-worker vat. How much confidence do we have that no such cases exist? Should we expose a way to get a functional harden even when the default one is fake, and if so then how would we know we're using it everywhere we should be?

packages/ses/docs/lockdown.md Outdated Show resolved Hide resolved
packages/pass-style/src/make-far.js Outdated Show resolved Hide resolved
packages/ses/docs/lockdown.md Show resolved Hide resolved
packages/ses/src/tame-harden.js Outdated Show resolved Hide resolved
packages/ses/test/test-tame-harden.js Outdated Show resolved Hide resolved
packages/init/test/test-async_hooks.js Outdated Show resolved Hide resolved
packages/marshal/test/test-marshal-capdata.js Outdated Show resolved Hide resolved
@erights
Copy link
Contributor Author

erights commented Mar 30, 2023

These changes are internally consistent, but I'm concerned about the boundary of a trusted environment, e.g. between the SwingSet kernel and a local-worker vat. How much confidence do we have that no such cases exist? Should we expose a way to get a functional harden even when the default one is fake, and if so then how would we know we're using it everywhere we should be?

@warner Are local-worker vats ever to be used in production?

@gibson042 @warner , I assumed not. If they are, we need to discuss.

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