Surface SES on globalThis #307

kriskowal · 2020-05-12T21:51:29Z

Previously, SES exported lockdown. Calling lockdown introduces a global harden.

This change removes all exports and instead surfaces the same names on globalThis. That is, lockdown, harden, Compartment, and ModuleStaticRecord are all globals as they would be if they were introduced by the platform rather than a shim.

Fixes #301

erights · 2020-05-17T01:22:35Z

packages/ses/NEWS.md

@@ -2,6 +2,9 @@ User-visible changes in SES:

 ## Next release

+* Adds support for modules to Compartments.
+* SES no longer exports anything. `Compartment`, `ModuleStaticRecord`,
+  `lockdown`, and `harden` are all introduced as properties of `globalThis`.
 * Repair `Function.apply` and `TypeError.message` (as well as `.message` on
  all the other Error types), to tolerate what the npm `es-abstract` module
  does. This allows `tape` (version 4.x) to be loaded in a locked-down SES


Please make the reference to #293 a markdown link to #293

erights · 2020-05-17T02:02:19Z

packages/ses/src/lockdown-shim.js

   */

  // Circumvent the override mistake.
  const detachedProperties = enablePropertyOverrides(intrinsics);

  // Finally register and optionally freeze all the intrinsics. This
  // must be the operation that modifies the intrinsics.
-  harden(intrinsics, registerOnly);
-  harden(detachedProperties, registerOnly);
+  lockdownHarden(intrinsics, registerOnly);


Where is this harden (aka lockdownHarden) function that takes a second parameter? I couldn't find it.

https://github.com/Agoric/SES-shim/blob/0e83a286dc01ee16d20434548bd8bac0faab30bc/packages/ses/src/lockdown-shim.js#L42

Whose source is at https://github.com/Agoric/SES-shim/blob/0e83a286dc01ee16d20434548bd8bac0faab30bc/packages/make-hardener/src/main.js#L67 which takes only one parameter?

I checked. lockdownHarden, which is just the local name for harden that is usable before lockdown finishes, does not accept a second argument. To me, this looks like the registerOnly behavior does not work and (because tests pass) is not tested. I don’t know what it is meant to do.

What is registerOnly supposed to mean or do? What is it needed for?

There is no indication anywhere in the code or documentation of what registerOnly means or should do. I would be glad to erase it in this change.

Yes, please do.

For a small security kernel, I'd prefer to err on the side of removing things we don't know the purpose of. Sometimes, in review, I'll know and clarify.

erights · 2020-05-17T02:12:00Z

packages/ses/src/main.js

+  lockdown,
+  harden,
+  Compartment,
+  ModuleStaticRecord,


All shimmed pervasively global names must be added to the appropriate whitelists. I don't see lockdown and ModuleStaticRecord (or StaticModuleRecord) in intrinsic-names.js

I do see ModuleStaticRecord in intrinsic-globals.js, but I don't see lockdown there.

lockdown missing from whitelist.js as well.

We should have a test that fails on such omission, because it is an easy mistake to keep making.

TODO(markm): the whole intrinsic management system now consists of nine separate files with too much redundancy, e.g., between intrinsic-names.js, intrinsic-globals.js, and whitelist.js. We should simplify, make it easier to maintain, and self-test consistency among what redundancy remains. Also, check-intrinsics.js seems vacuous. Bad edit from a previously meaningful state?

Filed TODO as #317

This is the kind of thing that I worry most about. Thanks for catching this.

Adding lockdown to the whitelist caused tests to fail, so I’ll come back to it when we have a firm resolution as to whether it should preserve itself after lockdown.

ok. But make sure the state as of merging this PR is consistent with itself.

erights · 2020-05-17T02:26:37Z

packages/ses/src/lockdown-shim.js

@@ -35,6 +33,19 @@ function assert(condition, message) {
  }


Why not import this from assert.js? It is currently identical, and it will make it easy to upgrade all occurrences to the new assert system when it is time to do that.

erights · 2020-05-17T02:39:19Z

packages/ses/README.md

+Alters the surrounding execution environment such that no object intrinsically
+accessible, like the array or function prototypes (`[].prototype` or `(() =>


We've been using the term "undeniable". "undeniably accessible" isn't great. But "intrinsically accessible" has no clear meaning. Better phrase appreciated.

erights · 2020-05-17T02:48:21Z

packages/ses/README.md

+accessible, like the array or function prototypes (`[].prototype` or `(() =>
+0).prototype`, can be replaced or subverted to attack other code in the same


It is not .prototype. It is .__proto__ or Object.getPrototypeOf or Reflect.getPrototypeOf.

Pedantic note that shouldn't affect this README.md:

In some thread elsewhere @caridy points out that these means of ascending the prototype chain are deniable in theory. But we all agreed they are not practically deniable. @ljharb pointed out that one undeniable, %ThrowTypeError% is obtained only by accessing the getter property of a property descriptor. This again is deniable in theory. So we're left with "reachable by syntax combined with universal reflective operations, like accessing [[Prototype]] or accessing a property getter". Fortunately, this README need not be that precise about undeniability.

Thread is at tc39/how-we-work#84

erights · 2020-05-17T02:50:13Z

packages/ses/README.md

+0).prototype`, can be replaced or subverted to attack other code in the same
+execution environment; and tames other intrinsically accessible utilities like
+regular expressions, random numbers, and clocks, to minimize opportunities for
+malicious programs to covertly infer the behavior of other programs running in


"infer or interfere"

erights · 2020-05-17T02:50:48Z

packages/ses/README.md

+import 'ses';
+
+import 'my-vetted-shim';
+
+lockdown();


Hm, that shows off the simple one-file use case well. I'm on the fence as to whether my-vetted-shim should be imported before ses (to visually suggest that it is unconfined), or after (to remind readers that nothing is actually confined until lockdown() is called).

But a more realistic usage summary would involve confined modules too. With the current feature set, that means writing an install-ses module (consisting entirely of import 'ses'; lockdown();), and doing the other imports before/after it:

import 'my-vetted-shim'; import 'install-ses'; import 'confined-module'; other_confined_code();

Do we want to show this here/now, or will the upcoming module-import support provide a better way? I'm hoping we'll have some docs that explain how to use this in a way similar to how folks are used to writing/running programs today, rather than putting off the practicalities in favor of a future application-loader/manifest/"larger-scale" ecosystem that we're still trying to figure out.

my-vetted-shim is not confined; which is why it, along with any code run before lockdown(), must be vetted. IOW, all code run before lockdown() can do powerful dangerous things, creating unlimited damage to everything run afterward, including lockdown() itself.

Running before import 'ses'; vs between it and lockdown() makes only one difference: whether harden,ModuleStaticRecord,Compartment, and lockdown are available as globals.

I just looked at the shim source and confirmed that import 'ses'; by itself causes no effects other than making these available as properties on the global object and thereby as global variables.

I believe @warner is speaking to the point of an install-ses module that looks like:

import 'ses'; lockdown();

Such that in his example, confined-module is running after lockdown.

My intuition is that this is dangerously clever since the so-called confined module still has access to power that lockdown was unable to expunge from the start compartment.

Not sure whether we should highlight that usage pattern, as I am uncertain about how dangerous it is.

erights · 2020-05-17T02:51:30Z

packages/ses/README.md

+
+lockdown();
+
+console.log(Object.isFrozen([].prototype));


[].__proto__

erights · 2020-05-17T02:52:07Z

packages/ses/README.md

+
+SES introduces the `harden` function.
+*After* calling `lockdown`, the `harden` function ensures that whatever object
+you give it is also transitively frozen out to the execution environments


environment's

"transitively frozen" always causes confusion, it that people mistake it as being about immutability. We must always be clear that it is only transitive across own property traversal, in order to hide API surface. You do mention "surface" here but without a bit more context it is not clear what you mean.

Your example below perfectly illustrates what is frozen by harden. Perhaps have use close over a variable it mutates, so you can explain what is not suppressed by harden?

Then link to wherever we have a more complete explanation of harden including its consistency check against of the [[Prototype]] properties.

We have no explanation, not even inline. #320

erights · 2020-05-17T03:00:14Z

packages/ses/README.md

+`globalThis` and wholly independent system of modules, but otherwise shares
+the same batch of intrinsics like `Array` with the surrounding compartment.
+The concept of a compartment implies the existence of a "start compartment",
+the initial execution argument.


I don't think you mean "argument"

erights · 2020-05-17T03:03:12Z

packages/ses/README.md

 const c = new Compartment({
    print: harden(console.log),
 });

 c.evaluate(`
-    print("Hello! Hello?");
+    print('Hello! Hello?');
 `);
 ```

 The new compartment has a different global object than the start compartment.
 The global object is initially mutable.
 Locking down the start compartment hardened many of the intrinsics in global


Avoid the phrase "locking down the start compartment". This invites the confusion the rest of the paragraph corrects. lockdown locks down the realm, and therefore the shared intrinsics, that the start compartment shares with these other compartments.

erights · 2020-05-17T03:05:04Z

packages/ses/README.md

 `);
 ```

 The new compartment has a different global object than the start compartment.
 The global object is initially mutable.
 Locking down the start compartment hardened many of the intrinsics in global
 scope.
-After lockdown, no compartment can tamper with these intrinsics.
+After `lockdown`, no compartment can tamper with these intrinsics.
 Many of these intrinsics are identical in the new compartment.

 ```js


Avoid terms like "the property holds" below when you're not referring to object properties but in a context where you might be.

warner

I don't see any big problems beyond the ones Mark flagged. The docs changes are minor, and if we have a usable module-importing example on the way, I'm ok to see this land without demonstrating any sort of install-ses scheme.

warner · 2020-05-18T19:24:43Z

packages/ses/README.md

-that that object and any object reachable from its surface.
-The compartment can import modules and evaluate programs.
+SES introduces the `lockdown` function.
+Alters the surrounding execution environment such that no object intrinsically


grammar/readability nits: maybe:

Calling lockdown alters the surrounding..

.. no intrinsically-accessible object, like ..

. lockdown also tames other ..

warner · 2020-05-18T19:31:59Z

packages/ses/README.md

+import 'ses';
+
+import 'my-vetted-shim';
+
+lockdown();


Hm, that shows off the simple one-file use case well. I'm on the fence as to whether my-vetted-shim should be imported before ses (to visually suggest that it is unconfined), or after (to remind readers that nothing is actually confined until lockdown() is called).

But a more realistic usage summary would involve confined modules too. With the current feature set, that means writing an install-ses module (consisting entirely of import 'ses'; lockdown();), and doing the other imports before/after it:

import 'my-vetted-shim'; import 'install-ses'; import 'confined-module'; other_confined_code();

Do we want to show this here/now, or will the upcoming module-import support provide a better way? I'm hoping we'll have some docs that explain how to use this in a way similar to how folks are used to writing/running programs today, rather than putting off the practicalities in favor of a future application-loader/manifest/"larger-scale" ecosystem that we're still trying to figure out.

warner · 2020-05-18T19:35:10Z

packages/ses/README.md

@@ -74,8 +119,6 @@ c1.global === c2.global; // false
 c1.global.JSON === c2.global.JSON; // true


c1.global or c1.globalThis? I guess it depends upon whether #313 lands before or after this one.

This is the PR that resolves #313, so I’ll fix this here!

kriskowal · 2020-05-19T02:39:44Z

I’ve responded to most the great feedback from @warner and @erights and request review to the two new fixup commits I’ve added to the end.

Lots of refinement on the verbiage.
Removed the registerOnly option that didn’t do anything.
Left lockdown out of the whitelists, but made sure ModuleStaticRecord and Compartment got in there with their prototypes, and harden.

erights

LGTM but for one doc correction.

erights · 2020-05-19T05:42:38Z

packages/ses/README.md

+{}).__proto__`, the properties of any accessible object, the return values of
+any accessible function, and the result of using any accessible constructor.


the return values of any accessible function, and the result of using any accessible constructor.

Either I misunderstand you, or this is wrong. Calling a function or constructor can, and often does, allocate and return a fresh mutable object not locked down by lockdown(). I think just deleting that part of the sentence fixes the problem.

kriskowal force-pushed the kris/global-ses branch 5 times, most recently from 096d712 to 777e58b Compare May 13, 2020 18:40

kriskowal self-assigned this May 13, 2020

kriskowal force-pushed the kris/global-ses branch 8 times, most recently from 190af04 to 3c10a06 Compare May 15, 2020 23:10

kriskowal marked this pull request as ready for review May 15, 2020 23:10

kriskowal requested a review from warner May 15, 2020 23:10

warner mentioned this pull request May 15, 2020

start to add some (manual) compatibility tests #311

Open

kriskowal force-pushed the kris/global-ses branch from 3c10a06 to 0e83a28 Compare May 16, 2020 00:57

kriskowal requested a review from erights May 16, 2020 00:58

erights requested changes May 17, 2020

View reviewed changes

warner approved these changes May 18, 2020

View reviewed changes

kriskowal added 5 commits May 18, 2020 19:34

Surface SES as global properties instead of exports

48510ed

Shim harden immediately but gate on lockdown

8b60f40

Update introduction to SES

82f7b25

fixup global SES erights warner feedback 1

9be0b37

fixup global SES, omit registerOnly

71b1d97

kriskowal force-pushed the kris/global-ses branch from 0e83a28 to 71b1d97 Compare May 19, 2020 02:34

erights approved these changes May 19, 2020

View reviewed changes

fixup Correction to the lockdown closure of properties

0fc17e8

kriskowal merged commit 3ddfb95 into master May 19, 2020

kriskowal deleted the kris/global-ses branch May 19, 2020 20:20

		Alters the surrounding execution environment such that no object intrinsically
		accessible, like the array or function prototypes (`[].prototype` or `(() =>

		accessible, like the array or function prototypes (`[].prototype` or `(() =>
		0).prototype`, can be replaced or subverted to attack other code in the same

		@@ -74,8 +119,6 @@ c1.global === c2.global; // false
		c1.global.JSON === c2.global.JSON; // true

		{}).__proto__`, the properties of any accessible object, the return values of
		any accessible function, and the result of using any accessible constructor.


		lockdown();

		console.log(Object.isFrozen([].prototype));

Surface SES on globalThis #307

Surface SES on globalThis #307

Conversation

kriskowal commented May 12, 2020

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

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

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

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

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

warner left a 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

kriskowal commented May 19, 2020

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment