Skip to content
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

Merged
merged 1 commit into from
Sep 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/ses/NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
21 changes: 21 additions & 0 deletions packages/ses/error-codes/SES_NO_DOMAINS.md
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)
Copy link
Contributor

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.


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
1 change: 1 addition & 0 deletions packages/ses/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export interface LockdownOptions {
stackFiltering?: 'concise' | 'verbose';
overrideTaming?: 'moderate' | 'min' | 'severe';
overrideDebug?: Array<string>;
domainTaming?: 'safe' | 'unsafe';
__allowUnsafeMonkeyPatching__?: 'safe' | 'unsafe';
}

Expand Down
1 change: 1 addition & 0 deletions packages/ses/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
49 changes: 40 additions & 9 deletions packages/ses/lockdown-options.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,16 @@ Each option is explained in its own section below.

| option | default setting | other settings | about |
|------------------|------------------|----------------|-------|
| `regExpTaming` | `'safe'` | `'unsafe'` | `RegExp.prototype.compile` |
| `localeTaming` | `'safe'` | `'unsafe'` | `toLocaleString` |
| `consoleTaming` | `'safe'` | `'unsafe'` | deep stacks |
| `errorTaming` | `'safe'` | `'unsafe'` | `errorInstance.stack` |
| `errorTrapping` | `'platform'` | `'exit'` `'abort'` `'report'` | handling of uncaught exceptions |
| `stackFiltering` | `'concise'` | `'verbose'` | deep stacks signal/noise |
| `overrideTaming` | `'moderate'` | `'min'` or `'severe'` | override mistake antidote |
| `overrideDebug` | `[]` | array of property names | detect override mistake |
| `__allowUnsafeMonkeyPatching__` | `'safe'` | `'unsafe'` | run unsafe code unsafely |
| `regExpTaming` | `'safe'` | `'unsafe'` | `RegExp.prototype.compile` |
| `localeTaming` | `'safe'` | `'unsafe'` | `toLocaleString` |
| `consoleTaming` | `'safe'` | `'unsafe'` | deep stacks |
| `errorTaming` | `'safe'` | `'unsafe'` | `errorInstance.stack` |
| `errorTrapping` | `'platform'` | `'exit'` `'abort'` `'report'` | handling of uncaught exceptions |
| `stackFiltering` | `'concise'` | `'verbose'` | deep stacks signal/noise |
| `overrideTaming` | `'moderate'` | `'min'` or `'severe'` | override mistake antidote |
| `overrideDebug` | `[]` | array of property names | detect override mistake |
| `domainTaming` | `'unsafe'` | `'safe'` | bans the Node.js `domain` module, to be `'safe'` by default in the next breaking-changes release |
| `__allowUnsafeMonkeyPatching__` | `'safe'` | `'unsafe'` | run unsafe code unsafely |

The options `mathTaming` and `dateTaming` are deprecated.
`Math.random`, `Date.now`, and the `new Date()` are disabled within
Expand Down Expand Up @@ -604,6 +605,36 @@ Error#1: Override property constructor
at packages/ses/test/test-enable-property-overrides-severe-debug.js:14:3
```

## `domainTaming` Options

The deprecated Node.js `domain` module adds `domain` properties to callbacks
and promises.
These `domain` properties allow communication between client programs that
`lockdown` otherwise isolates.

To disable this safety feature, call `lockdown` with `domainTaming` set to
`'unsafe'` explicitly.

```js
lockdown({
domainTaming: 'unsafe'
});
```

The `domainTaming` option, when set to `'safe'`, protects programs
by detecting whether the `'domain'` module has been initialized, and by laying
a trap that prevents it from initializing later.

The `domain` module adds a `domain` object to the `process` global object,
so it's possible to detect without consulting the module system.
Defining a non-configurable `domain` property on the `process` object
causes any later attempt to initialize domains to fail loudly.

Unfortunately, some modules ultimately depend on the `domain` module,
even when they do not actively use its features.
To run multi-tenant applications safely, these dependencies must be carefully
fixed or avoided.

## `__allowUnsafeMonkeyPatching__` Options

Sometimes SES is used where SES's safety is not required. Some libraries
Expand Down
6 changes: 6 additions & 0 deletions packages/ses/src/lockdown-shim.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -139,6 +140,7 @@ export const repairIntrinsics = (
overrideTaming = 'moderate',
overrideDebug = [],
stackFiltering = 'concise',
domainTaming = 'unsafe', // TODO become 'safe' by default in next-breaking-release.
__allowUnsafeMonkeyPatching__ = 'safe',

...extraOptions
Expand Down Expand Up @@ -173,6 +175,7 @@ export const repairIntrinsics = (
overrideTaming,
overrideDebug,
stackFiltering,
domainTaming,
__allowUnsafeMonkeyPatching__,
};

Expand Down Expand Up @@ -220,6 +223,9 @@ export const repairIntrinsics = (
/**
* 1. TAME powers & gather intrinsics first.
*/

tameDomains(domainTaming);

const {
addIntrinsics,
completePrototypes,
Expand Down
46 changes: 46 additions & 0 deletions packages/ses/src/tame-domains.js
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the get test?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 domain initializes.

// 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,
});
}
}
16 changes: 16 additions & 0 deletions packages/ses/test/test-tame-domains-after-lockdown.js
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();
}
});
7 changes: 7 additions & 0 deletions packages/ses/test/test-tame-domains-before-lockdown.js
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' }));
});