From 1175b4bfa72e9aa80cbe1b8c16d0fb457a8dfc9b Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Mon, 30 Aug 2021 14:30:48 -0700 Subject: [PATCH] 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 | 10 +++ app/scripts/lockdown-run.js | 98 ++++++++++++++++++++- development/build/static.js | 2 +- test/unit-global/frozenPromise.test.js | 53 ----------- test/unit-global/globalPatch.js | 2 - test/unit-global/protect-intrinsics.test.js | 71 +++++++++++++++ 6 files changed, 178 insertions(+), 58 deletions(-) delete mode 100644 test/unit-global/frozenPromise.test.js delete mode 100644 test/unit-global/globalPatch.js create mode 100644 test/unit-global/protect-intrinsics.test.js diff --git a/.eslintrc.js b/.eslintrc.js index d3763fdef..b6094c794 100644 --- a/.eslintrc.js +++ b/.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: { diff --git a/app/scripts/lockdown-run.js b/app/scripts/lockdown-run.js index f0682654e..3c7276fdf 100644 --- a/app/scripts/lockdown-run.js +++ b/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}`), + ); } } diff --git a/development/build/static.js b/development/build/static.js index 300134806..2aaa163ef 100644 --- a/development/build/static.js +++ b/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`, }, { diff --git a/test/unit-global/frozenPromise.test.js b/test/unit-global/frozenPromise.test.js deleted file mode 100644 index b41362266..000000000 --- a/test/unit-global/frozenPromise.test.js +++ /dev/null @@ -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'); - } - }); -}); diff --git a/test/unit-global/globalPatch.js b/test/unit-global/globalPatch.js deleted file mode 100644 index da239953c..000000000 --- a/test/unit-global/globalPatch.js +++ /dev/null @@ -1,2 +0,0 @@ -// eslint-disable-next-line import/unambiguous,node/no-unsupported-features/es-builtins -global.globalThis = global; diff --git a/test/unit-global/protect-intrinsics.test.js b/test/unit-global/protect-intrinsics.test.js new file mode 100644 index 000000000..5075080f9 --- /dev/null +++ b/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`, + ); + } + }); + }); +});