Skip to content

Commit b02d895

Browse files
RafaelGSSbmeck
authored and
richardlau
committedFeb 15, 2023
policy: makeRequireFunction on mainModule.require
Co-authored-by: Bradley Farias <bradley.meck@gmail.com> Backport-PR-URL: nodejs-private/node-private#372 Refs: https://hackerone.com/bugs?subject=nodejs&report_id=1747642 CVE-ID: CVE-2023-23918 PR-URL: nodejs-private/node-private#358 Reviewed-by: Bradley Farias <bradley.meck@gmail.com> Reviewed-by: Michael Dawson <midawson@redhat.com>
·
v16.20.2v16.19.1
1 parent 2d9ae4f commit b02d895

17 files changed

+151
-49
lines changed
 

‎lib/internal/modules/cjs/helpers.js

+7-3
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@ const { getOptionValue } = require('internal/options');
2727
const { setOwnProperty } = require('internal/util');
2828
const userConditions = getOptionValue('--conditions');
2929

30+
const {
31+
require_private_symbol,
32+
} = internalBinding('util');
33+
3034
let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
3135
debug = fn;
3236
});
@@ -86,7 +90,7 @@ function makeRequireFunction(mod, redirects) {
8690
filepath = fileURLToPath(destination);
8791
urlToFileCache.set(href, filepath);
8892
}
89-
return mod.require(filepath);
93+
return mod[require_private_symbol](mod, filepath);
9094
}
9195
}
9296
if (missing) {
@@ -96,11 +100,11 @@ function makeRequireFunction(mod, redirects) {
96100
ArrayPrototypeJoin([...conditions], ', ')
97101
));
98102
}
99-
return mod.require(specifier);
103+
return mod[require_private_symbol](mod, specifier);
100104
};
101105
} else {
102106
require = function require(path) {
103-
return mod.require(path);
107+
return mod[require_private_symbol](mod, path);
104108
};
105109
}
106110

‎lib/internal/modules/cjs/loader.js

+31-24
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,9 @@ const { sep } = path;
9595
const { internalModuleStat } = internalBinding('fs');
9696
const packageJsonReader = require('internal/modules/package_json_reader');
9797
const { safeGetenv } = internalBinding('credentials');
98+
const {
99+
require_private_symbol,
100+
} = internalBinding('util');
98101
const {
99102
cjsConditions,
100103
hasEsmSyntax,
@@ -155,6 +158,20 @@ let requireDepth = 0;
155158
let statCache = null;
156159
let isPreloading = false;
157160

161+
function internalRequire(module, id) {
162+
validateString(id, 'id');
163+
if (id === '') {
164+
throw new ERR_INVALID_ARG_VALUE('id', id,
165+
'must be a non-empty string');
166+
}
167+
requireDepth++;
168+
try {
169+
return Module._load(id, module, /* isMain */ false);
170+
} finally {
171+
requireDepth--;
172+
}
173+
}
174+
158175
function stat(filename) {
159176
filename = path.toNamespacedPath(filename);
160177
if (statCache !== null) {
@@ -203,6 +220,15 @@ function Module(id = '', parent) {
203220
this.filename = null;
204221
this.loaded = false;
205222
this.children = [];
223+
let redirects;
224+
if (policy?.manifest) {
225+
const moduleURL = pathToFileURL(id);
226+
redirects = policy.manifest.getDependencyMapper(moduleURL);
227+
}
228+
setOwnProperty(this, 'require', makeRequireFunction(this, redirects));
229+
// Loads a module at the given file path. Returns that module's
230+
// `exports` property.
231+
this[require_private_symbol] = internalRequire;
206232
}
207233

208234
const builtinModules = [];
@@ -863,6 +889,7 @@ Module._load = function(request, parent, isMain) {
863889

864890
if (isMain) {
865891
process.mainModule = module;
892+
setOwnProperty(module.require, 'main', process.mainModule);
866893
module.id = '.';
867894
}
868895

@@ -1053,24 +1080,6 @@ Module.prototype.load = function(filename) {
10531080
esmLoader.cjsCache.set(this, exports);
10541081
};
10551082

1056-
1057-
// Loads a module at the given file path. Returns that module's
1058-
// `exports` property.
1059-
Module.prototype.require = function(id) {
1060-
validateString(id, 'id');
1061-
if (id === '') {
1062-
throw new ERR_INVALID_ARG_VALUE('id', id,
1063-
'must be a non-empty string');
1064-
}
1065-
requireDepth++;
1066-
try {
1067-
return Module._load(id, this, /* isMain */ false);
1068-
} finally {
1069-
requireDepth--;
1070-
}
1071-
};
1072-
1073-
10741083
// Resolved path to process.argv[1] will be lazily placed here
10751084
// (needed for setting breakpoint when called with --inspect-brk)
10761085
let resolvedArgv;
@@ -1118,10 +1127,9 @@ function wrapSafe(filename, content, cjsModuleInstance) {
11181127
// Returns exception, if any.
11191128
Module.prototype._compile = function(content, filename) {
11201129
let moduleURL;
1121-
let redirects;
11221130
if (policy?.manifest) {
11231131
moduleURL = pathToFileURL(filename);
1124-
redirects = policy.manifest.getDependencyMapper(moduleURL);
1132+
policy.manifest.getDependencyMapper(moduleURL);
11251133
policy.manifest.assertIntegrity(moduleURL, content);
11261134
}
11271135

@@ -1152,18 +1160,17 @@ Module.prototype._compile = function(content, filename) {
11521160
}
11531161
}
11541162
const dirname = path.dirname(filename);
1155-
const require = makeRequireFunction(this, redirects);
11561163
let result;
11571164
const exports = this.exports;
11581165
const thisValue = exports;
11591166
const module = this;
11601167
if (requireDepth === 0) statCache = new SafeMap();
11611168
if (inspectorWrapper) {
11621169
result = inspectorWrapper(compiledWrapper, thisValue, exports,
1163-
require, module, filename, dirname);
1170+
module.require, module, filename, dirname);
11641171
} else {
11651172
result = ReflectApply(compiledWrapper, thisValue,
1166-
[exports, require, module, filename, dirname]);
1173+
[exports, module.require, module, filename, dirname]);
11671174
}
11681175
hasLoadedAnyUserCJSModule = true;
11691176
if (requireDepth === 0) statCache = null;
@@ -1339,7 +1346,7 @@ Module._preloadModules = function(requests) {
13391346
}
13401347
}
13411348
for (let n = 0; n < requests.length; n++)
1342-
parent.require(requests[n]);
1349+
internalRequire(parent, requests[n]);
13431350
isPreloading = false;
13441351
};
13451352

‎src/env.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,8 @@ class NoArrayBufferZeroFillScope {
174174
V(napi_type_tag, "node:napi:type_tag") \
175175
V(napi_wrapper, "node:napi:wrapper") \
176176
V(untransferable_object_private_symbol, "node:untransferableObject") \
177-
V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array")
177+
V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array") \
178+
V(require_private_symbol, "node:require_private_symbol")
178179

179180
// Symbols are per-isolate primitives but Environment proxies them
180181
// for the sake of convenience.

‎test/fixtures/policy-manifest/main-module-bypass.js

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
process.mainModule.require('os').cpus();

‎test/fixtures/policy-manifest/object-define-property-bypass.js

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
let requires = new WeakMap()
2+
Object.defineProperty(Object.getPrototypeOf(module), 'require', {
3+
get() {
4+
return requires.get(this);
5+
},
6+
set(v) {
7+
requires.set(this, v);
8+
process.nextTick(() => {
9+
let fs = Reflect.apply(v, this, ['fs'])
10+
if (typeof fs.readFileSync === 'function') {
11+
process.exit(1);
12+
}
13+
})
14+
return requires.get(this);
15+
},
16+
configurable: true
17+
})
18+
19+
require('./valid-module')

‎test/fixtures/policy-manifest/onerror-exit.json

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"onerror": "exit",
3+
"scopes": {
4+
"file:": {
5+
"integrity": true,
6+
"dependencies": {}
7+
}
8+
}
9+
}

‎test/fixtures/policy-manifest/onerror-resource-exit.json

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
{
2+
"onerror": "exit",
3+
"resources": {
4+
"./object-define-property-bypass.js": {
5+
"integrity": true,
6+
"dependencies": {
7+
"./valid-module": true
8+
}
9+
},
10+
"./valid-module.js": {
11+
"integrity": true,
12+
"dependencies": {
13+
"fs": true
14+
}
15+
}
16+
}
17+
}

‎test/fixtures/policy-manifest/valid-module.js

Whitespace-only changes.

‎test/message/source_map_disabled_by_api.out

+2-2
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ Error: an error!
88
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
99
at Module.load (node:internal/modules/cjs/loader:*)
1010
at Function.Module._load (node:internal/modules/cjs/loader:*)
11-
at Module.require (node:internal/modules/cjs/loader:*)
11+
at Module.internalRequire (node:internal/modules/cjs/loader:*)
1212
*enclosing-call-site.js:16
1313
throw new Error('an error!')
1414
^
@@ -23,4 +23,4 @@ Error: an error!
2323
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
2424
at Module.load (node:internal/modules/cjs/loader:*)
2525
at Function.Module._load (node:internal/modules/cjs/loader:*)
26-
at Module.require (node:internal/modules/cjs/loader:*)
26+
at Module.internalRequire (node:internal/modules/cjs/loader:*)

‎test/message/source_map_enabled_by_api.out

+2-2
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ Error: an error!
1212
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
1313
at Module.load (node:internal/modules/cjs/loader:*)
1414
at Function.Module._load (node:internal/modules/cjs/loader:*)
15-
at Module.require (node:internal/modules/cjs/loader:*)
15+
at Module.internalRequire (node:internal/modules/cjs/loader:*)
1616
*enclosing-call-site-min.js:1
1717
var functionA=function(){functionB()};function functionB(){functionC()}var functionC=function(){functionD()},functionD=function(){if(0<Math.random())throw Error("an error!");},thrower=functionA;try{functionA()}catch(a){throw a;};
1818
^
@@ -27,4 +27,4 @@ Error: an error!
2727
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
2828
at Module.load (node:internal/modules/cjs/loader:*)
2929
at Function.Module._load (node:internal/modules/cjs/loader:*)
30-
at Module.require (node:internal/modules/cjs/loader:*)
30+
at Module.internalRequire (node:internal/modules/cjs/loader:*)

‎test/message/source_map_enclosing_function.out

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,4 @@ Error: an error!
1212
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
1313
at Module.load (node:internal/modules/cjs/loader:*)
1414
at Function.Module._load (node:internal/modules/cjs/loader:*)
15-
at Module.require (node:internal/modules/cjs/loader:*)
15+
at Module.internalRequire (node:internal/modules/cjs/loader:*)

‎test/message/source_map_reference_error_tabs.out

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ ReferenceError: alert is not defined
99
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*
1010
at Module.load (node:internal/modules/cjs/loader:*
1111
at Function.Module._load (node:internal/modules/cjs/loader:*
12-
at Module.require (node:internal/modules/cjs/loader:*
12+
at Module.internalRequire (node:internal/modules/cjs/loader:*
1313
at require (node:internal/modules/cjs/helpers:*
1414
at Object.<anonymous> (*source_map_reference_error_tabs.js:*
1515
at Module._compile (node:internal/modules/cjs/loader:*

‎test/message/source_map_throw_catch.out

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ Error: an exception
99
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
1010
at Module.load (node:internal/modules/cjs/loader:*)
1111
at Function.Module._load (node:internal/modules/cjs/loader:*)
12-
at Module.require (node:internal/modules/cjs/loader:*)
12+
at Module.internalRequire (node:internal/modules/cjs/loader:*)
1313
at require (node:internal/modules/cjs/helpers:*)
1414
at Object.<anonymous> (*source_map_throw_catch.js:6:3)
1515
at Module._compile (node:internal/modules/cjs/loader:*)

‎test/message/source_map_throw_first_tick.out

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ Error: an exception
99
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*)
1010
at Module.load (node:internal/modules/cjs/loader:*)
1111
at Function.Module._load (node:internal/modules/cjs/loader:*)
12-
at Module.require (node:internal/modules/cjs/loader:*)
12+
at Module.internalRequire (node:internal/modules/cjs/loader:*)
1313
at require (node:internal/modules/cjs/helpers:*)
1414
at Object.<anonymous> (*source_map_throw_first_tick.js:5:1)
1515
at Module._compile (node:internal/modules/cjs/loader:*)

‎test/message/source_map_throw_icu.out

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ Error: an error
99
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*
1010
at Module.load (node:internal/modules/cjs/loader:*
1111
at Function.Module._load (node:internal/modules/cjs/loader:*
12-
at Module.require (node:internal/modules/cjs/loader:*
12+
at Module.internalRequire (node:internal/modules/cjs/loader:*
1313
at require (node:internal/modules/cjs/helpers:*
1414
at Object.<anonymous> (*source_map_throw_icu.js:*
1515
at Module._compile (node:internal/modules/cjs/loader:*

‎test/parallel/test-policy-manifest.js

+55-12
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,58 @@ const assert = require('assert');
1111
const { spawnSync } = require('child_process');
1212
const fixtures = require('../common/fixtures.js');
1313

14-
const policyFilepath = fixtures.path('policy-manifest', 'invalid.json');
15-
16-
const result = spawnSync(process.execPath, [
17-
'--experimental-policy',
18-
policyFilepath,
19-
'./fhqwhgads.js',
20-
]);
21-
22-
assert.notStrictEqual(result.status, 0);
23-
const stderr = result.stderr.toString();
24-
assert.match(stderr, /ERR_MANIFEST_INVALID_SPECIFIER/);
25-
assert.match(stderr, /pattern needs to have a single trailing "\*"/);
14+
{
15+
const policyFilepath = fixtures.path('policy-manifest', 'invalid.json');
16+
const result = spawnSync(process.execPath, [
17+
'--experimental-policy',
18+
policyFilepath,
19+
'./fhqwhgads.js',
20+
]);
21+
22+
assert.notStrictEqual(result.status, 0);
23+
const stderr = result.stderr.toString();
24+
assert.match(stderr, /ERR_MANIFEST_INVALID_SPECIFIER/);
25+
assert.match(stderr, /pattern needs to have a single trailing "\*"/);
26+
}
27+
28+
{
29+
const policyFilepath = fixtures.path('policy-manifest', 'onerror-exit.json');
30+
const result = spawnSync(process.execPath, [
31+
'--experimental-policy',
32+
policyFilepath,
33+
'-e',
34+
'require("os").cpus()',
35+
]);
36+
37+
assert.notStrictEqual(result.status, 0);
38+
const stderr = result.stderr.toString();
39+
assert.match(stderr, /ERR_MANIFEST_DEPENDENCY_MISSING/);
40+
assert.match(stderr, /does not list module as a dependency specifier for conditions: require, node, node-addons/);
41+
}
42+
43+
{
44+
const policyFilepath = fixtures.path('policy-manifest', 'onerror-exit.json');
45+
const mainModuleBypass = fixtures.path('policy-manifest', 'main-module-bypass.js');
46+
const result = spawnSync(process.execPath, [
47+
'--experimental-policy',
48+
policyFilepath,
49+
mainModuleBypass,
50+
]);
51+
52+
assert.notStrictEqual(result.status, 0);
53+
const stderr = result.stderr.toString();
54+
assert.match(stderr, /ERR_MANIFEST_DEPENDENCY_MISSING/);
55+
assert.match(stderr, /does not list os as a dependency specifier for conditions: require, node, node-addons/);
56+
}
57+
58+
{
59+
const policyFilepath = fixtures.path('policy-manifest', 'onerror-resource-exit.json');
60+
const objectDefinePropertyBypass = fixtures.path('policy-manifest', 'object-define-property-bypass.js');
61+
const result = spawnSync(process.execPath, [
62+
'--experimental-policy',
63+
policyFilepath,
64+
objectDefinePropertyBypass,
65+
]);
66+
67+
assert.strictEqual(result.status, 0);
68+
}

‎typings/internalBinding/util.d.ts

+1
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ declare function InternalBinding(binding: 'util'): {
1818
napi_wrapper: 6;
1919
untransferable_object_private_symbol: 7;
2020
exiting_aliased_Uint32Array: 8;
21+
require_private_symbol: 9;
2122

2223
kPending: 0;
2324
kFulfilled: 1;

0 commit comments

Comments
 (0)
Please sign in to comment.