-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ses): Add Node.js domain hazard mitigation #850
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# 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. | ||
|
||
To work-around this restriction and explicitly remain vulnerable to domains, | ||
call `lockdown` with the [domainTaming][] option set to `'unsafe'`. | ||
|
||
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. | ||
|
||
[domainTaming]: https://github.com/endojs/endo/blob/master/packages/ses/lockdown-options.md#domaintaming-options |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
// @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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an important distinction and I’ve added a comment to the effect. Node.js initializes process with a process.domain === null and replaces this with a get/set pair when |
||
// The domain descriptor on Node.js initially has value: null, which | ||
// becomes a get, set pair after domains initialize. | ||
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, | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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(); | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' })); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text should also mention
domainTaming
and link to an explanation.