Skip to content

Commit

Permalink
policy: manifest with subresource integrity checks
Browse files Browse the repository at this point in the history
This enables code loaded via the module system to be checked for
integrity to ensure the code loaded matches expectations.

PR-URL: #23834
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
  • Loading branch information
bmeck committed Jan 17, 2019
1 parent 7b6e9ae commit 9d5fbeb
Show file tree
Hide file tree
Showing 16 changed files with 779 additions and 5 deletions.
7 changes: 7 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ added: v8.5.0

Enable experimental ES module support and caching modules.

### `--experimental-policy`
<!-- YAML
added: TODO
-->

Use the specified file as a security policy.

### `--experimental-repl-await`
<!-- YAML
added: v10.0.0
Expand Down
42 changes: 42 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1380,6 +1380,39 @@ An attempt was made to open an IPC communication channel with a synchronously
forked Node.js process. See the documentation for the [`child_process`][] module
for more information.

<a id="ERR_MANIFEST_ASSERT_INTEGRITY"></a>
### ERR_MANIFEST_ASSERT_INTEGRITY

An attempt was made to load a resource, but the resource did not match the
integrity defined by the policy manifest. See the documentation for [policy]
manifests for more information.

<a id="ERR_MANIFEST_INTEGRITY_MISMATCH"></a>
### ERR_MANIFEST_INTEGRITY_MISMATCH

An attempt was made to load a policy manifest, but the manifest had multiple
entries for a resource which did not match each other. Update the manifest
entries to match in order to resolve this error. See the documentation for
[policy] manifests for more information.

<a id="ERR_MANIFEST_PARSE_POLICY"></a>
### ERR_MANIFEST_PARSE_POLICY

An attempt was made to load a policy manifest, but the manifest was unable to
be parsed. See the documentation for [policy] manifests for more information.

<a id="ERR_MANIFEST_TDZ"></a>
### ERR_MANIFEST_TDZ

An attempt was made to read from a policy manifest, but the manifest
initialization has not yet taken place. This is likely a bug in Node.js.

<a id="ERR_MANIFEST_UNKNOWN_ONERROR"></a>
### ERR_MANIFEST_UNKNOWN_ONERROR

A policy manifest was loaded, but had an unknown value for its "onerror"
behavior. See the documentation for [policy] manifests for more information.

<a id="ERR_MEMORY_ALLOCATION_FAILED"></a>
### ERR_MEMORY_ALLOCATION_FAILED

Expand Down Expand Up @@ -1590,6 +1623,13 @@ An attempt was made to operate on an already closed socket.

A call was made and the UDP subsystem was not running.

<a id="ERR_SRI_PARSE"></a>
### ERR_SRI_PARSE

A string was provided for a Subresource Integrity check, but was unable to be
parsed. Check the format of integrity attributes by looking at the
[Subresource Integrity specification][].

<a id="ERR_STREAM_CANNOT_PIPE"></a>
### ERR_STREAM_CANNOT_PIPE

Expand Down Expand Up @@ -2229,7 +2269,9 @@ such as `process.stdout.on('data')`.
[domains]: domain.html
[event emitter-based]: events.html#events_class_eventemitter
[file descriptors]: https://en.wikipedia.org/wiki/File_descriptor
[policy]: policy.html
[stream-based]: stream.html
[syscall]: http://man7.org/linux/man-pages/man2/syscalls.2.html
[Subresource Integrity specification]: https://www.w3.org/TR/SRI/#the-integrity-attribute
[try-catch]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch
[vm]: vm.html
1 change: 1 addition & 0 deletions doc/api/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
* [OS](os.html)
* [Path](path.html)
* [Performance Hooks](perf_hooks.html)
* [Policies](policy.html)
* [Process](process.html)
* [Punycode](punycode.html)
* [Query Strings](querystring.html)
Expand Down
104 changes: 104 additions & 0 deletions doc/api/policy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# Policies

<!--introduced_in=TODO-->
<!-- type=misc -->

> Stability: 1 - Experimental
<!-- name=policy -->

Node.js contains experimental support for creating policies on loading code.

Policies are a security feature intended to allow guarantees
about what code Node.js is able to load. The use of policies assumes
safe practices for the policy files such as ensuring that policy
files cannot be overwritten by the Node.js application by using
file permissions.

A best practice would be to ensure that the policy manifest is read only for
the running Node.js application, and that the file cannot be changed
by the running Node.js application in any way. A typical setup would be to
create the policy file as a different user id than the one running Node.js
and granting read permissions to the user id running Node.js.

## Enabling

<!-- type=misc -->

The `--experimental-policy` flag can be used to enable features for policies
when loading modules.

Once this has been set, all modules must conform to a policy manifest file
passed to the flag:

```sh
node --experimental-policy=policy.json app.js
```

The policy manifest will be used to enforce constraints on code loaded by
Node.js.

## Features

### Error Behavior

When a policy check fails, Node.js by default will throw an error.
It is possible to change the error behavior to one of a few possibilities
by defining an "onerror" field in a policy manifest. The following values are
available to change the behavior:

* `"exit"` - will exit the process immediately.
No cleanup code will be allowed to run.
* `"log"` - will log the error at the site of the failure.
* `"throw"` (default) - will throw a JS error at the site of the failure.

```json
{
"onerror": "log",
"resources": {
"./app/checked.js": {
"integrity": "sha384-SggXRQHwCG8g+DktYYzxkXRIkTiEYWBHqev0xnpCxYlqMBufKZHAHQM3/boDaI/0"
}
}
}
```

### Integrity Checks

Policy files must use integrity checks with Subresource Integrity strings
compatible with the browser
[integrity attribute](https://www.w3.org/TR/SRI/#the-integrity-attribute)
associated with absolute URLs.

When using `require()` all resources involved in loading are checked for
integrity if a policy manifest has been specified. If a resource does not match
the integrity listed in the manifest, an error will be thrown.

An example policy file that would allow loading a file `checked.js`:

```json
{
"resources": {
"./app/checked.js": {
"integrity": "sha384-SggXRQHwCG8g+DktYYzxkXRIkTiEYWBHqev0xnpCxYlqMBufKZHAHQM3/boDaI/0"
}
}
}
```

Each resource listed in the policy manifest can be of one the following
formats to determine its location:

1. A [relative url string][] to a resource from the manifest such as `./resource.js`, `../resource.js`, or `/resource.js`.
2. A complete url string to a resource such as `file:///resource.js`.

When loading resources the entire URL must match including search parameters
and hash fragment. `./a.js?b` will not be used when attempting to load
`./a.js` and vice versa.

In order to generate integrity strings, a script such as
`printf "sha384-$(cat checked.js | openssl dgst -sha384 -binary | base64)"`
can be used.


[relative url string]: https://url.spec.whatwg.org/#relative-url-with-fragment-string
3 changes: 3 additions & 0 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ Requires Node.js to be built with
.It Fl -experimental-modules
Enable experimental ES module support and caching modules.
.
.It Fl -experimental-policy
Use the specified file as a security policy.
.
.It Fl -experimental-repl-await
Enable experimental top-level
.Sy await
Expand Down
22 changes: 22 additions & 0 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,28 @@ function startup() {
mainThreadSetup.setupChildProcessIpcChannel();
}

// TODO(joyeecheung): move this down further to get better snapshotting
if (getOptionValue('[has_experimental_policy]')) {
process.emitWarning('Policies are experimental.',
'ExperimentalWarning');
const experimentalPolicy = getOptionValue('--experimental-policy');
const { pathToFileURL, URL } = NativeModule.require('url');
// URL here as it is slightly different parsing
// no bare specifiers for now
let manifestURL;
if (NativeModule.require('path').isAbsolute(experimentalPolicy)) {
manifestURL = new URL(`file:///${experimentalPolicy}`);
} else {
const cwdURL = pathToFileURL(process.cwd());
cwdURL.pathname += '/';
manifestURL = new URL(experimentalPolicy, cwdURL);
}
const fs = NativeModule.require('fs');
const src = fs.readFileSync(manifestURL, 'utf8');
NativeModule.require('internal/process/policy')
.setup(src, manifestURL.href);
}

const browserGlobals = !process._noBrowserGlobals;
if (browserGlobals) {
setupGlobalTimeouts();
Expand Down
25 changes: 25 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -818,6 +818,28 @@ E('ERR_IPC_CHANNEL_CLOSED', 'Channel closed', Error);
E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected', Error);
E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe', Error);
E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks', Error);
E('ERR_MANIFEST_ASSERT_INTEGRITY',
(moduleURL, realIntegrities) => {
let msg = `The content of "${
moduleURL
}" does not match the expected integrity.`;
if (realIntegrities.size) {
const sri = [...realIntegrities.entries()].map(([alg, dgs]) => {
return `${alg}-${dgs}`;
}).join(' ');
msg += ` Integrities found are: ${sri}`;
} else {
msg += ' The resource was not found in the policy.';
}
return msg;
}, Error);
E('ERR_MANIFEST_INTEGRITY_MISMATCH',
'Manifest resource %s has multiple entries but integrity lists do not match',
SyntaxError);
E('ERR_MANIFEST_TDZ', 'Manifest initialization has not yet run', Error);
E('ERR_MANIFEST_UNKNOWN_ONERROR',
'Manifest specified unknown error behavior "%s".',
SyntaxError);
E('ERR_METHOD_NOT_IMPLEMENTED', 'The %s method is not implemented', Error);
E('ERR_MISSING_ARGS',
(...args) => {
Expand Down Expand Up @@ -889,6 +911,9 @@ E('ERR_SOCKET_BUFFER_SIZE',
E('ERR_SOCKET_CANNOT_SEND', 'Unable to send data', Error);
E('ERR_SOCKET_CLOSED', 'Socket is closed', Error);
E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running', Error);
E('ERR_SRI_PARSE',
'Subresource Integrity string %s had an unexpected at %d',
SyntaxError);
E('ERR_STREAM_CANNOT_PIPE', 'Cannot pipe, not readable', Error);
E('ERR_STREAM_DESTROYED', 'Cannot call %s after a stream was destroyed', Error);
E('ERR_STREAM_NULL_VALUES', 'May not write null values to stream', TypeError);
Expand Down
37 changes: 32 additions & 5 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@
'use strict';

const { NativeModule } = require('internal/bootstrap/loaders');
const util = require('util');
const { pathToFileURL } = require('internal/url');
const util = require('util');
const vm = require('vm');
const assert = require('assert').ok;
const fs = require('fs');
Expand All @@ -45,6 +45,9 @@ const { getOptionValue } = require('internal/options');
const preserveSymlinks = getOptionValue('--preserve-symlinks');
const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
const experimentalModules = getOptionValue('--experimental-modules');
const manifest = getOptionValue('[has_experimental_policy]') ?
require('internal/process/policy').manifest :
null;

const {
ERR_INVALID_ARG_VALUE,
Expand Down Expand Up @@ -168,6 +171,11 @@ function readPackage(requestPath) {
return false;
}

if (manifest) {
const jsonURL = pathToFileURL(jsonPath);
manifest.assertIntegrity(jsonURL, json);
}

try {
return packageMainCache[requestPath] = JSON.parse(json).main;
} catch (e) {
Expand Down Expand Up @@ -676,6 +684,10 @@ function normalizeReferrerURL(referrer) {
// the file.
// Returns exception, if any.
Module.prototype._compile = function(content, filename) {
if (manifest) {
const moduleURL = pathToFileURL(filename);
manifest.assertIntegrity(moduleURL, content);
}

content = stripShebang(content);

Expand Down Expand Up @@ -715,11 +727,14 @@ Module.prototype._compile = function(content, filename) {
var depth = requireDepth;
if (depth === 0) stat.cache = new Map();
var result;
var exports = this.exports;
var thisValue = exports;
var module = this;
if (inspectorWrapper) {
result = inspectorWrapper(compiledWrapper, this.exports, this.exports,
require, this, filename, dirname);
result = inspectorWrapper(compiledWrapper, thisValue, exports,
require, module, filename, dirname);
} else {
result = compiledWrapper.call(this.exports, this.exports, require, this,
result = compiledWrapper.call(thisValue, exports, require, module,
filename, dirname);
}
if (depth === 0) stat.cache = null;
Expand All @@ -736,7 +751,13 @@ Module._extensions['.js'] = function(module, filename) {

// Native extension for .json
Module._extensions['.json'] = function(module, filename) {
var content = fs.readFileSync(filename, 'utf8');
const content = fs.readFileSync(filename, 'utf8');

if (manifest) {
const moduleURL = pathToFileURL(filename);
manifest.assertIntegrity(moduleURL, content);
}

try {
module.exports = JSON.parse(stripBOM(content));
} catch (err) {
Expand All @@ -748,6 +769,12 @@ Module._extensions['.json'] = function(module, filename) {

// Native extension for .node
Module._extensions['.node'] = function(module, filename) {
if (manifest) {
const content = fs.readFileSync(filename);
const moduleURL = pathToFileURL(filename);
manifest.assertIntegrity(moduleURL, content);
}
// be aware this doesn't use `content`
return process.dlopen(module, path.toNamespacedPath(filename));

This comment has been minimized.

Copy link
@addaleax

addaleax Jan 21, 2019

Member

This looks like a TOCTOU race condition which we should be very careful with security-relevant code. Are we really okay with this?

This comment has been minimized.

Copy link
@bmeck

bmeck Jan 21, 2019

Author Member

All code loaded prior to this has been labeled as having the proper integrity, integrity does not ensure the stability of the system in the face of mutation of the runtime. I think this does have problems, but find them to be within expectations. Authorizing usage of .node files is inherently dangerous and allowing arbitrary syscalls. Is there some method of mitigating this that you might suggest using just integrities? On their own integrity checks are insufficient to fully secure Node.js and multiple other layers will be needed. I felt that in light of the other capabilities requires, this was acceptable. Other ways to prevent this timing problem are not available since loading shared libraries from memory is deprecated or missing from modern OS. Paths involving code signing have faced some skepticism as well as laid out in https://docs.google.com/document/d/1h__FmXsEWRuNrzAV_l3Iw9i_z8fCXSokGfBiW8-nDNg/edit?ts=5c1adaed#heading=h.a3fvn8g1h48h . We could attempt to integrate with OS signing APIs, but that is kept to a separate feature from integrities.

This comment has been minimized.

Copy link
@addaleax

addaleax Jan 21, 2019

Member

Yeah, I’m just saying, this is one item where we might be running into trouble. I agree in that I currently don’t see any better way here.

This comment has been minimized.

Copy link
@bmeck

bmeck Jan 21, 2019

Author Member

@addaleax policies also do not guard child processes. The need for securing at the OS level is something I would consider independent from integrities for now. If we are to ban C++ loading due to timing issues, we also are likely to want to ban child processes that can be used to escape integrity checks.

};

Expand Down
Loading

0 comments on commit 9d5fbeb

Please sign in to comment.