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

vm2 malforms arrays when passing them outside the sandbox boundary #265

Closed
kalevalp opened this issue Feb 17, 2020 · 4 comments
Closed

vm2 malforms arrays when passing them outside the sandbox boundary #265

kalevalp opened this issue Feb 17, 2020 · 4 comments

Comments

@kalevalp
Copy link

When an array produced within the sandbox is passed to a function outside of the sandbox boundary, the passed array contains additional elements (fields, maybe?). This, in turn, causes problems for child_process.spawn, which fails when running with a malformed array as a parameter.

The following code example demonstrates the problem:

const {NodeVM} = require("vm2");
const vm = new NodeVM({
    require: {
        mock: {
            'dummy': (arr) => console.log(arr)
        }}});

const script = `
const dummy = require('dummy');

const arr = ['a','b','c'];
dummy(arr);   
`

vm.run(script);

The result of executing the above script is the following printout:

[ 'a', 'b', 'c', '0': 'a', '1': 'b', '2': 'c' ]

Note the additional '0': 'a', '1': 'b', '2': 'c' elements.

This causes a failure when running child_process.spawn from within the sandbox. Specifically, the spawn function cannot handle the malformed array passed as the arguments parameter, and instead passes it as a null, which usually causes the executed child process to fail.

The following code exemplifies this behaviour:

const {NodeVM} = require("vm2");
const vm = new NodeVM({require: {builtin: ['child_process']}});

const script = `
const proc = require('child_process').spawn('ls', ['./']);
proc.stdout.on('data', (data) => console.log('stdout:', data.toString()));
proc.stderr.on('data', (data) => console.log('stderr:', data.toString()));
proc.on('close', (code) => console.log('exit code:', code));
`

vm.run(script);

The above execution results in the following printout:

stderr: A NULL argv[0] was passed through an exec system call.

exit code: null
@XmiliaH
Copy link
Collaborator

XmiliaH commented Feb 17, 2020

The extra properties reported by console.log is a problem with node v10, see #198, these properties do not exisit. The spawn seems to be an seperate issue with node having problems working with proxied arrays. I don't see how we could fix this issue.

@kalevalp
Copy link
Author

Is it something in particular with how you use proxies in vm2 that causes the problem?
Simply wrapping the array in a proxy does not seem to reproduce the problem outside of vm2.

The following code works correctly:

const p = new Proxy(['./'], {});
const proc = require('child_process').spawn('ls', p);

@XmiliaH
Copy link
Collaborator

XmiliaH commented Feb 17, 2020

Thats the case since spawn will call slice(0) on the array and so the proxy will be removed. The proxies we use for vm2 however will wrap return values, so the return from slice will also be an proxy.
This will result in an error:

const handler = {
		get(target, key) {
			if(key==='raw')
				return target;
			return this.wrap(target[key]);
		},
		apply(target, thiz, args) {
			return this.wrap(target.apply(this.unwrap(thiz), args));
		},
		wrap(obj) {
			if(obj===null || (typeof obj!=='object' && typeof obj!=='function'))
				return obj;
			return new Proxy(obj, this);
		},
		unwrap(obj) {
			return obj.raw || obj;
		}
};
	
const proc = require('child_process').spawn('ls', new Proxy(['./'], handler));
proc.stdout.on('data', (data) => console.log('stdout:', data.toString()));
proc.stderr.on('data', (data) => console.log('stderr:', data.toString()));
proc.on('close', (code) => console.log('exit code:', code));

@aapoalas
Copy link

Is there anything that the user can do to overcome this problem? I have the same issue with Node 8.17.0 and vm2 3.8.4 and would prefer not to remove the sandboxing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants