8000 policy: makeRequireFunction on mainModule.require · nodejs/node@af91400 · GitHub
[go: up one dir, main page]
Skip to content

Commit af91400

Browse files
RafaelGSSbmeck
RafaelGSS
and
bmeck
committed
policy: makeRequireFunction on mainModule.require
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com> Co-authored-by: Bradley Farias <bradley.meck@gmail.com> 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: Matteo Collina <matteo.collina@gmail.com>
1 parent dd1977f commit af91400

File tree

9 files changed

+146
-40
lines changed

9 files changed

+146
-40
lines changed

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

Lines changed: 34 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,11 @@ const path = require('path');
9696
const { sep } = path;
9797
const { internalModuleStat } = internalBinding('fs');
9898
const { safeGetenv } = internalBinding('credentials');
99+
const {
100+
privateSymbols: {
101+
require_private_symbol,
102+
},
103+
} = internalBinding('util');
99104
const {
100105
getCjsConditions,
101106
initializeCjsConditions,
@@ -151,6 +156,20 @@ let requireDepth = 0;
151156
let statCache = null;
152157
let isPreloading = false;
153158

159+
function internalRequire(module, id) {
160+
validateString(id, 'id');
161+
if (id === '') {
162+
throw new ERR_INVALID_ARG_VALUE('id', id,
163+
'must be a non-empty string');
164+
}
165+
requireDepth++;
166+
try {
167+
return Module._load(id, module, /* isMain */ false);
168+
} finally {
169+
requireDepth--;
170+
}
171+
}
172+
154173
function stat(filename) {
155174
filename = path.toNamespacedPath(filename);
156175
if (statCache !== null) {
@@ -205,6 +224,16 @@ function Module(id = '', parent) {
205224
this.filename = null;
206225
this.loaded = false;
207226
this.children = [];
227+
let redirects;
228+
const manifest = policy()?.manifest;
229+
if (manifest) {
230+
const moduleURL = pathToFileURL(id);
231+
redirects = manifest.getDependencyMapper(moduleURL);
232+
}
233+
setOwnProperty(this, 'require', makeRequireFunction(this, redirects));
234+
// Loads a module at the given file path. Returns that module's
235+
// `exports` property.
236+
this[require_private_symbol] = internalRequire;
208237
}
209238

210239
Module._cache = { __proto__: null };
@@ -927,6 +956,7 @@ Module._load = function(request, parent, isMain) {
927956

928957
if (isMain) {
929958
process.mainModule = module;
959+
setOwnProperty(module.require, 'main', process.mainModule);
930960
module.id = '.';
931961
}
932962

@@ -1113,24 +1143,6 @@ Module.prototype.load = function(filename) {
11131143
cascadedLoader.cjsCache.set(this, exports);
11141144
};
11151145

1116-
1117-
// Loads a module at the given file path. Returns that module's
1118-
// `exports` property.
1119-
Module.prototype.require = function(id) {
1120-
validateString(id, 'id');
1121-
if (id === '') {
1122-
throw new ERR_INVALID_ARG_VALUE('id', id,
1123-
'must be a non-empty string');
1124-
}
1125-
requireDepth++;
1126-
try {
1127-
return Module._load(id, this, /* isMain */ false);
1128-
} finally {
1129-
requireDepth--;
1130-
}
1131-
};
1132-
1133-
11341146
// Resolved path to process.argv[1] will be lazily placed here
11351147
// (needed for setting breakpoint when called with --inspect-brk)
11361148
let resolvedArgv;
@@ -1199,11 +1211,10 @@ function wrapSafe(filename, content, cjsModuleInstance) {
11991211
// Returns exception, if any.
12001212
Module.prototype._compile = function(content, filename) {
12011213
let moduleURL;
1202-
let redirects;
12031214
const manifest = policy()?.manifest;
12041215
if (manifest) {
12051216
moduleURL = pathToFileURL(filename);
1206-
redirects = manifest.getDependencyMapper(moduleURL);
1217+
manifest.getDependencyMapper(moduleURL);
12071218
manifest.assertIntegrity(moduleURL, content);
12081219
}
12091220

@@ -1233,18 +1244,17 @@ Module.prototype._compile = function(content, filename) {
12331244
}
12341245
}
12351246
const dirname = path.dirname(filename);
1236-
const require = makeRequireFunction(this, redirects);
12371247
let result;
12381248
const exports = this.exports;
12391249
const thisValue = exports;
12401250
const module = this;
12411251
if (requireDepth === 0) statCache = new SafeMap();
12421252
if (inspectorWrapper) {
12431253
result = inspectorWrapper(compiledWrapper, thisValue, exports,
1244-
require, module, filename, dirname);
1254+
module.require, module, filename, dirname);
12451255
} else {
12461256
result = ReflectApply(compiledWrapper, thisValue,
1247-
[exports, require, module, filename, dirname]);
1257+
[exports, module.require, module, filename, dirname]);
12481258
}
12491259
hasLoadedAnyUserCJSModule = true;
12501260
if (requireDepth === 0) statCache = null;
@@ -1422,7 +1432,7 @@ Module._preloadModules = function(requests) {
14221432
}
14231433
}
14241434
for (let n = 0; n < requests.length; n++)
1425-
parent.require(requests[n]);
1435+
internalRequire(parent, requests[n]);
14261436
isPreloading = false;
14271437
};
14281438

‎lib/internal/modules/helpers.js‎

Expand all lines: lib/internal/modules/helpers.js
Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,12 @@ const { pathToFileURL, fileURLToPath, URL } = require('internal/url');
2626
const { getOptionValue } = require('internal/options');
2727
const { setOwnProperty } = require('internal/util');
2828

29+
const {
30+
privateSymbols: {
31+
require_private_symbol,
32+
},
33+
} = internalBinding('util');
34+
2935
let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
3036
debug = fn;
3137
});
@@ -95,7 +101,7 @@ function makeRequireFunction(mod, redirects) {
95101
filepath = fileURLToPath(destination);
96102
urlToFileCache.set(href, filepath);
97103
}
98-
return mod.require(filepath);
104+
return mod[require_private_symbol](mod, filepath);
99105
}
100106
}
101107
if (missing) {
@@ -105,11 +111,11 @@ function makeRequireFunction(mod, redirects) {
105111
ArrayPrototypeJoin([...conditions], ', '),
106112
));
107113
}
108-
return mod.require(specifier);
114+
return mod[require_private_symbol](mod, specifier);
109115
};
110116
} else {
111117
require = function require(path) {
112-
return mod.require(path);
118+
return mod[require_private_symbol](mod, path);
113119
};
114120
}
115121

‎src/env_properties.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@
2424
V(napi_type_tag, "node:napi:type_tag") \
2525
V(napi_wrapper, "node:napi:wrapper") \
2626
V(untransferable_object_private_symbol, "node:untransferableObject") \
27-
V(exit_info_private_symbol, "node:exit_info_private_symbol")
27+
V(exit_info_private_symbol, "node:exit_info_private_symbol") \
28+
V(require_private_symbol, "node:require_private_symbol")
2829

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

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

Lines changed: 1 addition & 0 deletions
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‎

Lines changed: 19 additions & 0 deletions
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‎

Lines changed: 9 additions & 0 deletions
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‎

Lines changed: 17 additions & 0 deletions
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/parallel/test-policy-manifest.js‎

Lines changed: 55 additions & 12 deletions
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+
}

0 commit comments

Comments
 (0)
0