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: explicitly harden some shared prototypes #1939

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Conversation

mhofman
Copy link
Contributor

@mhofman mhofman commented Jan 9, 2024

refs: #1914

Description

While working on #1686, I found out that some shared prototypes are currently hardened as a side effect of an instance being hardened. This PR adds explicit harden for these.

It also contains a small refactor to harden all intrinsics in one shot instead of multiple calls to harden, as there are cycles in those, and some intrinsics were effectively hardened in other calls.

Security Considerations

More explicit hardening brings more security

Scaling Considerations

None

Documentation Considerations

In Node.js, the Buffer and when the console is untamed, the internal SafeMap are now explicitly hardened. These would likely get hardened as side-effect of hardening instances before.

Testing Considerations

Tested in #1914 and Agoric/agoric-sdk#8700 that these are all prototypes found through instances by existing tests. There may be others lurking, but until we have shallow hardening implemented, we won't find them.

Upgrade Considerations

This is a non breaking change that should be mostly unobservable to consumers.

@mhofman mhofman requested a review from erights January 9, 2024 16:14
@mhofman mhofman force-pushed the mhofman/explicit-harden branch from dd18c5a to 1fedbe1 Compare January 9, 2024 16:33
@mhofman mhofman marked this pull request as draft January 9, 2024 16:38
@mhofman
Copy link
Contributor Author

mhofman commented Jan 9, 2024

Apparently some changes run before lockdown, back to drawing board

Nope I'm an idiot, I forgot a line in my refactor

@mhofman mhofman force-pushed the mhofman/explicit-harden branch from 1fedbe1 to 87d0f65 Compare January 9, 2024 17:26
@mhofman mhofman marked this pull request as ready for review January 9, 2024 17:49
@mhofman mhofman mentioned this pull request Jan 9, 2024
Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

Except for the one misplaced set of changes to ses-ava-test.js, LGTM.

Thanks for this!

Thanks especially for figuring out and taking care of that Node console weirdness.

@@ -75,6 +75,16 @@ const overrideList = [
'only',
];

// Helper to explicitely harden an object including its prototype chain
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the changes in this file be moved into #1914 ? They don't really make sense in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I debated this. Happy to move to #1914

@mhofman mhofman force-pushed the mhofman/explicit-harden branch from 87d0f65 to 148f101 Compare January 9, 2024 23:53
@mhofman mhofman enabled auto-merge January 10, 2024 00:01
@mhofman mhofman merged commit 91b0ac9 into master Jan 10, 2024
14 checks passed
@mhofman mhofman deleted the mhofman/explicit-harden branch January 10, 2024 00:01
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.

2 participants