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

--policy-integrity #28734

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 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 doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,14 @@ unless either the `--pending-deprecation` command line flag, or the
are used to provide a kind of selective "early warning" mechanism that
developers may leverage to detect deprecated API usage.

### `--policy-integrity`
Copy link
Member

Choose a reason for hiding this comment

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

This should be --policy-integrity=checksum with some explanation about the expected format of the argument (checksum or a more appropriate word if the argument isn't actually a checksum).

Copy link
Member Author

Choose a reason for hiding this comment

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

well it is an SRI string so we could link to that somewhere but the SRI spec is kind of not great for explaining that, would linking to mdn be fine?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think linking to mdn should be ok. (and rename my suggested parameter name to sri instead of checksum).

<!-- YAML
added: TODO
bmeck marked this conversation as resolved.
Show resolved Hide resolved
-->

Instructs node to error prior to running any code if the policy does not have
bmeck marked this conversation as resolved.
Show resolved Hide resolved
the specified integrity.

### `--preserve-symlinks`
<!-- YAML
added: v6.3.0
Expand Down Expand Up @@ -959,6 +967,7 @@ Node.js options that are allowed are:
- `--no-warnings`
- `--openssl-config`
- `--pending-deprecation`
- `--policy-integrity`
- `--preserve-symlinks-main`
- `--preserve-symlinks`
- `--prof-process`
Expand Down
9 changes: 9 additions & 0 deletions doc/api/policy.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ node --experimental-policy=policy.json app.js
The policy manifest will be used to enforce constraints on code loaded by
Node.js.

In order to mitigate tampering with policy files on disk, an integrity for
the policy file itself may be provided via `--policy-integrity`.
This allows running `node` and asserting the policy file contents
even if the file is changed on disk.

```sh
node --experimental-policy=policy.json --policy-integrity="sha384-SggXRQHwCG8g+DktYYzxkXRIkTiEYWBHqev0xnpCxYlqMBufKZHAHQM3/boDaI/0" app.js
```

## Features

### Error Behavior
Expand Down
3 changes: 3 additions & 0 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,9 @@ Among other uses, this can be used to enable FIPS-compliant crypto if Node.js is
.It Fl -pending-deprecation
Emit pending deprecation warnings.
.
.It Fl -experimental-policy
bmeck marked this conversation as resolved.
Show resolved Hide resolved
Instructs node to error prior to running any code if the policy does not have the specified integrity.
bmeck marked this conversation as resolved.
Show resolved Hide resolved
.
.It Fl -preserve-symlinks
Instructs the module loader to preserve symbolic links when resolving and caching modules other than the main module.
.
Expand Down
27 changes: 27 additions & 0 deletions lib/internal/bootstrap/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const { Object, SafeWeakMap } = primordials;

const { getOptionValue } = require('internal/options');
const { Buffer } = require('buffer');
const { ERR_MANIFEST_ASSERT_INTEGRITY } = require('internal/errors').codes;

function prepareMainThreadExecution(expandArgv1 = false) {
// Patch the process object with legacy properties and normalizations
Expand Down Expand Up @@ -332,6 +333,32 @@ function initializePolicy() {
}
const fs = require('fs');
const src = fs.readFileSync(manifestURL, 'utf8');
const experimentalPolicyIntegrity = getOptionValue('--policy-integrity');
if (experimentalPolicyIntegrity) {
const SRI = require('internal/policy/sri');
const { createHash, timingSafeEqual } = require('crypto');
const realIntegrities = new Map();
const integrityEntries = SRI.parse(experimentalPolicyIntegrity);
let foundMatch = false;
for (var i = 0; i < integrityEntries.length; i++) {
const {
algorithm,
value: expected
} = integrityEntries[i];
const hash = createHash(algorithm);
hash.update(src);
const digest = hash.digest();
if (digest.length === expected.length &&
timingSafeEqual(digest, expected)) {
foundMatch = true;
break;
}
realIntegrities.set(algorithm, digest.toString('base64'));
}
if (!foundMatch) {
throw new ERR_MANIFEST_ASSERT_INTEGRITY(manifestURL, realIntegrities);
}
}
require('internal/process/policy')
.setup(src, manifestURL.href);
}
Expand Down
16 changes: 16 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,13 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
if (!userland_loader.empty() && !experimental_modules) {
errors->push_back("--loader requires --experimental-modules be enabled");
}
if (has_policy_integrity_string && experimental_policy.empty()) {
errors->push_back("--policy-integrity requires "
"--experimental-policy be enabled");
}
if (has_policy_integrity_string && experimental_policy_integrity.empty()) {
errors->push_back("--policy-integrity cannot be empty");
}

if (!module_type.empty()) {
if (!experimental_modules) {
Expand Down Expand Up @@ -313,6 +320,15 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"security policy",
&EnvironmentOptions::experimental_policy,
kAllowedInEnvironment);
AddOption("[has_policy_integrity_string]",
"",
&EnvironmentOptions::has_policy_integrity_string);
AddOption("--policy-integrity",
"ensure the security policy contents match "
"the specified integrity",
&EnvironmentOptions::experimental_policy_integrity,
kAllowedInEnvironment);
Implies("--policy-integrity", "[has_policy_integrity_string]");
AddOption("--experimental-repl-await",
"experimental await keyword support in REPL",
&EnvironmentOptions::experimental_repl_await,
Expand Down
2 changes: 2 additions & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ class EnvironmentOptions : public Options {
bool experimental_wasm_modules = false;
std::string module_type;
std::string experimental_policy;
std::string experimental_policy_integrity;
bool has_policy_integrity_string;
bool experimental_repl_await = false;
bool experimental_vm_modules = false;
bool expose_internals = false;
Expand Down
7 changes: 7 additions & 0 deletions test/fixtures/policy/dep-policy.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"resources": {
"./dep.js": {
"integrity": "sha512-7CMcc2oytFfMnGQaXbJk84gYWF2J7p/fmWPW7dsnJyniD+vgxtK9VAZ/22UxFOA4q5d27RoGLxSqNZ/nGCJkMw=="
}
}
}
2 changes: 2 additions & 0 deletions test/fixtures/policy/dep.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
'use strict';
module.exports = 'The Secret Ingredient';
57 changes: 57 additions & 0 deletions test/parallel/test-policy-integrity-flag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const fixtures = require('../common/fixtures');

const assert = require('assert');
const { spawnSync } = require('child_process');
const fs = require('fs');
const crypto = require('crypto');

const depPolicy = fixtures.path('policy', 'dep-policy.json');
const dep = fixtures.path('policy', 'dep.js');

const emptyHash = crypto.createHash('sha512');
emptyHash.update('');
const emptySRI = `sha512-${emptyHash.digest('base64')}`;
const policyHash = crypto.createHash('sha512');
policyHash.update(fs.readFileSync(depPolicy));
const depPolicySRI = `sha512-${policyHash.digest('base64')}`;
{
const { status, stderr } = spawnSync(
process.execPath,
[
'--policy-integrity', emptySRI,
'--experimental-policy', depPolicy, dep,
]
);

assert.ok(stderr.includes('ERR_MANIFEST_ASSERT_INTEGRITY'));
assert.strictEqual(status, 1);
}
{
const { status, stderr } = spawnSync(
process.execPath,
[
'--policy-integrity', '',
'--experimental-policy', depPolicy, dep,
]
);

assert.ok(stderr.includes('--policy-integrity'));
assert.strictEqual(status, 9);
}
{
const { status } = spawnSync(
process.execPath,
[
'--policy-integrity', depPolicySRI,
'--experimental-policy', depPolicy, dep,
]
);

assert.strictEqual(status, 0);
bmeck marked this conversation as resolved.
Show resolved Hide resolved
}