Not immune to while(1){}

[XmiliaH]

Following code will not time out.

const {VM} = require('vm2');
new VM({timeout:1}).run(`
		function main(){
		while(1){}
	}
	new Proxy({}, {
		getPrototypeOf(t){
			global.main();
		}
	})`);

[anonyco]

There is likely no good solution to this. NodeJS is single process. while(1){} protection requires multi-process so that the checker does not get blocked by the while(1){}. Maybe if we could rekindle the SpiderNode project.......

[iamahuman]

This is caused by Decontextify.value testing for the guest object's prototype with instanceOf. Node.js vm module is of course immune to this. A poor man's patch might be to backdoor Proxy so that we could detect any Proxy objects from the sandbox, but an ideal approach would be to just never trust any objects and functions from it and put contextify.js into sandbox, using primitive values to communicate and handles for referencing guest objects.

[joelgriffith]

Just for folks to see, browserless uses a child-process and message-passing to talk with the child running the untrusted code. It's a bit of a setup task, but once done you'll be immune to while(1){} attacks. The source is here.

[iamahuman]

The whole point of this project seems to be to not spawn a whole separate OS process. Otherwise there are many solutions available which are much more secure.

[joelgriffith]

Otherwise there are many solutions available which are much more secure.

Which ones? I'm not sure there's a better alternative to VM2 in NodeJS, at least not that I've seen

[chung-leong]

The maintainer of this library pointed to this alternative in issue #80:

https://github.com/laverdet/isolated-vm

I haven't tried it yet myself.

[sebastien-ma]

I only run untrusted code in child process. Can't risk running it in the main V8 instance especially when you know it's single threaded.

The problem of calling V8 engine directly is lack of importing module. In my case I need to import request module to allow HTTP operations. It also has to be asynchronous.

I find vm2 + child_process a good combination.

[idan-at]

Hey,

myself and @harelmo have created a library on top of vm2 that is immune to while(1) {}.

Similar to what tian-ma mentioned, but we use node.js worker threads instead of child processes.

take a look here: https://www.npmjs.com/package/isolated-runtime and let us know what you think :)

[Yogu]

Another workaround would be to just disable Proxy within the sandbox, right?

const vm = new VM({
    sandbox: {
        Proxy: undefined
    },
    timeout: 100
});

[XmiliaH]

Using Promise it is also possible to write code that never times out:

"use strict";
const {VM} = require('vm2');
const untrusted = '(' + function(){
	Promise.resolve().then(a=>{
		while(1){}
	});
}+')()';
try{
	console.log(new VM({timeout:10}).run(untrusted));
}catch(x){
	console.log(x);
}

Problem here is that .then will queue the function to be called later in the microtask queue.
To fix this we need to write our own Promise.then & related functions or disable Promise.

[XmiliaH]

Also this works:

const {VM} = require('vm2');

const script = '(' + function() {
	(async function x(){
		await {then: y=>y()};
		while(1){}
	})();
}+')()';

new VM({timeout:10}).run(script);

So just overriding Promise won't work.
No idea how to fix this.

[typeofweb]

While this remains open, we should add a clarification to the README to warn potential users. This vulnerability allows one to escape the sandbox and thus it defeats the purpose of the library.

[XmiliaH]

Readme was updated 77f5265#diff-04c6e90faac2675aa89e2176d2eec7d8. And I don't know how this could be used to escape the sandbox, you only can escape the intended timeframe the script should run in.

[crowder]

Perhaps this should be its own issue, but why does NodeVM not support "timeout"?

[XmiliaH]

@crowder since in NodeVM the setTimeout, setInterval & setImmediate exists, it would be easy to circumvent the timeout. Maybe there are other reasons too, but I don't work with NodeVM, only VM.

[szydan]

@XmiliaH wouldn't implementing a timeout prevent at least from unintended infinite loops?
The malicious while(true) + setTimeout combination would still be an issue but any simple unintended infinite loop done by mistake could be handled by supporting timeout

[XmiliaH]

My concern is that NodeVM allowes to require modules which might be host modules and timing out while they change global state might be bad. Also the NodeVM returns a module and users quite likely would call functions exposed by the module, however the timeout would only apply to the module loading and not the function calls.
If you want to call a function with a timeout you can just implement it yourself like

function doWithTimeout(fn, timeout) {
	let ctx = CACHE.timeoutContext;
	let script = CACHE.timeoutScript;
	if (!ctx) {
		CACHE.timeoutContext = ctx = vm.createContext();
		CACHE.timeoutScript = script = new vm.Script('fn()', {
			filename: 'timeout_bridge.js',
			displayErrors: false
		});
	}
	ctx.fn = fn;
	try {
		return script.runInContext(ctx, {
			displayErrors: false,
			timeout
		});
	} finally {
		ctx.fn = null;
	}
}

[szydan]

Good point @XmiliaH and thanks for the example
I see your example is using vm package and not vm2
Would be nice to be able to use vm2 and still be able to timeout it out

[XmiliaH]

@szydan The code I posted is used by VM2 to timeout the VM. If the NodeVM had a timeout too, it would use the same functionality.

[dejavughost]

Readme was updated 77f5265#diff-04c6e90faac2675aa89e2176d2eec7d8. And I don't know how this could be used to escape the sandbox, you only can escape the intended timeframe the script should run in.

@XmiliaH I am a bit confused now. Does this limitation still apply?

Regarding the following statements. Could you please elaborate a bit more, so that I can understand?

timeout - Script timeout in milliseconds. WARNING: You might want to use this option together with allowAsync=false

IMPORTANT: Timeout is only effective on synchronous code that you run through run. Timeout does NOT work on any method returned by VM.

[dejavughost]

@XmiliaH

  const vm = new VM({
    timeout: 500, // specifies the number of milliseconds to execute code before terminating execution
    allowAsync: true
    // [...]
  })

Does this permutation (allowAsync=true and timeout=500) work as expected in all known contexts for async code? For example:

(async function () {
  return await db.getData()
})()

[XmiliaH]

With allowAsync=true and a timeout the following code

(async function () {
  while(1){}
})()

would run the main chunk with the timeout but all async micro tasks are not affected by the timeout, so the while loop would not be stopped.

[arleytm]

so the while loop would not be stopped.

XmiliaH In the following scenario (allowAsync: true and timeout: 500), the script execution timed out after 500ms as expected 🤷‍♂️.

const { VM } = require('vm2')

async function run(code) {
  const vm = new VM({
    timeout: 500, // specifies the number of milliseconds to execute code before terminating execution
    allowAsync: true
  })

  try {
    await vm.run(code)
  } catch (error) {
    console.log(error.message) // script execution timed out after 500ms
  }
}

const code = `(async function () {
  while (true) {}
})()`

run(code)

Tested with vm2 version 3.9.11.

[XmiliaH]

Try with:

const code = `(async()=>{
  await(async()=>{})();
  while(true){}
})()`

[arleytm]

const code = `(async()=>{
  await(async()=>{})();
  while(true){}
})()`

@XmiliaH Thanks for the clarification 👍.

I debugged the vm2.run(...) method and found that it uses the script.runInContext(contextifiedObject[, options]) API under the hood. Interestingly, currently, in the documentation of the vm (Node.js core module), the async/timeout limitation seems not to be mentioned.

vm2/lib/vm.js

Lines 281 to 290 in 62062ad

	const runScript = (script) => {
	// This closure is intentional to hide _context and bridge since the allow to access the sandbox directly which is unsafe.
	let ret;
	try {
	ret = script.runInContext(_context, DEFAULT_RUN_OPTIONS);
	} catch (e) {
	throw bridge.from(e);
	}
	return bridge.from(ret);
	};

In this scenario (in Node.js core vm land), I would have expected the execution to be terminated, and an Error to be thrown and then be propagated to the vm2 bridge. I have reproduced the same behavior using the node core vm module. For example:

const vm = require('node:vm')
const context = vm.createContext()
const code = `
(async () => {
  await (async () => {})()
  while (true) {}
})()
`
const scriptCode = new vm.Script(code)
scriptCode.runInContext(context, { timeout: 500 }) // this run does not timeout after 500 ms

Do you know if the Node.js runtime team is aware of this limitation?

[XmiliaH]

They are aware (see https://nodejs.org/api/vm.html#timeout-interactions-with-asynchronous-tasks-and-promises).

[arleytm]

@XmiliaH At least some timeout escapes issues caused by microtask scheduled by Promise.resolve() or the like can be addressed by passing microtaskMode: 'afterEvaluate' option to the code that creates the context. For example:

const vm = require('node:vm')

function loop() {
  while (1) console.log(Date.now())
}

vm.runInNewContext(
  'Promise.resolve().then(() => loop());',
  { loop, console },
  { timeout: 5, microtaskMode: 'afterEvaluate' }
)

https://nodejs.org/api/vm.html#timeout-interactions-with-asynchronous-tasks-and-promises

Have you considered enabling or providing a configuration option for this microtask mode?
This mode does not work in all scenarios — not bulletproof →

Promise callbacks are entered into the microtask queue of the context in which they were created. For example, if () => loop() is replaced with just loop in the above example, then loop will be pushed into the global microtask queue, because it is a function from the outer (main) context, and thus will also be able to escape the timeout.

, but reduces the attack surface.

[XmiliaH]

I know of the option. However, I did not really look into it as it is not bulletproof and we use a passthrough context for the timeout which should only capture the microtasks from that context and not the sandbox one so it would need a redesign of the timeout functionality.

vm2/lib/vm.js

Lines 121 to 140 in 62062ad

	function doWithTimeout(fn, timeout) {
	if (!cacheTimeoutContext) {
	cacheTimeoutContext = createContext();
	cacheTimeoutScript = new Script('fn()', {
	__proto__: null,
	filename: 'timeout_bridge.js',
	displayErrors: false
	});
	}
	cacheTimeoutContext.fn = fn;
	try {
	return cacheTimeoutScript.runInContext(cacheTimeoutContext, {
	__proto__: null,
	displayErrors: false,
	timeout
	});
	} finally {
	cacheTimeoutContext.fn = null;
	}
	}