Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some bugfixes for v3.9.6 #400

Merged
merged 3 commits into from Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
@@ -1,3 +1,11 @@
v3.9.7 (2022-02-10)
-------------------
[fix] Allow relative require from base script
[fix] Fix issue with modules with exports clause in package json
[fix] Added missing whitelist check before custom require
[fix] Revert plain object toString behavior
[fix] Root path check improved

v3.9.6 (2022-02-08)
-------------------
[fix] Security fixes (XmiliaH)
Expand Down
3 changes: 3 additions & 0 deletions README.md
Expand Up @@ -377,6 +377,9 @@ vm2 ./script.js
## Known Issues

* It is not possible to define a class that extends a proxied class.
* Direct eval does not work.
* Logging sandbox arrays will repeat the array part in the properties.
* Source code transformations can result a different source string for a function.

## Deployment

Expand Down
3 changes: 2 additions & 1 deletion lib/bridge.js
Expand Up @@ -30,7 +30,6 @@ const globalsList = [
'Set',
'WeakSet',
'Promise',
'Object',
'Function'
];

Expand All @@ -48,6 +47,7 @@ const OPNA = 'Operation not allowed on contextified object.';

const thisGlobalPrototypes = {
__proto__: null,
Object: Object.prototype,
Array: Array.prototype
};

Expand Down Expand Up @@ -927,6 +927,7 @@ function createBridge(otherInit, registerProxy) {
thisReflectApply(thisWeakMapSet, mappingOtherToThis, [other, obj]);
}

thisAddProtoMapping(thisGlobalPrototypes.Object, otherGlobalPrototypes.Object);
thisAddProtoMapping(thisGlobalPrototypes.Array, otherGlobalPrototypes.Array);

for (let i = 0; i < globalsList.length; i++) {
Expand Down
2 changes: 2 additions & 0 deletions lib/nodevm.js
Expand Up @@ -389,6 +389,7 @@ class NodeVM extends VM {
const resolvedFilename = this._resolver.pathResolve(code.filename);
dirname = this._resolver.pathDirname(resolvedFilename);
sandboxModule = new (this._Module)(resolvedFilename, dirname);
this._resolver.registerModule(sandboxModule, resolvedFilename, dirname, null, false);
}
} else {
const unresolvedFilename = filename || 'vm.js';
Expand All @@ -397,6 +398,7 @@ class NodeVM extends VM {
const resolvedFilename = this._resolver.pathResolve(filename);
dirname = this._resolver.pathDirname(resolvedFilename);
sandboxModule = new (this._Module)(resolvedFilename, dirname);
this._resolver.registerModule(sandboxModule, resolvedFilename, dirname, null, false);
} else {
sandboxModule = new (this._Module)(null, null);
sandboxModule.id = unresolvedFilename;
Expand Down
59 changes: 42 additions & 17 deletions lib/resolver-compat.js
Expand Up @@ -34,19 +34,24 @@ function escapeRegExp(string) {
return string.replace(/[.*+\-?^${}()|[\]\\]/g, '\\$&'); // $& means the whole matched string
}

function makeExternalMatcher(obj) {
const regexString = escapeRegExp(obj).replace(/\\\\|\//g, '[\\\\/]')
function makeExternalMatcherRegex(obj) {
return escapeRegExp(obj).replace(/\\\\|\//g, '[\\\\/]')
.replace(/\\\*\\\*/g, '.*').replace(/\\\*/g, '[^\\\\/]*').replace(/\\\?/g, '[^\\\\/]');
}

function makeExternalMatcher(obj) {
const regexString = makeExternalMatcherRegex(obj);
return new RegExp(`[\\\\/]node_modules[\\\\/]${regexString}(?:[\\\\/](?!(?:.*[\\\\/])?node_modules[\\\\/]).*)?$`);
}

class TransitiveResolver extends DefaultResolver {
class LegacyResolver extends DefaultResolver {

constructor(builtinModules, checkPath, globalPaths, pathContext, customResolver, hostRequire, compiler, externals) {
constructor(builtinModules, checkPath, globalPaths, pathContext, customResolver, hostRequire, compiler, externals, allowTransitive) {
super(builtinModules, checkPath, globalPaths, pathContext, customResolver, hostRequire, compiler);
this.externals = externals;
this.currMod = undefined;
this.trustedMods = new WeakMap();
this.allowTransitive = allowTransitive;
}

isPathAllowed(path) {
Expand All @@ -55,7 +60,13 @@ class TransitiveResolver extends DefaultResolver {

isPathAllowedForModule(path, mod) {
if (!super.isPathAllowed(path)) return false;
if (mod && (mod.allowTransitive || path.startsWith(mod.path))) return true;
if (mod) {
if (mod.allowTransitive) return true;
if (path.startsWith(mod.path)) {
const rem = path.slice(mod.path.length);
if (!/(?:^|[\\\\/])node_modules(?:$|[\\\\/])/.test(rem)) return true;
}
}
return this.externals.some(regex => regex.test(path));
}

Expand All @@ -65,7 +76,8 @@ class TransitiveResolver extends DefaultResolver {
filename,
path,
paths: this.genLookupPaths(path),
allowTransitive: (direct && trustedParent && trustedParent.allowTransitive) || this.externals.some(regex => regex.test(filename))
allowTransitive: this.allowTransitive &&
((direct && trustedParent && trustedParent.allowTransitive) || this.externals.some(regex => regex.test(filename)))
});
}

Expand Down Expand Up @@ -101,9 +113,14 @@ class TransitiveResolver extends DefaultResolver {
loadJS(vm, mod, filename) {
filename = this.pathResolve(filename);
this.checkAccess(mod, filename);
const trustedMod = this.trustedMods.get(mod);
const script = this.readScript(filename);
vm.run(script, {filename, strict: true, module: mod, wrapper: 'none', dirname: trustedMod ? trustedMod.path : mod.path});
if (this.pathContext(filename, 'js') === 'sandbox') {
const trustedMod = this.trustedMods.get(mod);
const script = this.readScript(filename);
vm.run(script, {filename, strict: true, module: mod, wrapper: 'none', dirname: trustedMod ? trustedMod.path : mod.path});
} else {
const m = this.hostRequire(filename);
mod.exports = vm.readonly(m);
}
}

}
Expand Down Expand Up @@ -276,16 +293,30 @@ function resolverFromOptions(vm, options, override, compiler) {
if (rootPaths) {
const checkedRootPaths = (Array.isArray(rootPaths) ? rootPaths : [rootPaths]).map(f => pa.resolve(f));
checkPath = (filename) => {
return checkedRootPaths.some(path => filename.startsWith(path));
return checkedRootPaths.some(path => {
if (!filename.startsWith(path)) return false;
const len = path.length;
if (filename.length === len) return true;
const sep = filename[len];
return sep === '/' || sep === pa.sep;
});
};
} else {
checkPath = () => true;
}

let newCustomResolver = defaultCustomResolver;
let externals = undefined;
let external = undefined;
if (customResolver) {
let externalCache;
newCustomResolver = (resolver, x, path, extList) => {
if (external && !(resolver.pathIsAbsolute(x) || resolver.pathIsRelative(x))) {
if (!externalCache) {
externalCache = external.map(ext => new RegExp(makeExternalMatcherRegex(ext)));
}
if (!externalCache.some(regex => regex.test(x))) return undefined;
}
const resolved = customResolver(x, path);
if (!resolved) return undefined;
if (externals) externals.push(new RegExp('^' + escapeRegExp(resolved)));
Expand All @@ -297,7 +328,6 @@ function resolverFromOptions(vm, options, override, compiler) {
return new DefaultResolver(builtins, checkPath, [], () => context, newCustomResolver, hostRequire, compiler);
}

let external;
let transitive = false;
if (Array.isArray(externalOpt)) {
external = externalOpt;
Expand All @@ -306,12 +336,7 @@ function resolverFromOptions(vm, options, override, compiler) {
transitive = context === 'sandbox' && externalOpt.transitive;
}
externals = external.map(makeExternalMatcher);
if (transitive) return new TransitiveResolver(builtins, checkPath, [], () => context, newCustomResolver, hostRequire, compiler, externals);
const nextCheckPath = checkPath;
checkPath = (filename) => {
return nextCheckPath(filename) && externals.some(regex => regex.test(filename));
};
return new DefaultResolver(builtins, checkPath, [], () => context, newCustomResolver, hostRequire, compiler);
return new LegacyResolver(builtins, checkPath, [], () => context, newCustomResolver, hostRequire, compiler, externals, transitive);
}

exports.resolverFromOptions = resolverFromOptions;
6 changes: 3 additions & 3 deletions lib/resolver.js
Expand Up @@ -508,15 +508,15 @@ class DefaultResolver extends Resolver {
// 2. If X does not match this pattern or DIR/NAME/package.json is not a file,
// return.
if (!res) return undefined;
const scope = this.pathConcat(dir, res[0]);
const scope = this.pathConcat(dir, res[1]);
const pack = this.readPackage(scope);
if (!pack) return undefined;
// 3. Parse DIR/NAME/package.json, and look for "exports" field.
// 4. If "exports" is null or undefined, return.
if (!pack.exports) return undefined;
// 5. let MATCH = PACKAGE_EXPORTS_RESOLVE(pathToFileURL(DIR/NAME), "." + SUBPATH,
// `package.json` "exports", ["node", "require"]) defined in the ESM resolver.
const match = this.packageExportsResolve(scope, '.' + res[1], pack.exports, ['node', 'require'], extList);
const match = this.packageExportsResolve(scope, '.' + (res[2] || ''), pack.exports, ['node', 'require'], extList);
// 6. RESOLVE_ESM_MATCH(MATCH)
return this.resolveEsmMatch(match, x, extList);
}
Expand Down Expand Up @@ -777,7 +777,7 @@ class DefaultResolver extends Resolver {
for (let i = 0; i < keys.length; i++) {
const p = keys[i];
// 1. If p equals "default" or conditions contains an entry for p, then
if (p === 'default' || conditions.contains(p)) {
if (p === 'default' || conditions.includes(p)) {
// a. Let targetValue be the value of the p property in target.
const targetValue = target[p];
// b. Let resolved be the result of PACKAGE_TARGET_RESOLVE( packageURL, targetValue, subpath, pattern, internal, conditions).
Expand Down
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -13,7 +13,7 @@
"alcatraz",
"contextify"
],
"version": "3.9.6",
"version": "3.9.7",
"main": "index.js",
"sideEffects": false,
"repository": "github:patriksimek/vm2",
Expand Down
1 change: 1 addition & 0 deletions test/node_modules/with-exports/main.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions test/node_modules/with-exports/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions test/nodevm.js
Expand Up @@ -272,6 +272,54 @@ describe('modules', () => {
assert.equal(vm.run("module.exports = require('module-main-without-extension').bar()", __filename), 1);
});

it('module with exports', () => {
const vm = new NodeVM({
require: {
external: [
'with-exports'
]
}
});

assert.strictEqual(vm.run("module.exports = require('with-exports')", __filename).ok, true);

});

it('whitelist check before custom resolver', () => {
const vm = new NodeVM({
require: {
external: [],
resolve: () => {
throw new Error('Unexpected');
},
},
});

assert.throws(() => vm.run("require('mocha')", __filename), /Cannot find module 'mocha'/);
});

it('root path checking', () => {
const vm = new NodeVM({
require: {
external: true,
root: `${__dirname}/node_modules/module`
},
});

assert.throws(() => vm.run("require('module2')", __filename), /Cannot find module 'module2'/);
});

it('relative require not allowed to enter node modules', () => {
const vm = new NodeVM({
require: {
external: ['mocha'],
root: `${__dirname}`
},
});

assert.throws(() => vm.run("require('./node_modules/module2')", __filename), /Cannot find module '\.\/node_modules\/module2'/);
});

it('arguments attack', () => {
let vm = new NodeVM;

Expand Down
30 changes: 30 additions & 0 deletions test/vm.js
Expand Up @@ -350,6 +350,36 @@ describe('contextify', () => {
assert.strictEqual(vm.run('test.error.constructor.constructor === Function;'), true);
});

it('tostring', () => {
const list = [
'Object',
'Array',
'Number',
'String',
'Boolean',
'Date',
'RegExp',
'Map',
'WeakMap',
'Set',
'WeakSet',
'Function',
'RangeError',
'ReferenceError',
'SyntaxError',
'TypeError',
'EvalError',
'URIError',
'Error'
];
const gen = vm.run('name => new (global[name])()');
const oToString = Object.prototype.toString;
for (let i = 0; i < list.length; i++) {
const obj = list[i];
assert.strictEqual(oToString.call(gen(obj)), oToString.call(new (global[obj])()));
}
});

after(() => {
vm = null;
});
Expand Down