Make all named intrinsics non-modifiable (#11953)

This PR makes ~all named intrinsics in all of our JavaScript processes non-modifiable. A named intrinsic is any property specified by the ECMAScript specification that exists on `globalThis` when the JavaScript process starts. We say that a property is non-modifiable if it is non-configurable and non-writable. We make exceptions for properties that meet any of the following criteria:

1. Properties that are non-configurable by the time `lockdown-run.js` is executed are not modified, because they can't be.
2. Properties that have accessor properties (`get` or `set`) are made non-configurable, but their writability cannot be modified, and is therefore left unchanged. It's unclear how many of the named intrinsics this applies to, if any, but it's good defensive programming, regardless.

.eslintrc.js

@@ -166,6 +166,16 @@ module.exports = {
         sourceType: 'script',
       },
     },
+    {
+      files: [
+        'app/scripts/lockdown-run.js',
+        'test/unit-global/protect-intrinsics.test.js',
+      ],
+      globals: {
+        harden: 'readonly',
+        Compartment: 'readonly',
+      },
+    },
   ],
 
   settings: {

app/scripts/lockdown-run.js

@@ -12,9 +12,103 @@ try {
   // If the `lockdown` call throws an exception, it interferes with the
   // contentscript injection on some versions of Firefox. The error is
   // caught and logged here so that the contentscript still gets injected.
-  // This affects Firefox v56 and Waterfox Classic
+  // This affects Firefox v56 and Waterfox Classic.
   console.error('Lockdown failed:', error);
   if (globalThis.sentry && globalThis.sentry.captureException) {
-    globalThis.sentry.captureException(error);
+    globalThis.sentry.captureException(
+      new Error(`Lockdown failed: ${error.message}`),
+    );
+  }
+}
+
+// Make all "object" and "function" own properties of globalThis
+// non-configurable and non-writable, when possible.
+// We call the a property that is non-configurable and non-writable,
+// "non-modifiable".
+try {
+  /**
+   * `lockdown` only hardens the properties enumerated by the
+   * universalPropertyNames constant specified in 'ses/src/whitelist'. This
+   * function makes all function and object properties on the start compartment
+   * global non-configurable and non-writable, unless they are already
+   * non-configurable.
+   *
+   * It is critical that this function runs at the right time during
+   * initialization, which should always be immediately after `lockdown` has been
+   * called. At the time of writing, the modifications this function makes to the
+   * runtime environment appear to be non-breaking, but that could change with
+   * the addition of dependencies, or the order of our scripts in our HTML files.
+   * Exercise caution.
+   *
+   * See inline comments for implementation details.
+   *
+   * We write this function in IIFE format to avoid polluting global scope.
+   */
+  (function protectIntrinsics() {
+    const namedIntrinsics = Reflect.ownKeys(new Compartment().globalThis);
+
+    // These named intrinsics are not automatically hardened by `lockdown`
+    const shouldHardenManually = new Set(['eval', 'Function']);
+
+    const globalProperties = new Set([
+      // universalPropertyNames is a constant added by lockdown to global scope
+      // at the time of writing, it is initialized in 'ses/src/whitelist'.
+      // These properties tend to be non-enumerable.
+      ...namedIntrinsics,
+
+      // TODO: Also include the named platform globals
+      // This grabs every enumerable property on globalThis.
+      // ...Object.keys(globalThis),
+    ]);
+
+    globalProperties.forEach((propertyName) => {
+      const descriptor = Reflect.getOwnPropertyDescriptor(
+        globalThis,
+        propertyName,
+      );
+
+      if (descriptor) {
+        if (descriptor.configurable) {
+          // If the property on globalThis is configurable, make it
+          // non-configurable. If it has no accessor properties, also make it
+          // non-writable.
+          if (hasAccessor(descriptor)) {
+            Object.defineProperty(globalThis, propertyName, {
+              configurable: false,
+            });
+          } else {
+            Object.defineProperty(globalThis, propertyName, {
+              configurable: false,
+              writable: false,
+            });
+          }
+        }
+
+        if (shouldHardenManually.has(propertyName)) {
+          harden(globalThis[propertyName]);
+        }
+      }
+    });
+
+    /**
+     * Checks whether the given propertyName descriptor has any accessors, i.e. the
+     * properties `get` or `set`.
+     *
+     * We want to make globals non-writable, and we can't set the `writable`
+     * property and accessor properties at the same time.
+     *
+     * @param {Object} descriptor - The propertyName descriptor to check.
+     * @returns {boolean} Whether the propertyName descriptor has any accessors.
+     */
+    function hasAccessor(descriptor) {
+      return 'set' in descriptor || 'get' in descriptor;
+    }
+  })();
+} catch (error) {
+  console.error('Protecting intrinsics failed:', error);
+  if (globalThis.sentry && globalThis.sentry.captureException) {
+    globalThis.sentry.captureException(
+      new Error(`Protecting intrinsics failed: ${error.message}`),
+    );
   }
 }

development/build/static.js

@@ -48,7 +48,7 @@ const copyTargets = [
     dest: `globalthis.js`,
   },
   {
-    src: `./node_modules/ses/dist/lockdown.cjs`,
+    src: `./node_modules/ses/dist/lockdown.umd.min.js`,
     dest: `lockdown-install.js`,
   },
   {

test/unit-global/frozenPromise.test.js

@@ -1,53 +0,0 @@
-// Should occur before anything else
-import './globalPatch';
-import 'ses/lockdown';
-import '../../app/scripts/lockdown-run';
-import { strict as assert } from 'assert'; /* eslint-disable-line import/first,import/order */
-
-describe('Promise global is immutable', function () {
-  it('throws when reassinging promise (syntax 1)', function () {
-    try {
-      // eslint-disable-next-line no-global-assign,no-native-reassign
-      Promise = {};
-      assert.fail('did not throw error');
-    } catch (err) {
-      assert.ok(err, 'did throw error');
-    }
-  });
-
-  it('throws when reassinging promise (syntax 2)', function () {
-    try {
-      global.Promise = {};
-      assert.fail('did not throw error');
-    } catch (err) {
-      assert.ok(err, 'did throw error');
-    }
-  });
-
-  it('throws when mutating existing Promise property', function () {
-    try {
-      Promise.all = () => undefined;
-      assert.fail('did not throw error');
-    } catch (err) {
-      assert.ok(err, 'did throw error');
-    }
-  });
-
-  it('throws when adding new Promise property', function () {
-    try {
-      Promise.foo = 'bar';
-      assert.fail('did not throw error');
-    } catch (err) {
-      assert.ok(err, 'did throw error');
-    }
-  });
-
-  it('throws when deleting Promise from global', function () {
-    try {
-      delete global.Promise;
-      assert.fail('did not throw error');
-    } catch (err) {
-      assert.ok(err, 'did throw error');
-    }
-  });
-});

test/unit-global/globalPatch.js

@@ -1,2 +0,0 @@
-// eslint-disable-next-line import/unambiguous,node/no-unsupported-features/es-builtins
-global.globalThis = global;

test/unit-global/protect-intrinsics.test.js

@@ -0,0 +1,71 @@
+import 'ses/lockdown';
+import '../../app/scripts/lockdown-run';
+import { strict as assert } from 'assert';
+
+// These are Agoric inventions, and we don't care about them.
+const ignoreList = new Set([
+  'Compartment',
+  'HandledPromise',
+  'StaticModuleRecord',
+]);
+
+describe('non-modifiable intrinsics', function () {
+  const namedIntrinsics = Reflect.ownKeys(new Compartment().globalThis);
+
+  const globalProperties = new Set(
+    [
+      // Added to global scope by ses/dist/lockdown.cjs.
+      ...namedIntrinsics,
+
+      // TODO: Also include the named platform globals
+      // This grabs every enumerable property on globalThis.
+      // ...Object.keys(globalThis),
+    ].filter((propertyName) => !ignoreList.has(propertyName)),
+  );
+
+  globalProperties.forEach((propertyName) => {
+    it(`intrinsic globalThis["${propertyName}"]`, function () {
+      const descriptor = Reflect.getOwnPropertyDescriptor(
+        globalThis,
+        propertyName,
+      );
+
+      assert.ok(
+        descriptor,
+        `globalThis["${propertyName}"] should have a descriptor`,
+      );
+
+      // As long as Object.isFrozen is the true Object.isFrozen, the object
+      // it is called with cannot lie about being frozen.
+      const value = globalThis[propertyName];
+      if (value !== globalThis) {
+        assert.equal(
+          Object.isFrozen(value),
+          true,
+          `value of universal property globalThis["${propertyName}"] should be frozen`,
+        );
+      }
+
+      // The writability of properties with accessors cannot be modified.
+      if ('set' in descriptor || 'get' in descriptor) {
+        assert.equal(
+          descriptor.configurable,
+          false,
+          `globalThis["${propertyName}"] should be non-configurable`,
+        );
+      } else {
+        assert.equal(
+          descriptor.configurable,
+          false,
+          `globalThis["${propertyName}"] should be non-configurable`,
+        );
+
+        assert.equal(
+          descriptor.writable,
+          false,
+          `globalThis["${propertyName}"] should be non-writable`,
+        );
+      }
+    });
+  });
+});