From 443b0362d7e0f95cc475fc2a386746b8f4f5af5c Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Thu, 18 Jul 2019 09:01:59 -0500 Subject: [PATCH 01/10] policy: add dependencies map for redirect and whitelisting --- doc/api/errors.md | 7 ++++ doc/api/policy.md | 57 +++++++++++++++++++++++++++++ lib/internal/errors.js | 3 ++ lib/internal/modules/cjs/helpers.js | 37 +++++++++++++++++-- lib/internal/modules/cjs/loader.js | 9 +++-- lib/internal/policy/manifest.js | 56 ++++++++++++++++++++++------ 6 files changed, 151 insertions(+), 18 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index f638f0ba905db6..bf63bc8fcb8593 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1432,6 +1432,13 @@ 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. + +### ERR_MANIFEST_DEPENDENCY_MISSING + +An attempt was made to load a resource, but the resource was not listed as a +dependency from the location that attempted to load it. See the documentation +for [policy] manifests for more information. + ### ERR_MANIFEST_INTEGRITY_MISMATCH diff --git a/doc/api/policy.md b/doc/api/policy.md index a1955f2b3ee835..5cfa04fbc11da4 100644 --- a/doc/api/policy.md +++ b/doc/api/policy.md @@ -109,5 +109,62 @@ In order to generate integrity strings, a script such as `printf "sha384-$(cat checked.js | openssl dgst -sha384 -binary | base64)"` can be used. +### Dependency Redirection + +An application may need to ship patched versions of software or to prevent +software from allowing all modules access to all other modules. In order to +do so redirection can be used. + +```json +{ + "builtins": [], + "resources": { + "./app/checked.js": { + "dependencies": { + "fs": true, + "os": "./app/node_modules/alt-os" + } + } + } +} +``` + +The dependencies are keyed by the requested string specifier and have values +of either `true` or a string pointing to a module that will be resolved. + +The specifier string does not perform any searching and must match exactly +what is provided to the `require()`. Therefore, multiple specifiers may be +needed in the policy if `require()` uses multiple different strings to point +to the same module (such as excluding the extension). + +If the value of the redirection is `true` the default searching algorithms will +be used to find the module. + +If the value of the redirection is a string, it will be resolved relative to +the manifest and them immediately be used without searching. + +Any specifier missing from the list of dependency will result in an error +according to the policy. + +#### Example: Patched Dependency + +Since a dependency can be redirected, you can provide attenuated or modified +forms of dependencies as fits your application. For example, you could log +data about timing of function durations by wrapping the original: + +```js +const original = require('fn'); +module.exports = function fn(...args) { + console.time(); + try { + return new.target ? + Reflect.construct(original, args) : + Reflect.apply(original, this, args); + } finally { + console.timeEnd(); + } +}; +``` + [relative url string]: https://url.spec.whatwg.org/#relative-url-with-fragment-string diff --git a/lib/internal/errors.js b/lib/internal/errors.js index c36f34eff40cab..4869d1b4dd37ac 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1033,6 +1033,9 @@ E('ERR_MANIFEST_ASSERT_INTEGRITY', } return msg; }, Error); +E('ERR_MANIFEST_DEPENDENCY_MISSING', + 'Manifest resource %s does not list %s as a dependency specifier', + Error); E('ERR_MANIFEST_INTEGRITY_MISMATCH', 'Manifest resource %s has multiple entries but integrity lists do not match', SyntaxError); diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 71abcbf880ebb1..abecd1deb4f3ee 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -1,19 +1,48 @@ 'use strict'; const { Object } = primordials; +const { + ERR_MANIFEST_DEPENDENCY_MISSING +} = require('internal/errors').codes; const { validateString } = require('internal/validators'); const path = require('path'); -const { pathToFileURL } = require('internal/url'); +const { pathToFileURL, fileURLToPath } = require('internal/url'); const { URL } = require('url'); // Invoke with makeRequireFunction(module) where |module| is the Module object // to use as the context for the require() function. -function makeRequireFunction(mod) { +// Use redirects to set up a mapping from a policy and restrict dependencies +function makeRequireFunction(mod, redirects) { const Module = mod.constructor; - function require(path) { - return mod.require(path); + let require; + if (redirects) { + const { map, reaction } = redirects; + require = function require(path) { + if (map.has(path)) { + const redirect = map.get(path); + if (redirect === true) { + return mod.require(path); + } else if (redirect) { + const parsed = new URL(redirect); + if (parsed.protocol === 'node:') { + return mod.require(parsed.pathname); + } else if (parsed.protocol === 'file:') { + return mod.require(fileURLToPath(parsed)); + } + } + } + reaction(new ERR_MANIFEST_DEPENDENCY_MISSING( + path, + mod.filename || mod.id)); + // In the case of log just fall back to same behavior as `true` + return mod.require(path); + }; + } else { + require = function require(path) { + return mod.require(path); + }; } function resolve(request, options) { diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 199405b0e24457..4f4be5173139e9 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -741,8 +741,11 @@ function wrapSafe(filename, content) { // the file. // Returns exception, if any. Module.prototype._compile = function(content, filename) { + let moduleURL; + let redirects; if (manifest) { - const moduleURL = pathToFileURL(filename); + moduleURL = pathToFileURL(filename); + redirects = manifest.getRedirects(moduleURL); manifest.assertIntegrity(moduleURL, content); } @@ -766,7 +769,7 @@ Module.prototype._compile = function(content, filename) { } } const dirname = path.dirname(filename); - const require = makeRequireFunction(this); + const require = makeRequireFunction(this, redirects); var result; const exports = this.exports; const thisValue = exports; @@ -855,7 +858,7 @@ function createRequireFromPath(filename) { m.filename = proxyPath; m.paths = Module._nodeModulePaths(m.path); - return makeRequireFunction(m); + return makeRequireFunction(m, null); } Module.createRequireFromPath = deprecate( diff --git a/lib/internal/policy/manifest.js b/lib/internal/policy/manifest.js index 0714b2294b637f..6205303d631745 100644 --- a/lib/internal/policy/manifest.js +++ b/lib/internal/policy/manifest.js @@ -1,11 +1,15 @@ 'use strict'; const { + SafeMap, SafeWeakMap, Object, RegExpPrototype, uncurryThis } = primordials; +const { + canBeRequiredByUsers +} = require('internal/bootstrap/loaders').NativeModule; const { ERR_MANIFEST_ASSERT_INTEGRITY, @@ -24,6 +28,7 @@ const BufferEquals = uncurryThis(Buffer.prototype.equals); const BufferToString = uncurryThis(Buffer.prototype.toString); const { entries } = Object; const kIntegrities = new SafeWeakMap(); +const kDependencies = new SafeWeakMap(); const kReactions = new SafeWeakMap(); const kRelativeURLStringPattern = /^\.{0,2}\//; const { getOptionValue } = require('internal/options'); @@ -52,34 +57,35 @@ class Manifest { const integrities = { __proto__: null, }; - const reactions = { + const dependencies = { __proto__: null, - integrity: REACTION_THROW, }; + let reaction = REACTION_THROW; if (obj.onerror) { const behavior = obj.onerror; if (behavior === 'throw') { } else if (behavior === 'exit') { - reactions.integrity = REACTION_EXIT; + reaction = REACTION_EXIT; } else if (behavior === 'log') { - reactions.integrity = REACTION_LOG; + reaction = REACTION_LOG; } else { throw new ERR_MANIFEST_UNKNOWN_ONERROR(behavior); } } - kReactions.set(this, Object.freeze(reactions)); + kReactions.set(this, reaction); const manifestEntries = entries(obj.resources); for (var i = 0; i < manifestEntries.length; i++) { let url = manifestEntries[i][0]; + const originalURL = url; + if (RegExpPrototype.test(kRelativeURLStringPattern, url)) { + url = new URL(url, manifestURL).href; + } const integrity = manifestEntries[i][1].integrity; if (integrity != null) { - debug(`Manifest contains integrity for url ${url}`); - if (RegExpPrototype.test(kRelativeURLStringPattern, url)) { - url = new URL(url, manifestURL).href; - } + debug(`Manifest contains integrity for url ${originalURL}`); const sri = Object.freeze(SRI.parse(integrity)); if (url in integrities) { @@ -109,16 +115,44 @@ class Manifest { } integrities[url] = sri; } + + const dependencyMap = manifestEntries[i][1].dependencies; + if (dependencyMap) { + dependencies[url] = new SafeMap(Object.entries(dependencyMap).map( + ([ from, to ]) => { + if (to === true) { + return [from, to]; + } + if (canBeRequiredByUsers(to)) { + return [from, `node:${to}`]; + } else if (RegExpPrototype.test(kRelativeURLStringPattern, to)) { + return [from, new URL(to, manifestURL).href]; + } + return [from, new URL(to).href]; + }) + ); + } } Object.freeze(integrities); kIntegrities.set(this, integrities); + Object.freeze(dependencies); + kDependencies.set(this, dependencies); Object.freeze(this); } + getRedirects(requester) { + const dependencies = kDependencies.get(this); + if (!dependencies[requester]) return null; + return { + map: dependencies[requester], + reaction: kReactions.get(this) + }; + } + assertIntegrity(url, content) { debug(`Checking integrity of ${url}`); const integrities = kIntegrities.get(this); - const realIntegrities = new Map(); + const realIntegrities = new SafeMap(); if (integrities && url in integrities) { const integrityEntries = integrities[url]; @@ -139,7 +173,7 @@ class Manifest { } } const error = new ERR_MANIFEST_ASSERT_INTEGRITY(url, realIntegrities); - kReactions.get(this).integrity(error); + kReactions.get(this)(error); } } From 4cecf1c5692f4e0097ba28088c399a2f7fc13f06 Mon Sep 17 00:00:00 2001 From: Bradley Meck Date: Sat, 20 Jul 2019 08:02:39 -0500 Subject: [PATCH 02/10] Update doc/api/policy.md Co-Authored-By: Vse Mozhet Byt --- doc/api/policy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/policy.md b/doc/api/policy.md index 5cfa04fbc11da4..9b87578096cb9f 100644 --- a/doc/api/policy.md +++ b/doc/api/policy.md @@ -141,7 +141,7 @@ If the value of the redirection is `true` the default searching algorithms will be used to find the module. If the value of the redirection is a string, it will be resolved relative to -the manifest and them immediately be used without searching. +the manifest and then immediately be used without searching. Any specifier missing from the list of dependency will result in an error according to the policy. From 1f1e8d8771fb66fb8bd7f981590a66121f589a3c Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Mon, 22 Jul 2019 09:03:54 -0500 Subject: [PATCH 03/10] add doc note about needing more mitigations --- doc/api/policy.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/api/policy.md b/doc/api/policy.md index 9b87578096cb9f..f98cba0099ebe8 100644 --- a/doc/api/policy.md +++ b/doc/api/policy.md @@ -146,6 +146,10 @@ the manifest and then immediately be used without searching. Any specifier missing from the list of dependency will result in an error according to the policy. +This will not prevent access to APIs through other means such as direct access +to `require.cache` and/or through `module.constructor`. Other means such as +attenuating variables are necessary to lock down that path of loading modules. + #### Example: Patched Dependency Since a dependency can be redirected, you can provide attenuated or modified From ae801f2d975645e47d8ff9fa94468e5ae180fc0a Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Mon, 22 Jul 2019 11:00:02 -0500 Subject: [PATCH 04/10] add tests and wildcards --- lib/internal/errors.js | 3 + lib/internal/modules/cjs/helpers.js | 43 +++++++-- lib/internal/modules/cjs/loader.js | 8 +- lib/internal/modules/esm/translators.js | 7 +- lib/internal/policy/manifest.js | 75 ++++++++++------ .../dependencies-empty-policy.json | 11 +++ .../dependencies-missing-policy.json | 10 +++ .../dependencies-redirect-builtin-policy.json | 10 +++ .../dependencies-redirect-policy.json | 13 +++ ...ncies-redirect-unknown-builtin-policy.json | 10 +++ .../dependencies-wildcard-policy.json | 11 +++ test/fixtures/policy/parent.js | 3 + test/parallel/test-policy-dependencies.js | 90 +++++++++++++++++++ test/parallel/test-policy-integrity-flag.js | 4 - 14 files changed, 247 insertions(+), 51 deletions(-) create mode 100644 test/fixtures/policy/dependencies/dependencies-empty-policy.json create mode 100644 test/fixtures/policy/dependencies/dependencies-missing-policy.json create mode 100644 test/fixtures/policy/dependencies/dependencies-redirect-builtin-policy.json create mode 100644 test/fixtures/policy/dependencies/dependencies-redirect-policy.json create mode 100644 test/fixtures/policy/dependencies/dependencies-redirect-unknown-builtin-policy.json create mode 100644 test/fixtures/policy/dependencies/dependencies-wildcard-policy.json create mode 100644 test/fixtures/policy/parent.js create mode 100644 test/parallel/test-policy-dependencies.js diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 4869d1b4dd37ac..6e3bfb29c03f27 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1039,6 +1039,9 @@ E('ERR_MANIFEST_DEPENDENCY_MISSING', E('ERR_MANIFEST_INTEGRITY_MISMATCH', 'Manifest resource %s has multiple entries but integrity lists do not match', SyntaxError); +E('ERR_MANIFEST_INVALID_RESOURCE_FIELD', + 'Manifest resource %s has invalid property value for %s', + TypeError); E('ERR_MANIFEST_TDZ', 'Manifest initialization has not yet run', Error); E('ERR_MANIFEST_UNKNOWN_ONERROR', 'Manifest specified unknown error behavior "%s".', diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index abecd1deb4f3ee..fa4292486f5af6 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -2,14 +2,28 @@ const { Object } = primordials; const { - ERR_MANIFEST_DEPENDENCY_MISSING + ERR_MANIFEST_DEPENDENCY_MISSING, + ERR_UNKNOWN_BUILTIN_MODULE } = require('internal/errors').codes; +const { NativeModule } = require('internal/bootstrap/loaders'); +const { getOptionValue } = require('internal/options'); +const experimentalModules = getOptionValue('--experimental-modules'); const { validateString } = require('internal/validators'); const path = require('path'); const { pathToFileURL, fileURLToPath } = require('internal/url'); const { URL } = require('url'); +const debug = require('internal/util/debuglog').debuglog('module'); + +function loadNativeModule(filename, request, experimentalModules) { + const mod = NativeModule.map.get(filename); + if (mod && mod.canBeRequiredByUsers) { + debug('load native module %s', request); + return mod.compileForPublicLoader(experimentalModules); + } +} + // Invoke with makeRequireFunction(module) where |module| is the Module object // to use as the context for the require() function. // Use redirects to set up a mapping from a policy and restrict dependencies @@ -20,23 +34,33 @@ function makeRequireFunction(mod, redirects) { if (redirects) { const { map, reaction } = redirects; require = function require(path) { - if (map.has(path)) { + let missing = true; + if (map === true) { + missing = false; + } else if (map.has(path)) { const redirect = map.get(path); if (redirect === true) { - return mod.require(path); - } else if (redirect) { + missing = false; + } else if (typeof redirect === 'string') { const parsed = new URL(redirect); if (parsed.protocol === 'node:') { - return mod.require(parsed.pathname); + const specifier = parsed.pathname; + const mod = loadNativeModule( + specifier, + redirect, + experimentalModules); + if (mod) return mod; + throw new ERR_UNKNOWN_BUILTIN_MODULE(specifier); } else if (parsed.protocol === 'file:') { return mod.require(fileURLToPath(parsed)); } } } - reaction(new ERR_MANIFEST_DEPENDENCY_MISSING( - path, - mod.filename || mod.id)); - // In the case of log just fall back to same behavior as `true` + if (missing) { + reaction(new ERR_MANIFEST_DEPENDENCY_MISSING( + mod.filename || mod.id, + path)); + } return mod.require(path); }; } else { @@ -143,6 +167,7 @@ function normalizeReferrerURL(referrer) { module.exports = { addBuiltinLibsToObject, builtinLibs, + loadNativeModule, makeRequireFunction, normalizeReferrerURL, stripBOM, diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 4f4be5173139e9..54c86f9f9fa0b5 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -40,6 +40,7 @@ const { makeRequireFunction, normalizeReferrerURL, stripBOM, + loadNativeModule } = require('internal/modules/cjs/helpers'); const { getOptionValue } = require('internal/options'); const preserveSymlinks = getOptionValue('--preserve-symlinks'); @@ -531,11 +532,8 @@ Module._load = function(request, parent, isMain) { return cachedModule.exports; } - const mod = NativeModule.map.get(filename); - if (mod && mod.canBeRequiredByUsers) { - debug('load native module %s', request); - return mod.compileForPublicLoader(experimentalModules); - } + const mod = loadNativeModule(filename, request, experimentalModules); + if (mod) return mod; // Don't call updateChildren(), Module constructor already does. const module = new Module(filename, parent); diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index b1f2c9f2595fc7..584a3303002f3d 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -9,9 +9,9 @@ const { StringPrototype } = primordials; -const { NativeModule } = require('internal/bootstrap/loaders'); const { - stripBOM + stripBOM, + loadNativeModule } = require('internal/modules/cjs/helpers'); const CJSModule = require('internal/modules/cjs/loader').Module; const internalURLModule = require('internal/url'); @@ -93,11 +93,10 @@ translators.set('builtin', async function builtinStrategy(url) { debug(`Translating BuiltinModule ${url}`); // Slice 'node:' scheme const id = url.slice(5); - const module = NativeModule.map.get(id); + const module = loadNativeModule(id, url, true); if (!module) { throw new ERR_UNKNOWN_BUILTIN_MODULE(id); } - module.compileForPublicLoader(true); return createDynamicModule( [], [...module.exportKeys, 'default'], url, (reflect) => { debug(`Loading BuiltinModule ${url}`); diff --git a/lib/internal/policy/manifest.js b/lib/internal/policy/manifest.js index 6205303d631745..f91eb5b0129d48 100644 --- a/lib/internal/policy/manifest.js +++ b/lib/internal/policy/manifest.js @@ -14,6 +14,7 @@ const { const { ERR_MANIFEST_ASSERT_INTEGRITY, ERR_MANIFEST_INTEGRITY_MISMATCH, + ERR_MANIFEST_INVALID_RESOURCE_FIELD, ERR_MANIFEST_UNKNOWN_ONERROR, } = require('internal/errors').codes; const debug = require('internal/util/debuglog').debuglog('policy'); @@ -83,41 +84,50 @@ class Manifest { if (RegExpPrototype.test(kRelativeURLStringPattern, url)) { url = new URL(url, manifestURL).href; } - const integrity = manifestEntries[i][1].integrity; + let integrity = manifestEntries[i][1].integrity; + if (!integrity) integrity = null; if (integrity != null) { debug(`Manifest contains integrity for url ${originalURL}`); + if (typeof integrity === 'string') { + const sri = Object.freeze(SRI.parse(integrity)); + if (url in integrities) { + const old = integrities[url]; + let mismatch = false; - const sri = Object.freeze(SRI.parse(integrity)); - if (url in integrities) { - const old = integrities[url]; - let mismatch = false; - - if (old.length !== sri.length) { - mismatch = true; - } else { - compare: - for (var sriI = 0; sriI < sri.length; sriI++) { - for (var oldI = 0; oldI < old.length; oldI++) { - if (sri[sriI].algorithm === old[oldI].algorithm && - BufferEquals(sri[sriI].value, old[oldI].value) && - sri[sriI].options === old[oldI].options) { - continue compare; + if (old.length !== sri.length) { + mismatch = true; + } else { + compare: + for (var sriI = 0; sriI < sri.length; sriI++) { + for (var oldI = 0; oldI < old.length; oldI++) { + if (sri[sriI].algorithm === old[oldI].algorithm && + BufferEquals(sri[sriI].value, old[oldI].value) && + sri[sriI].options === old[oldI].options) { + continue compare; + } } + mismatch = true; + break compare; } - mismatch = true; - break compare; } - } - if (mismatch) { - throw new ERR_MANIFEST_INTEGRITY_MISMATCH(url); + if (mismatch) { + throw new ERR_MANIFEST_INTEGRITY_MISMATCH(url); + } } + integrities[url] = sri; + } else if (integrity === true) { + integrities[url] = true; + } else { + throw new ERR_MANIFEST_INVALID_RESOURCE_FIELD(url, 'integrity'); } - integrities[url] = sri; } - const dependencyMap = manifestEntries[i][1].dependencies; - if (dependencyMap) { + let dependencyMap = manifestEntries[i][1].dependencies; + if (dependencyMap === null || dependencyMap === undefined) { + dependencyMap = {}; + } + if (typeof dependencyMap === 'object' && !Array.isArray(dependencyMap)) { dependencies[url] = new SafeMap(Object.entries(dependencyMap).map( ([ from, to ]) => { if (to === true) { @@ -131,6 +141,10 @@ class Manifest { return [from, new URL(to).href]; }) ); + } else if (dependencyMap === true) { + dependencies[url] = true; + } else { + throw new ERR_MANIFEST_INVALID_RESOURCE_FIELD(url, 'dependencies'); } } Object.freeze(integrities); @@ -142,11 +156,13 @@ class Manifest { getRedirects(requester) { const dependencies = kDependencies.get(this); - if (!dependencies[requester]) return null; - return { - map: dependencies[requester], - reaction: kReactions.get(this) - }; + if (dependencies && requester in dependencies) { + return { + map: dependencies[requester], + reaction: kReactions.get(this) + }; + } + return null; } assertIntegrity(url, content) { @@ -156,6 +172,7 @@ class Manifest { if (integrities && url in integrities) { const integrityEntries = integrities[url]; + if (integrityEntries === true) return true; // Avoid clobbered Symbol.iterator for (var i = 0; i < integrityEntries.length; i++) { const { diff --git a/test/fixtures/policy/dependencies/dependencies-empty-policy.json b/test/fixtures/policy/dependencies/dependencies-empty-policy.json new file mode 100644 index 00000000000000..9c0389cd03928f --- /dev/null +++ b/test/fixtures/policy/dependencies/dependencies-empty-policy.json @@ -0,0 +1,11 @@ +{ + "resources": { + "../parent.js": { + "integrity": true, + "dependencies": {} + }, + "../dep.js": { + "integrity": true + } + } +} \ No newline at end of file diff --git a/test/fixtures/policy/dependencies/dependencies-missing-policy.json b/test/fixtures/policy/dependencies/dependencies-missing-policy.json new file mode 100644 index 00000000000000..40d2866ba5e06d --- /dev/null +++ b/test/fixtures/policy/dependencies/dependencies-missing-policy.json @@ -0,0 +1,10 @@ +{ + "resources": { + "../parent.js": { + "integrity": true + }, + "../dep.js": { + "integrity": true + } + } +} \ No newline at end of file diff --git a/test/fixtures/policy/dependencies/dependencies-redirect-builtin-policy.json b/test/fixtures/policy/dependencies/dependencies-redirect-builtin-policy.json new file mode 100644 index 00000000000000..437228a2e50277 --- /dev/null +++ b/test/fixtures/policy/dependencies/dependencies-redirect-builtin-policy.json @@ -0,0 +1,10 @@ +{ + "resources": { + "../parent.js": { + "integrity": true, + "dependencies": { + "./dep.js": "node:util" + } + } + } +} \ No newline at end of file diff --git a/test/fixtures/policy/dependencies/dependencies-redirect-policy.json b/test/fixtures/policy/dependencies/dependencies-redirect-policy.json new file mode 100644 index 00000000000000..993a683f10a38f --- /dev/null +++ b/test/fixtures/policy/dependencies/dependencies-redirect-policy.json @@ -0,0 +1,13 @@ +{ + "resources": { + "../parent.js": { + "integrity": true, + "dependencies": { + "./dep.js": "../dep.js" + } + }, + "../dep.js": { + "integrity": true + } + } +} \ No newline at end of file diff --git a/test/fixtures/policy/dependencies/dependencies-redirect-unknown-builtin-policy.json b/test/fixtures/policy/dependencies/dependencies-redirect-unknown-builtin-policy.json new file mode 100644 index 00000000000000..db2046c6d36f07 --- /dev/null +++ b/test/fixtures/policy/dependencies/dependencies-redirect-unknown-builtin-policy.json @@ -0,0 +1,10 @@ +{ + "resources": { + "../parent.js": { + "integrity": true, + "dependencies": { + "./dep.js": "node:404" + } + } + } +} \ No newline at end of file diff --git a/test/fixtures/policy/dependencies/dependencies-wildcard-policy.json b/test/fixtures/policy/dependencies/dependencies-wildcard-policy.json new file mode 100644 index 00000000000000..04ae9a318f6dc0 --- /dev/null +++ b/test/fixtures/policy/dependencies/dependencies-wildcard-policy.json @@ -0,0 +1,11 @@ +{ + "resources": { + "../parent.js": { + "integrity": true, + "dependencies": true + }, + "../dep.js": { + "integrity": true + } + } +} \ No newline at end of file diff --git a/test/fixtures/policy/parent.js b/test/fixtures/policy/parent.js new file mode 100644 index 00000000000000..b821bfee184a35 --- /dev/null +++ b/test/fixtures/policy/parent.js @@ -0,0 +1,3 @@ +'use strict'; +// Included in parent-policy.json +require('./dep.js'); diff --git a/test/parallel/test-policy-dependencies.js b/test/parallel/test-policy-dependencies.js new file mode 100644 index 00000000000000..e7b2289714dff4 --- /dev/null +++ b/test/parallel/test-policy-dependencies.js @@ -0,0 +1,90 @@ +'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 dep = fixtures.path('policy', 'parent.js'); +{ + const depPolicy = fixtures.path( + 'policy', + 'dependencies', + 'dependencies-redirect-policy.json'); + const { status } = spawnSync( + process.execPath, + [ + '--experimental-policy', depPolicy, dep, + ] + ); + assert.strictEqual(status, 0); +} +{ + const depPolicy = fixtures.path( + 'policy', + 'dependencies', + 'dependencies-redirect-builtin-policy.json'); + const { status } = spawnSync( + process.execPath, + [ + '--experimental-policy', depPolicy, dep, + ] + ); + assert.strictEqual(status, 0); +} +{ + const depPolicy = fixtures.path( + 'policy', + 'dependencies', + 'dependencies-redirect-unknown-builtin-policy.json'); + const { status } = spawnSync( + process.execPath, + [ + '--experimental-policy', depPolicy, dep, + ] + ); + assert.strictEqual(status, 1); +} +{ + const depPolicy = fixtures.path( + 'policy', + 'dependencies', + 'dependencies-wildcard-policy.json'); + const { status } = spawnSync( + process.execPath, + [ + '--experimental-policy', depPolicy, dep, + ] + ); + assert.strictEqual(status, 0); +} +{ + const depPolicy = fixtures.path( + 'policy', + 'dependencies', + 'dependencies-empty-policy.json'); + const { status } = spawnSync( + process.execPath, + [ + '--experimental-policy', depPolicy, dep, + ] + ); + assert.strictEqual(status, 1); +} +{ + const depPolicy = fixtures.path( + 'policy', + 'dependencies', + 'dependencies-missing-policy.json'); + const { status } = spawnSync( + process.execPath, + [ + '--experimental-policy', depPolicy, dep, + ] + ); + assert.strictEqual(status, 1); +} diff --git a/test/parallel/test-policy-integrity-flag.js b/test/parallel/test-policy-integrity-flag.js index 3b332758d18c0a..d97ef86cbe9d0f 100644 --- a/test/parallel/test-policy-integrity-flag.js +++ b/test/parallel/test-policy-integrity-flag.js @@ -28,10 +28,6 @@ const windowsPolicySRI = 'sha512-OeyCPRo4OZMosHyquZXDHpuU1F4KzG9UHFnn12FMaHsvqFU /* eslint-enable max-len */ const depPolicySRI = `${nixPolicySRI} ${windowsPolicySRI}`; -console.dir({ - depPolicySRI, - body: JSON.stringify(fs.readFileSync(depPolicy).toString('utf8')) -}); { const { status, stderr } = spawnSync( process.execPath, From 3514dbc5ec20cdd0f893e68357f30fa67cd2bc8f Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Mon, 22 Jul 2019 11:58:04 -0500 Subject: [PATCH 05/10] slightly better docs --- doc/api/errors.md | 7 +++++++ doc/api/policy.md | 10 ++++++++++ 2 files changed, 17 insertions(+) diff --git a/doc/api/errors.md b/doc/api/errors.md index bf63bc8fcb8593..e809b26f6d0d9c 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1447,6 +1447,13 @@ 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. + +### ERR_MANIFEST_INVALID_RESOURCE_FIELD + +A policy manifest resource had an invalid value for one of its fields. Update +the manifest entry to match in order to resolve this error. See the +documentation for [policy] manifests for more information. + ### ERR_MANIFEST_PARSE_POLICY diff --git a/doc/api/policy.md b/doc/api/policy.md index f98cba0099ebe8..9f3c1c329d6620 100644 --- a/doc/api/policy.md +++ b/doc/api/policy.md @@ -109,6 +109,11 @@ In order to generate integrity strings, a script such as `printf "sha384-$(cat checked.js | openssl dgst -sha384 -binary | base64)"` can be used. +Integrity can be specified as the boolean value `true` in order to accept any +body for the resource which can be useful for local development. It is not +recommended in production since it would allow unexpected alteration of +resources to be considered valid. + ### Dependency Redirection An application may need to ship patched versions of software or to prevent @@ -150,6 +155,11 @@ This will not prevent access to APIs through other means such as direct access to `require.cache` and/or through `module.constructor`. Other means such as attenuating variables are necessary to lock down that path of loading modules. +A boolean value of `true` for the dependencies map can be specified to allow a +module to load any specifier without redirection. This can be useful for local +development and may have some valid usage in production, but should be used +only with care after auditing a module to ensure its behavior is valid. + #### Example: Patched Dependency Since a dependency can be redirected, you can provide attenuated or modified From 88c578c0bf986ddd23fb9453587bdb2b656c41dc Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Tue, 23 Jul 2019 08:16:25 -0500 Subject: [PATCH 06/10] fix ESM integration / tests --- lib/internal/modules/cjs/helpers.js | 12 ++++++------ lib/internal/modules/cjs/loader.js | 2 +- test/common/index.mjs | 7 +------ test/message/async_error_sync_esm.out | 1 - test/parallel/test-policy-integrity.js | 5 +++-- 5 files changed, 11 insertions(+), 16 deletions(-) diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index fa4292486f5af6..5e0e1b06ae3a1c 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -18,9 +18,10 @@ const debug = require('internal/util/debuglog').debuglog('module'); function loadNativeModule(filename, request, experimentalModules) { const mod = NativeModule.map.get(filename); - if (mod && mod.canBeRequiredByUsers) { + if (mod) { debug('load native module %s', request); - return mod.compileForPublicLoader(experimentalModules); + mod.compileForPublicLoader(experimentalModules); + return mod; } } @@ -33,6 +34,7 @@ function makeRequireFunction(mod, redirects) { let require; if (redirects) { const { map, reaction } = redirects; + const id = mod.filename || mod.id; require = function require(path) { let missing = true; if (map === true) { @@ -49,7 +51,7 @@ function makeRequireFunction(mod, redirects) { specifier, redirect, experimentalModules); - if (mod) return mod; + if (mod && mod.canBeRequiredByUsers) return mod.exports; throw new ERR_UNKNOWN_BUILTIN_MODULE(specifier); } else if (parsed.protocol === 'file:') { return mod.require(fileURLToPath(parsed)); @@ -57,9 +59,7 @@ function makeRequireFunction(mod, redirects) { } } if (missing) { - reaction(new ERR_MANIFEST_DEPENDENCY_MISSING( - mod.filename || mod.id, - path)); + reaction(new ERR_MANIFEST_DEPENDENCY_MISSING(id, path)); } return mod.require(path); }; diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 54c86f9f9fa0b5..df5172c1b4db4e 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -533,7 +533,7 @@ Module._load = function(request, parent, isMain) { } const mod = loadNativeModule(filename, request, experimentalModules); - if (mod) return mod; + if (mod && mod.canBeRequiredByUsers) return mod.exports; // Don't call updateChildren(), Module constructor already does. const module = new Module(filename, parent); diff --git a/test/common/index.mjs b/test/common/index.mjs index 47587044020fcc..f747ee327913a5 100644 --- a/test/common/index.mjs +++ b/test/common/index.mjs @@ -1,12 +1,7 @@ // Flags: --experimental-modules /* eslint-disable node-core/require-common-first, node-core/required-modules */ -import { createRequireFromPath } from 'module'; -import { fileURLToPath as toPath } from 'url'; - -function createRequire(metaUrl) { - return createRequireFromPath(toPath(metaUrl)); -} +import { createRequire } from 'module'; const require = createRequire(import.meta.url); const common = require('./index.js'); diff --git a/test/message/async_error_sync_esm.out b/test/message/async_error_sync_esm.out index 544916e24866e2..f34628ef44e52a 100644 --- a/test/message/async_error_sync_esm.out +++ b/test/message/async_error_sync_esm.out @@ -5,4 +5,3 @@ Error: test at async three (*fixtures*async-error.js:20:3) at async four (*fixtures*async-error.js:24:3) at async main (*message*async_error_sync_esm.mjs:7:5) -(node:*) [DEP0130] DeprecationWarning: Module.createRequireFromPath() is deprecated. Use Module.createRequire() instead. diff --git a/test/parallel/test-policy-integrity.js b/test/parallel/test-policy-integrity.js index 86d1078d9f24b3..d2a18c27a9ff25 100644 --- a/test/parallel/test-policy-integrity.js +++ b/test/parallel/test-policy-integrity.js @@ -71,12 +71,13 @@ function test({ }; for (const [url, { body, match }] of Object.entries(resources)) { manifest.resources[url] = { - integrity: `sha256-${hash('sha256', match ? body : body + '\n')}` + integrity: `sha256-${hash('sha256', match ? body : body + '\n')}`, + dependencies: true }; fs.writeFileSync(new URL(url, tmpdirURL.href), body); } fs.writeFileSync(policyFilepath, JSON.stringify(manifest, null, 2)); - const { status } = spawnSync(process.execPath, [ + const { status, stderr, stdout } = spawnSync(process.execPath, [ '--experimental-policy', policyFilepath, ...preload.map((m) => ['-r', m]).flat(), entry From 5774f7825ea67fb34ec0a36a9e63405f3b861b59 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Tue, 23 Jul 2019 08:37:23 -0500 Subject: [PATCH 07/10] lint nits --- test/parallel/test-policy-integrity.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-policy-integrity.js b/test/parallel/test-policy-integrity.js index d2a18c27a9ff25..722b45f633174d 100644 --- a/test/parallel/test-policy-integrity.js +++ b/test/parallel/test-policy-integrity.js @@ -77,7 +77,7 @@ function test({ fs.writeFileSync(new URL(url, tmpdirURL.href), body); } fs.writeFileSync(policyFilepath, JSON.stringify(manifest, null, 2)); - const { status, stderr, stdout } = spawnSync(process.execPath, [ + const { status } = spawnSync(process.execPath, [ '--experimental-policy', policyFilepath, ...preload.map((m) => ['-r', m]).flat(), entry From 8a2e4a535afcf89af7783a93bf8b941bfc6c6a20 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 24 Jul 2019 08:23:50 -0500 Subject: [PATCH 08/10] doc wording --- doc/api/policy.md | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/doc/api/policy.md b/doc/api/policy.md index 9f3c1c329d6620..3306ac3765cd94 100644 --- a/doc/api/policy.md +++ b/doc/api/policy.md @@ -116,9 +116,10 @@ resources to be considered valid. ### Dependency Redirection -An application may need to ship patched versions of software or to prevent -software from allowing all modules access to all other modules. In order to -do so redirection can be used. +An application may need to ship patched versions of modules or to prevent +modules from allowing all modules access to all other modules. In order to +do so redirection can be used by intercepting attempts to load the modules +wishing to be replaced. ```json { @@ -148,12 +149,14 @@ be used to find the module. If the value of the redirection is a string, it will be resolved relative to the manifest and then immediately be used without searching. -Any specifier missing from the list of dependency will result in an error -according to the policy. +Any specifier string that is `require()`ed and not listed in the dependencies +will result in an error according to the policy. -This will not prevent access to APIs through other means such as direct access -to `require.cache` and/or through `module.constructor`. Other means such as -attenuating variables are necessary to lock down that path of loading modules. +Redirection will not prevent access to APIs through means such as direct access +to `require.cache` and/or through `module.constructor` which allow access to +loading modules. Policy redirection only affect specifiers to `require()`. +Other means such as to prevent undesired access to APIs through variables are +necessary to lock down that path of loading modules. A boolean value of `true` for the dependencies map can be specified to allow a module to load any specifier without redirection. This can be useful for local From 282ed3f81e5132d4c4f04e866d728ebc75152524 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 24 Jul 2019 08:27:17 -0500 Subject: [PATCH 09/10] doc wording --- doc/api/policy.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/policy.md b/doc/api/policy.md index 3306ac3765cd94..98beaccef37506 100644 --- a/doc/api/policy.md +++ b/doc/api/policy.md @@ -117,9 +117,9 @@ resources to be considered valid. ### Dependency Redirection An application may need to ship patched versions of modules or to prevent -modules from allowing all modules access to all other modules. In order to -do so redirection can be used by intercepting attempts to load the modules -wishing to be replaced. +modules from allowing all modules access to all other modules. Redirection +can be used by intercepting attempts to load the modules wishing to be +replaced. ```json { From 7452bb62736b7baecaf38ee41fbb00667c23cf93 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Wed, 24 Jul 2019 08:30:46 -0500 Subject: [PATCH 10/10] doc wording --- doc/api/policy.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/policy.md b/doc/api/policy.md index 98beaccef37506..d4c649636e49a8 100644 --- a/doc/api/policy.md +++ b/doc/api/policy.md @@ -38,7 +38,7 @@ 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 +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. @@ -105,11 +105,11 @@ 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 +To generate integrity strings, a script such as `printf "sha384-$(cat checked.js | openssl dgst -sha384 -binary | base64)"` can be used. -Integrity can be specified as the boolean value `true` in order to accept any +Integrity can be specified as the boolean value `true` to accept any body for the resource which can be useful for local development. It is not recommended in production since it would allow unexpected alteration of resources to be considered valid.