Skip to content

Commit

Permalink
policy: makeRequireFunction on mainModule.require
Browse files Browse the repository at this point in the history
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: #358
Reviewed-by: Bradley Farias <bradley.meck@gmail.com>
Reviewed-by: Michael Dawson <midawson@redhat.com>
  • Loading branch information
RafaelGSS and bmeck committed Feb 14, 2023
1 parent 10a4c47 commit 6d17b69
Show file tree
Hide file tree
Showing 9 changed files with 147 additions and 40 deletions.
59 changes: 35 additions & 24 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ const path = require('path');
const { sep } = path;
const { internalModuleStat } = internalBinding('fs');
const { safeGetenv } = internalBinding('credentials');
const {
privateSymbols: {
require_private_symbol,
},
} = internalBinding('util');

const {
getCjsConditions,
initializeCjsConditions,
Expand Down Expand Up @@ -151,6 +157,20 @@ let requireDepth = 0;
let statCache = null;
let isPreloading = false;

function internalRequire(module, id) {
validateString(id, 'id');
if (id === '') {
throw new ERR_INVALID_ARG_VALUE('id', id,
'must be a non-empty string');
}
requireDepth++;
try {
return Module._load(id, module, /* isMain */ false);
} finally {
requireDepth--;
}
}

function stat(filename) {
filename = path.toNamespacedPath(filename);
if (statCache !== null) {
Expand Down Expand Up @@ -205,6 +225,16 @@ function Module(id = '', parent) {
this.filename = null;
this.loaded = false;
this.children = [];
let redirects;
const manifest = policy()?.manifest;
if (manifest) {
const moduleURL = pathToFileURL(id);
redirects = manifest.getDependencyMapper(moduleURL);
}
setOwnProperty(this, 'require', makeRequireFunction(this, redirects));
// Loads a module at the given file path. Returns that module's
// `exports` property.
this[require_private_symbol] = internalRequire;
}

Module._cache = { __proto__: null };
Expand Down Expand Up @@ -927,6 +957,7 @@ Module._load = function(request, parent, isMain) {

if (isMain) {
process.mainModule = module;
setOwnProperty(module.require, 'main', process.mainModule);
module.id = '.';
}

Expand Down Expand Up @@ -1113,24 +1144,6 @@ Module.prototype.load = function(filename) {
cascadedLoader.cjsCache.set(this, exports);
};


// Loads a module at the given file path. Returns that module's
// `exports` property.
Module.prototype.require = function(id) {
validateString(id, 'id');
if (id === '') {
throw new ERR_INVALID_ARG_VALUE('id', id,
'must be a non-empty string');
}
requireDepth++;
try {
return Module._load(id, this, /* isMain */ false);
} finally {
requireDepth--;
}
};


// Resolved path to process.argv[1] will be lazily placed here
// (needed for setting breakpoint when called with --inspect-brk)
let resolvedArgv;
Expand Down Expand Up @@ -1199,11 +1212,10 @@ function wrapSafe(filename, content, cjsModuleInstance) {
// Returns exception, if any.
Module.prototype._compile = function(content, filename) {
let moduleURL;
let redirects;
const manifest = policy()?.manifest;
if (manifest) {
moduleURL = pathToFileURL(filename);
redirects = manifest.getDependencyMapper(moduleURL);
manifest.getDependencyMapper(moduleURL);
manifest.assertIntegrity(moduleURL, content);
}

Expand Down Expand Up @@ -1233,18 +1245,17 @@ Module.prototype._compile = function(content, filename) {
}
}
const dirname = path.dirname(filename);
const require = makeRequireFunction(this, redirects);
let result;
const exports = this.exports;
const thisValue = exports;
const module = this;
if (requireDepth === 0) statCache = new SafeMap();
if (inspectorWrapper) {
result = inspectorWrapper(compiledWrapper, thisValue, exports,
require, module, filename, dirname);
module.require, module, filename, dirname);
} else {
result = ReflectApply(compiledWrapper, thisValue,
[exports, require, module, filename, dirname]);
[exports, module.require, module, filename, dirname]);
}
hasLoadedAnyUserCJSModule = true;
if (requireDepth === 0) statCache = null;
Expand Down Expand Up @@ -1422,7 +1433,7 @@ Module._preloadModules = function(requests) {
}
}
for (let n = 0; n < requests.length; n++)
parent.require(requests[n]);
internalRequire(parent, requests[n]);
isPreloading = false;
};

Expand Down
12 changes: 9 additions & 3 deletions lib/internal/modules/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ const { pathToFileURL, fileURLToPath, URL } = require('internal/url');
const { getOptionValue } = require('internal/options');
const { setOwnProperty } = require('internal/util');

const {
privateSymbols: {
require_private_symbol,
},
} = internalBinding('util');

let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
debug = fn;
});
Expand Down Expand Up @@ -95,7 +101,7 @@ function makeRequireFunction(mod, redirects) {
filepath = fileURLToPath(destination);
urlToFileCache.set(href, filepath);
}
return mod.require(filepath);
return mod[require_private_symbol](mod, filepath);
}
}
if (missing) {
Expand All @@ -105,11 +111,11 @@ function makeRequireFunction(mod, redirects) {
ArrayPrototypeJoin([...conditions], ', ')
));
}
return mod.require(specifier);
return mod[require_private_symbol](mod, specifier);
};
} else {
require = function require(path) {
return mod.require(path);
return mod[require_private_symbol](mod, path);
};
}

Expand Down
3 changes: 2 additions & 1 deletion src/env_properties.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
V(napi_type_tag, "node:napi:type_tag") \
V(napi_wrapper, "node:napi:wrapper") \
V(untransferable_object_private_symbol, "node:untransferableObject") \
V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array")
V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array") \
V(require_private_symbol, "node:require_private_symbol")

// Symbols are per-isolate primitives but Environment proxies them
// for the sake of convenience.
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/policy-manifest/main-module-bypass.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
process.mainModule.require('os').cpus();
19 changes: 19 additions & 0 deletions test/fixtures/policy-manifest/object-define-property-bypass.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
let requires = new WeakMap()
Object.defineProperty(Object.getPrototypeOf(module), 'require', {
get() {
return requires.get(this);
},
set(v) {
requires.set(this, v);
process.nextTick(() => {
let fs = Reflect.apply(v, this, ['fs'])
if (typeof fs.readFileSync === 'function') {
process.exit(1);
}
})
return requires.get(this);
},
configurable: true
})

require('./valid-module')
9 changes: 9 additions & 0 deletions test/fixtures/policy-manifest/onerror-exit.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"onerror": "exit",
"scopes": {
"file:": {
"integrity": true,
"dependencies": {}
}
}
}
17 changes: 17 additions & 0 deletions test/fixtures/policy-manifest/onerror-resource-exit.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"onerror": "exit",
"resources": {
"./object-define-property-bypass.js": {
"integrity": true,
"dependencies": {
"./valid-module": true
}
},
"./valid-module.js": {
"integrity": true,
"dependencies": {
"fs": true
}
}
}
}
Empty file.
67 changes: 55 additions & 12 deletions test/parallel/test-policy-manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,58 @@ const assert = require('assert');
const { spawnSync } = require('child_process');
const fixtures = require('../common/fixtures.js');

const policyFilepath = fixtures.path('policy-manifest', 'invalid.json');

const result = spawnSync(process.execPath, [
'--experimental-policy',
policyFilepath,
'./fhqwhgads.js',
]);

assert.notStrictEqual(result.status, 0);
const stderr = result.stderr.toString();
assert.match(stderr, /ERR_MANIFEST_INVALID_SPECIFIER/);
assert.match(stderr, /pattern needs to have a single trailing "\*"/);
{
const policyFilepath = fixtures.path('policy-manifest', 'invalid.json');
const result = spawnSync(process.execPath, [
'--experimental-policy',
policyFilepath,
'./fhqwhgads.js',
]);

assert.notStrictEqual(result.status, 0);
const stderr = result.stderr.toString();
assert.match(stderr, /ERR_MANIFEST_INVALID_SPECIFIER/);
assert.match(stderr, /pattern needs to have a single trailing "\*"/);
}

{
const policyFilepath = fixtures.path('policy-manifest', 'onerror-exit.json');
const result = spawnSync(process.execPath, [
'--experimental-policy',
policyFilepath,
'-e',
'require("os").cpus()',
]);

assert.notStrictEqual(result.status, 0);
const stderr = result.stderr.toString();
assert.match(stderr, /ERR_MANIFEST_DEPENDENCY_MISSING/);
assert.match(stderr, /does not list module as a dependency specifier for conditions: require, node, node-addons/);
}

{
const policyFilepath = fixtures.path('policy-manifest', 'onerror-exit.json');
const mainModuleBypass = fixtures.path('policy-manifest', 'main-module-bypass.js');
const result = spawnSync(process.execPath, [
'--experimental-policy',
policyFilepath,
mainModuleBypass,
]);

assert.notStrictEqual(result.status, 0);
const stderr = result.stderr.toString();
assert.match(stderr, /ERR_MANIFEST_DEPENDENCY_MISSING/);
assert.match(stderr, /does not list os as a dependency specifier for conditions: require, node, node-addons/);
}

{
const policyFilepath = fixtures.path('policy-manifest', 'onerror-resource-exit.json');
const objectDefinePropertyBypass = fixtures.path('policy-manifest', 'object-define-property-bypass.js');
const result = spawnSync(process.execPath, [
'--experimental-policy',
policyFilepath,
objectDefinePropertyBypass,
]);

assert.strictEqual(result.status, 0);
}

0 comments on commit 6d17b69

Please sign in to comment.