Skip to content

Commit 2eeb44f

Browse files
bmecktargos
authored andcommitted
policy: add policy-integrity to mitigate policy tampering
PR-URL: #28734 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent cf811ec commit 2eeb44f

9 files changed

Lines changed: 148 additions & 0 deletions

File tree

‎doc/api/cli.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -457,6 +457,17 @@ unless either the `--pending-deprecation` command line flag, or the
457457
are used to provide a kind of selective "early warning" mechanism that
458458
developers may leverage to detect deprecated API usage.
459459

460+
### `--policy-integrity=sri`
461+
<!-- YAML
462+
added: REPLACEME
463+
-->
464+
465+
> Stability: 1 - Experimental
466+
467+
Instructs Node.js to error prior to running any code if the policy does not have
468+
the specified integrity. It expects a [Subresource Integrity][] string as a
469+
parameter.
470+
460471
### `--preserve-symlinks`
461472
<!-- YAML
462473
added: v6.3.0
@@ -992,6 +1003,7 @@ Node.js options that are allowed are:
9921003
-`--no-warnings`
9931004
-`--openssl-config`
9941005
-`--pending-deprecation`
1006+
-`--policy-integrity`
9951007
-`--preserve-symlinks-main`
9961008
-`--preserve-symlinks`
9971009
-`--prof-process`
@@ -1196,6 +1208,7 @@ greater than `4` (its current default value). For more information, see the
11961208
[Chrome DevTools Protocol]: https://chromedevtools.github.io/devtools-protocol/
11971209
[REPL]: repl.html
11981210
[ScriptCoverage]: https://chromedevtools.github.io/devtools-protocol/tot/Profiler#type-ScriptCoverage
1211+
[Subresource Integrity]: https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity
11991212
[V8 JavaScript code coverage]: https://v8project.blogspot.com/2017/12/javascript-code-coverage.html
12001213
[customizing esm specifier resolution]: esm.html#esm_customizing_esm_specifier_resolution_algorithm
12011214
[debugger]: debugger.html

‎doc/api/policy.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,15 @@ node --experimental-policy=policy.json app.js
3838
The policy manifest will be used to enforce constraints on code loaded by
3939
Node.js.
4040

41+
In order to mitigate tampering with policy files on disk, an integrity for
42+
the policy file itself may be provided via `--policy-integrity`.
43+
This allows running `node` and asserting the policy file contents
44+
even if the file is changed on disk.
45+
46+
```sh
47+
node --experimental-policy=policy.json --policy-integrity="sha384-SggXRQHwCG8g+DktYYzxkXRIkTiEYWBHqev0xnpCxYlqMBufKZHAHQM3/boDaI/0" app.js
48+
```
49+
4150
## Features
4251

4352
### Error Behavior

‎doc/node.1‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,9 @@ Among other uses, this can be used to enable FIPS-compliant crypto if Node.js is
231231
.ItFl-pending-deprecation
232232
Emit pending deprecation warnings.
233233
.
234+
.ItFl-policy-integrityNs=NsArsri
235+
Instructs Node.js to error prior to running any code if the policy does not have the specified integrity. It expects a Subresource Integrity string as a parameter.
236+
.
234237
.ItFl-preserve-symlinks
235238
Instructs the module loader to preserve symbolic links when resolving and caching modules other than the main module.
236239
.

‎lib/internal/bootstrap/pre_execution.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ const { Object, SafeWeakMap } = primordials;
44

55
const{ getOptionValue }=require('internal/options');
66
const{ Buffer }=require('buffer');
7+
const{ERR_MANIFEST_ASSERT_INTEGRITY}=require('internal/errors').codes;
78

89
functionprepareMainThreadExecution(expandArgv1=false){
910
// Patch the process object with legacy properties and normalizations
@@ -332,6 +333,32 @@ function initializePolicy() {
332333
}
333334
constfs=require('fs');
334335
constsrc=fs.readFileSync(manifestURL,'utf8');
336+
constexperimentalPolicyIntegrity=getOptionValue('--policy-integrity');
337+
if(experimentalPolicyIntegrity){
338+
constSRI=require('internal/policy/sri');
339+
const{ createHash, timingSafeEqual }=require('crypto');
340+
constrealIntegrities=newMap();
341+
constintegrityEntries=SRI.parse(experimentalPolicyIntegrity);
342+
letfoundMatch=false;
343+
for(vari=0;i<integrityEntries.length;i++){
344+
const{
345+
algorithm,
346+
value: expected
347+
}=integrityEntries[i];
348+
consthash=createHash(algorithm);
349+
hash.update(src);
350+
constdigest=hash.digest();
351+
if(digest.length===expected.length&&
352+
timingSafeEqual(digest,expected)){
353+
foundMatch=true;
354+
break;
355+
}
356+
realIntegrities.set(algorithm,digest.toString('base64'));
357+
}
358+
if(!foundMatch){
359+
thrownewERR_MANIFEST_ASSERT_INTEGRITY(manifestURL,realIntegrities);
360+
}
361+
}
335362
require('internal/process/policy')
336363
.setup(src,manifestURL.href);
337364
}

‎src/node_options.cc‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,13 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
116116
if (!userland_loader.empty() && !experimental_modules) {
117117
errors->push_back("--loader requires --experimental-modules be enabled");
118118
}
119+
if (has_policy_integrity_string && experimental_policy.empty()) {
120+
errors->push_back("--policy-integrity requires "
121+
"--experimental-policy be enabled");
122+
}
123+
if (has_policy_integrity_string && experimental_policy_integrity.empty()) {
124+
errors->push_back("--policy-integrity cannot be empty");
125+
}
119126

120127
if (!module_type.empty()) {
121128
if (!experimental_modules) {
@@ -321,6 +328,15 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
321328
"security policy",
322329
&EnvironmentOptions::experimental_policy,
323330
kAllowedInEnvironment);
331+
AddOption("[has_policy_integrity_string]",
332+
"",
333+
&EnvironmentOptions::has_policy_integrity_string);
334+
AddOption("--policy-integrity",
335+
"ensure the security policy contents match "
336+
"the specified integrity",
337+
&EnvironmentOptions::experimental_policy_integrity,
338+
kAllowedInEnvironment);
339+
Implies("--policy-integrity", "[has_policy_integrity_string]");
324340
AddOption("--experimental-repl-await",
325341
"experimental await keyword support in REPL",
326342
&EnvironmentOptions::experimental_repl_await,

‎src/node_options.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,8 @@ class EnvironmentOptions : public Options {
106106
bool experimental_wasm_modules = false;
107107
std::string module_type;
108108
std::string experimental_policy;
109+
std::string experimental_policy_integrity;
110+
bool has_policy_integrity_string;
109111
bool experimental_repl_await = false;
110112
bool experimental_vm_modules = false;
111113
bool expose_internals = false;
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"resources": {
3+
"./dep.js": {
4+
"integrity": "sha512-7CMcc2oytFfMnGQaXbJk84gYWF2J7p/fmWPW7dsnJyniD+vgxtK9VAZ/22UxFOA4q5d27RoGLxSqNZ/nGCJkMw== sha512-scgN9Td0bGMlGH2lUHvEeHtz92Hx6AO+sYhU3WRI6bn3jEUCXbXJs68nOOsGzRWR7a2tbqGoETnOCpHHf1Njhw=="
5+
}
6+
}
7+
}

‎test/fixtures/policy/dep.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
'use strict';
2+
module.exports='The Secret Ingredient';
Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
'use strict';
2+
3+
constcommon=require('../common');
4+
if(!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
constfixtures=require('../common/fixtures');
8+
9+
constassert=require('assert');
10+
const{ spawnSync }=require('child_process');
11+
constfs=require('fs');
12+
constcrypto=require('crypto');
13+
14+
constdepPolicy=fixtures.path('policy','dep-policy.json');
15+
constdep=fixtures.path('policy','dep.js');
16+
17+
constemptyHash=crypto.createHash('sha512');
18+
emptyHash.update('');
19+
constemptySRI=`sha512-${emptyHash.digest('base64')}`;
20+
constpolicyHash=crypto.createHash('sha512');
21+
policyHash.update(fs.readFileSync(depPolicy));
22+
23+
/* eslint-disable max-len */
24+
// When using \n only
25+
constnixPolicySRI='sha512-u/nXI6UacK5fKDC2bopcgnuQY4JXJKlK3dESO3GIKKxwogVHjJqpF9rgk7Zw+TJXIc96xBUWKHuUgOzic8/4tQ==';
26+
// When \n is turned into \r\n
27+
constwindowsPolicySRI='sha512-OeyCPRo4OZMosHyquZXDHpuU1F4KzG9UHFnn12FMaHsvqFUt3TFZ+7wmZE7ThZ5rsQWkUjc9ZH0knGZ2e8BYPQ==';
28+
/* eslint-enable max-len */
29+
30+
constdepPolicySRI=`${nixPolicySRI}${windowsPolicySRI}`;
31+
console.dir({
32+
depPolicySRI,
33+
body: JSON.stringify(fs.readFileSync(depPolicy).toString('utf8'))
34+
});
35+
{
36+
const{ status, stderr }=spawnSync(
37+
process.execPath,
38+
[
39+
'--policy-integrity',emptySRI,
40+
'--experimental-policy',depPolicy,dep,
41+
]
42+
);
43+
44+
assert.ok(stderr.includes('ERR_MANIFEST_ASSERT_INTEGRITY'));
45+
assert.strictEqual(status,1);
46+
}
47+
{
48+
const{ status, stderr }=spawnSync(
49+
process.execPath,
50+
[
51+
'--policy-integrity','',
52+
'--experimental-policy',depPolicy,dep,
53+
]
54+
);
55+
56+
assert.ok(stderr.includes('--policy-integrity'));
57+
assert.strictEqual(status,9);
58+
}
59+
{
60+
const{ status, stderr }=spawnSync(
61+
process.execPath,
62+
[
63+
'--policy-integrity',depPolicySRI,
64+
'--experimental-policy',depPolicy,dep,
65+
]
66+
);
67+
68+
assert.strictEqual(status,0,`status: ${status}\nstderr: ${stderr}`);
69+
}

0 commit comments

Comments
 (0)