diff --git a/packages/ses/NEWS.md b/packages/ses/NEWS.md index 28b3ad0319..07b4fc1cbe 100644 --- a/packages/ses/NEWS.md +++ b/packages/ses/NEWS.md @@ -8,6 +8,15 @@ User-visible changes in SES: setting. At this setting, assigning to the `name` property of a mutable error instance should work. It will continue not to work at the `'min'` setting, so use the default `'moderate'` setting if you need to. +- Adds a lockdown option `domainTaming` to detect whether Node.js domains have + been initialized and prevents them from being initialized afterward. + Domains otherwise presented a hazard to the integrity of SES containment on + Node.js. + The option defaults to `"unsafe"` in this version and will be switched to + `"safe"` by default in the next release that allows for breaking-changes, to + afford a gradual migration. + Thank you to @dominictarr with [Least Authority](https://leastauthority.com/) + for devising this feature. # 0.14.1 (2021-08-12) diff --git a/packages/ses/error-codes/SES_NO_DOMAINS.md b/packages/ses/error-codes/SES_NO_DOMAINS.md new file mode 100644 index 0000000000..239cd49a75 --- /dev/null +++ b/packages/ses/error-codes/SES_NO_DOMAINS.md @@ -0,0 +1,16 @@ +# SES failed to lockdown, Node.js domains have been initialized (SES_NO_DOMAINS) + +The SES shim cannot guarantee the containment of tenant programs if a program +uses the deprecated [Node.js domains](https://nodejs.org/api/domain.html) +feature. + +SES attempts to detect whether Node.js domains have been initialized, and also +attempts to prevent the domains module from initializing in the future, by +testing and breaking the `process.domain` property. + +Domains introduce a `domain` property on various objects, but most +perniciously, every `Promise` object, in a way that SES is unable to intercept +or prevent. +These domain properties are unfrozen, so they can be used for covert +communication between tenant programs, and provide a view into dynamic scope +that any program can directly manipulate to confuse other programs. diff --git a/packages/ses/index.d.ts b/packages/ses/index.d.ts index 1509144991..1c922893c0 100644 --- a/packages/ses/index.d.ts +++ b/packages/ses/index.d.ts @@ -25,6 +25,7 @@ export interface LockdownOptions { stackFiltering?: 'concise' | 'verbose'; overrideTaming?: 'moderate' | 'min' | 'severe'; overrideDebug?: Array; + domainTaming?: 'safe' | 'unsafe'; __allowUnsafeMonkeyPatching__?: 'safe' | 'unsafe'; } diff --git a/packages/ses/index.js b/packages/ses/index.js index 9e4860eaed..19f51472e5 100644 --- a/packages/ses/index.js +++ b/packages/ses/index.js @@ -13,6 +13,7 @@ // limitations under the License. import { globalThis, TypeError, assign } from './src/commons.js'; + import { tameFunctionToString } from './src/tame-function-tostring.js'; import { getGlobalIntrinsics } from './src/intrinsics.js'; import { getAnonymousIntrinsics } from './src/get-anonymous-intrinsics.js'; diff --git a/packages/ses/src/lockdown-shim.js b/packages/ses/src/lockdown-shim.js index d0d3ac9648..418a2a00a3 100644 --- a/packages/ses/src/lockdown-shim.js +++ b/packages/ses/src/lockdown-shim.js @@ -27,6 +27,7 @@ import tameLocaleMethods from './tame-locale-methods.js'; import { initGlobalObject } from './global-object.js'; import { initialGlobalPropertyNames } from './whitelist.js'; import { tameFunctionToString } from './tame-function-tostring.js'; +import { tameDomains } from './tame-domains.js'; import { tameConsole } from './error/tame-console.js'; import tameErrorConstructor from './error/tame-error-constructor.js'; @@ -139,6 +140,7 @@ export const repairIntrinsics = ( overrideTaming = 'moderate', overrideDebug = [], stackFiltering = 'concise', + domainTaming = 'unsafe', // To become 'safe' by default in next-breaking-release. __allowUnsafeMonkeyPatching__ = 'safe', ...extraOptions @@ -173,6 +175,7 @@ export const repairIntrinsics = ( overrideTaming, overrideDebug, stackFiltering, + domainTaming, __allowUnsafeMonkeyPatching__, }; @@ -220,6 +223,9 @@ export const repairIntrinsics = ( /** * 1. TAME powers & gather intrinsics first. */ + + tameDomains(domainTaming); + const { addIntrinsics, completePrototypes, diff --git a/packages/ses/src/tame-domains.js b/packages/ses/src/tame-domains.js new file mode 100644 index 0000000000..ee7f1df2d7 --- /dev/null +++ b/packages/ses/src/tame-domains.js @@ -0,0 +1,44 @@ +// @ts-check + +import { + TypeError, + globalThis, + getOwnPropertyDescriptor, + defineProperty, +} from './commons.js'; + +export function tameDomains(domainTaming = 'safe') { + if (domainTaming !== 'safe' && domainTaming !== 'unsafe') { + throw new TypeError(`unrecognized domainTaming ${domainTaming}`); + } + + if (domainTaming === 'unsafe') { + return; + } + + // Protect against the hazard presented by Node.js domains. + if (typeof globalThis.process === 'object' && globalThis.process !== null) { + // Check whether domains were initialized. + const domainDescriptor = getOwnPropertyDescriptor( + globalThis.process, + 'domain', + ); + if (domainDescriptor !== undefined && domainDescriptor.get !== undefined) { + throw new TypeError( + `SES failed to lockdown, Node.js domains have been initialized (SES_NO_DOMAINS)`, + ); + } + // Prevent domains from initializing. + // This is clunky because the exception thrown from the domains package does + // not direct the user's gaze toward a knowledge base about the problem. + // The domain module merely throws an exception when it attempts to define + // the domain property of the process global during its initialization. + // We have no better recourse because Node.js uses defineProperty too. + defineProperty(globalThis.process, 'domain', { + value: null, + configurable: false, + writable: false, + enumerable: false, + }); + } +} diff --git a/packages/ses/test/test-tame-domains-after-lockdown.js b/packages/ses/test/test-tame-domains-after-lockdown.js new file mode 100644 index 0000000000..c7506e3a1e --- /dev/null +++ b/packages/ses/test/test-tame-domains-after-lockdown.js @@ -0,0 +1,16 @@ +import '../index.js'; +import test from 'ava'; + +lockdown({ domainTaming: 'safe' }); + +test('import domains after lockdown', async t => { + try { + await import('domain'); + t.fail('importing domain should throw'); + } catch (error) { + // This assertion omitted to avoid coupling to a specific engine. + // t.is(error.message, 'Cannot redefine property: domain'); + t.log(error); + t.pass(); + } +}); diff --git a/packages/ses/test/test-tame-domains-before-lockdown.js b/packages/ses/test/test-tame-domains-before-lockdown.js new file mode 100644 index 0000000000..faa307f0ec --- /dev/null +++ b/packages/ses/test/test-tame-domains-before-lockdown.js @@ -0,0 +1,7 @@ +import '../index.js'; +import test from 'ava'; +import 'domain'; + +test('lockdown after domains introduced', async t => { + t.throws(() => lockdown({ domainTaming: 'safe' })); +});