feat(ses): Vetted shims #1705

kriskowal · 2023-07-29T00:06:43Z

Closes #422

This change separates out the internal phases of lockdown: repairIntrinsics() and hardenIntrinsics().

kriskowal · 2023-07-29T00:08:01Z

Developer log (draft):

I’m attempting to make it clearer that ses is itself a stack of shims, so I’ve refactored index.js to reflect that and hope to break it down further. I’ve also attempted to factor x.js and x-shim.js modules such that x.js implements the behavior in a way that might be used without shimming.

In the course of doing this, it became clear that the errors sub-section should not be using ambient types. I’ve attempted to correct this. I’m stopping for now at this yak:

$ tsc
types.test-d.ts:91:1 - error TS2775: Assertions require every name in the call target to be declared with an explicit type annotation.

91 assert.typeof(10.1, 'number');
...

kriskowal · 2023-08-19T00:24:49Z

I managed to getting vetted shims implemented without refactoring ambient types. Or rather, I managed to keep that work out of this pull request for purposes of limiting the scope of review and will submit that cleanup separately.

kriskowal · 2023-08-19T00:29:39Z

For reviewers: I’ve created an initial commit that renames lockdown-shim.js and compartment-shim.js to lockdown.js and compartment.js so that the subsequent commit’s diff clearly illustrates that all of the shim behavior was extracted from those modules and moved into the (new) adjacent -shim.js layering.

kriskowal · 2023-08-19T00:39:18Z

packages/ses/index.js

-/** getThis returns globalThis in sloppy mode or undefined in strict mode. */
-function getThis() {
-  return this;
-}
-
-if (getThis()) {
-  // See https://github.com/endojs/endo/blob/master/packages/ses/error-codes/SES_NO_SLOPPY.md
-  throw TypeError(`SES failed to initialize, sloppy mode (SES_NO_SLOPPY)`);
-}


Moves to a new assert-sloppy-mode.js, as imported by lockdown-shim.js to ensure it runs first and fails fast.

kriskowal · 2023-08-19T00:40:01Z

packages/ses/index.js

-
-assign(globalThis, {
-  lockdown,
-  Compartment,


Moves to compartment-shim.js

kriskowal · 2023-08-19T00:40:12Z

packages/ses/index.js

-);
-
-assign(globalThis, {
-  lockdown,


Moves to lockdown-shim.js

kriskowal · 2023-08-19T00:40:23Z

packages/ses/index.js

-assign(globalThis, {
-  lockdown,
-  Compartment,
-  assert,


Moves to assert-shim.js

kriskowal · 2023-08-19T00:40:45Z

packages/ses/index.js

-const markVirtualizedNativeFunction = tameFunctionToString();
-
-const Compartment = makeCompartmentConstructor(
-  makeCompartmentConstructor,
-  getGlobalIntrinsics(globalThis),
-  markVirtualizedNativeFunction,
-);


Moves to compartment-shim.js

kriskowal · 2023-08-19T00:43:14Z

packages/ses/src/lockdown-shim.js

+globalThis.repairIntrinsics = options => {
+  const hardenIntrinsics = repairIntrinsics(options);
+  // Reveal hardenIntrinsics after repairs.
+  globalThis.hardenIntrinsics = () => {
+    // Reveal harden after hardenIntrinsics.
+    // Harden is dangerous before hardenIntrinsics because hardening just
+    // about anything will inadvertently render intrinsics irreparable.
+    // Also, for modules that must work both before or after lockdown (code
+    // that is portable between JS and SES), the existence of harden in global
+    // scope signals whether such code should attempt to use harden in the
+    // defense of its own API.
+    // @ts-ignore harden not yet recognized on globalThis.
+    globalThis.harden = hardenIntrinsics();
+  };


I’ve moved all of the globalThis assignment behaviors into the shim to decontaminate the non-shim modules.

So, note that the global repairIntrinsics doesn’t return hardenIntrinsics and the global hardenIntrinsics does not return harden, but the internal versions perform this cascade.

hardenIntrinsics did not previously return harden.

leotm

we should update docs too right (mb better as follow-up)

so lockdown and harden relevant parts then add repair section

can prob reuse bits from the well-written issue description too

kriskowal · 2023-08-21T20:12:28Z

I’ve added a commit with documentation changes and the update for the change log. Thank you!

erights

just doc comments so far. rest coming but will take longer.

erights · 2023-08-21T21:53:23Z

packages/ses/docs/guide.md

+hardenIntrinsics();
+```
+
+And, an application that choses to call these also has the option of running


Suggested change

And, an application that choses to call these also has the option of running

An application that choses to call these also has the option of running

I'll do things like a jarring initial "And, " when it adds enough meaning to be worth it. I don't think it is here.

erights · 2023-08-21T21:57:55Z

packages/ses/docs/guide.md

+  The globals in a child compartment include the shared intrinsics including
+  `harden` and a batch of evaluators that run programs that will also be
+  confined to the compartment including `eval`, `Function`, and `Compartment`
+  itself.
+  Compartments also load and confine modules.


Need to be explicit that this is the post-repairIntrinsics behavior of Compartment. In our system as it is now, or as it is as of this PR, what is the status of Compartment before repairIntrinsics? I'm probably happy with any answer for now, since I only really care about after repairIntrinsics. But we should be explicit somewhere. If we are explicit elsewhere, can we link to it from here?

Compartment exists before repairIntrinsics and the only current difference is that the intrinsics are not hardened. Compartments constructed before repairIntrinsics do not guarantee confinement even after repairIntrinsics.

erights · 2023-08-21T22:01:30Z

packages/ses/docs/reference.md

+```js
+import './non-ses-code-before-lockdown.js';
+import './ses-repair-intrinsics.js'; // calls repairIntrinsics.
+import './vetted-shim.js';
+import './ses-harden-intrinsics.js'; // calls repairIntrinsics.
+import './ses-code-after-lockdown.js';
+```


It is even more important that this example import sequence appear in the guide. This is the use pattern we should generally encourage.

erights

What about https://github.com/endojs/endo/blob/master/packages/ses/docs/lockdown.md ?

Should "Lockdown options" be renamed? What about LOCKDOWN_* environment variables?

IMO the env vars do not need to be renamed. The doc should at least be clear about what these options mean in the new world.

kriskowal · 2023-08-21T23:54:49Z

In the type definition, I took the liberty of renaming LockdownOptions to RepairOptions and leaving an alias for backward compatibility. I agree that we do not need to change the names of the environment variables, or even really call attention to repairIntrinsics() unless it’s needed.

kriskowal · 2023-08-22T00:07:39Z

Question for reviewers: do we need to tackle making harden available after repairIntrinsics but before hardenIntrinsics with the queuing behavior? The heart of this question is whether it’s necessary and whether we can expect any environment to repair but not harden indefinitely.

On the question of necessity, a vetted shim could be divided into layers that run before and after hardenIntrinsics(). So, I’d argue it’s not necessary but might make for better ergonomics once we start implementing hardened shims.

I also don’t think repairing but not hardening intrinsics is coherent, but I might be forgetting something.

erights · 2023-08-22T00:55:31Z

Question for reviewers: do we need to tackle making harden available after repairIntrinsics but before hardenIntrinsics with the queuing behavior? The heart of this question is whether it’s necessary and whether we can expect any environment to repair but not harden indefinitely.

On the question of necessity, a vetted shim could be divided into layers that run before and after hardenIntrinsics(). So, I’d argue it’s not necessary but might make for better ergonomics once we start implementing hardened shims.

I also don’t think repairing but not hardening intrinsics is coherent, but I might be forgetting something.

How we want to bridge harden per-vs-post hardenIntrinsics is still an open design issue.
Let's keep this PR conservative without painting ourselves into any corners. I think the best way to do that is for hardenIntrinsics to introduce harden for now, but with a note that you should not count on harden being absent prior to hardenIntrinsics. In the future, some attenuated form of harden may exist between repairIntrinsics and hardenIntrinsics.

If we already have an issue where we're discussing that open design issue, we should link to it.

mhofman · 2023-08-22T02:14:51Z

Agreed with @erights. Let's not introduce some queuing behavior for harden that may not be part of the final design. I am ok with requiring shims to run a part post hardenIntrinsics for now. Maybe we should consider for harden to throw before hardenIntrinsics is called (which I believe is currently the behavior of XS's harden implementation before lockdown is called).

To be clear: a world where shims explicitly harden after hardenIntrinsics is forward compatible with a world where a pre-hardenIntrinsics vetted shim is able to use some version of harden and avoid running a post-hardenIntrinsics step.

erights · 2023-08-22T02:55:08Z

Maybe we should consider for harden to throw before ...

Also fine with me. Even better because the diagnostic can help set expectations. My favorite word for such messages is "yet" ;)

erights · 2023-08-22T02:56:58Z

OTOH, absence would make feature testing pleasant. I'm ok either way.

kriskowal · 2023-08-22T18:19:28Z

I’m settling on the current state: presence of globalThis.harden is a reliable signal that it’s available for use. I think this is more likely to be consistent with the @endo/harden ponyfill @mhofman and I have been discussing, and is the more reversible option.

erights · 2023-08-28T00:00:54Z

For reviewers: I’ve created an initial commit that renames lockdown-shim.js and compartment-shim.js to lockdown.js and compartment.js so that the subsequent commit’s diff clearly illustrates that all of the shim behavior was extracted from those modules and moved into the (new) adjacent -shim.js layering.

Thank you! I was reviewing the overall differences until I got to one of those. I'm surprised that github diff loses the mv behavior for the overall diffs, but it does. Your separation is a lifesaver.

erights

LGTM!!!!

I love that we can get this into the next endo release. Awesome!

erights · 2023-08-28T00:15:35Z

packages/ses/types.d.ts

@@ -36,6 +36,10 @@ export interface LockdownOptions {
  __hardenTaming__?: 'safe' | 'unsafe';
 }

+export type LockdownOptions = RepairOptions;


Please comment that we preserve LockdownOptions only for legacy compat. Perhaps deprecate?

- from (dist): endojs/endo#1705 - eslintignore unneeded since cjs not js

kriskowal force-pushed the kriskowal/422-vetted-shims branch from b049ea5 to c619f09 Compare August 19, 2023 00:22

kriskowal marked this pull request as ready for review August 19, 2023 00:38

kriskowal requested a review from erights August 19, 2023 00:38

kriskowal commented Aug 19, 2023

View reviewed changes

leotm reviewed Aug 21, 2023

View reviewed changes

kriskowal force-pushed the kriskowal/422-vetted-shims branch from db4a42e to fb969cb Compare August 21, 2023 20:12

erights reviewed Aug 21, 2023

View reviewed changes

erights self-requested a review August 22, 2023 21:30

kriskowal mentioned this pull request Aug 23, 2023

feat(ses): Freeze evaluators, Compartment constructor and Symbol #1731

Merged

erights approved these changes Aug 28, 2023

View reviewed changes

kriskowal added 4 commits August 27, 2023 21:50

chore(ses): Rename non-shims to make room for shim layers

d2e502f

feat(ses): Support vetted shims

40b59cc

fix(compartment-mapper): Update stack trace sensitive snapshots

7addc5c

docs(ses): Document vetted shims

76787e7

kriskowal force-pushed the kriskowal/422-vetted-shims branch from 0069174 to 76787e7 Compare August 28, 2023 04:51

kriskowal enabled auto-merge August 28, 2023 04:51

kriskowal merged commit 7f1f331 into master Aug 28, 2023

kriskowal deleted the kriskowal/422-vetted-shims branch August 28, 2023 04:53

leotm added a commit to MetaMask/metamask-mobile that referenced this pull request Aug 28, 2023

Add SES (cjs) with vetted shims pre-v0.18.8 tag plublish

5d473a2

- from (dist): endojs/endo#1705 - eslintignore unneeded since cjs not js

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ses): Vetted shims #1705

feat(ses): Vetted shims #1705

kriskowal commented Jul 29, 2023

kriskowal commented Jul 29, 2023 •

edited

Loading

kriskowal commented Aug 19, 2023

kriskowal commented Aug 19, 2023

kriskowal Aug 19, 2023

kriskowal Aug 19, 2023

kriskowal Aug 19, 2023

kriskowal Aug 19, 2023

kriskowal Aug 19, 2023

kriskowal Aug 19, 2023

leotm left a comment

kriskowal commented Aug 21, 2023

erights left a comment

erights Aug 21, 2023

erights Aug 21, 2023 •

edited

Loading

kriskowal Aug 21, 2023

erights Aug 21, 2023

erights left a comment

kriskowal commented Aug 21, 2023

kriskowal commented Aug 22, 2023

erights commented Aug 22, 2023 •

edited

Loading

mhofman commented Aug 22, 2023 •

edited

Loading

erights commented Aug 22, 2023

erights commented Aug 22, 2023

kriskowal commented Aug 22, 2023

erights commented Aug 28, 2023

erights left a comment

erights Aug 28, 2023

	And, an application that choses to call these also has the option of running
	An application that choses to call these also has the option of running

feat(ses): Vetted shims #1705

feat(ses): Vetted shims #1705

Conversation

kriskowal commented Jul 29, 2023

kriskowal commented Jul 29, 2023 • edited Loading

kriskowal commented Aug 19, 2023

kriskowal commented Aug 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leotm left a comment

Choose a reason for hiding this comment

kriskowal commented Aug 21, 2023

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

kriskowal commented Aug 21, 2023

kriskowal commented Aug 22, 2023

erights commented Aug 22, 2023 • edited Loading

mhofman commented Aug 22, 2023 • edited Loading

erights commented Aug 22, 2023

erights commented Aug 22, 2023

kriskowal commented Aug 22, 2023

erights commented Aug 28, 2023

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Jul 29, 2023 •

edited

Loading

erights Aug 21, 2023 •

edited

Loading

erights commented Aug 22, 2023 •

edited

Loading

mhofman commented Aug 22, 2023 •

edited

Loading