-
Notifications
You must be signed in to change notification settings - Fork 295
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
Not immune to while(1){} #180
Comments
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....... |
This is caused by |
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 |
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.
|
Which ones? I'm not sure there's a better alternative to VM2 in NodeJS, at least not that I've seen |
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. |
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. |
Hey, myself and @harelmo have created a library on top of vm2 that is immune to 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 :) |
Another workaround would be to just disable
|
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. |
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. |
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. |
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. |
Perhaps this should be its own issue, but why does NodeVM not support "timeout"? |
@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. |
@XmiliaH wouldn't implementing a timeout prevent at least from unintended infinite loops? |
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. 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;
}
} |
Good point @XmiliaH and thanks for the example |
@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. |
@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?
|
const vm = new VM({
timeout: 500, // specifies the number of milliseconds to execute code before terminating execution
allowAsync: true
// [...]
}) Does this permutation ( (async function () {
return await db.getData()
})() |
With (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. |
XmiliaH In the following scenario ( 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. |
Try with: const code = `(async()=>{
await(async()=>{})();
while(true){}
})()` |
@XmiliaH Thanks for the clarification 👍. I debugged the Lines 281 to 290 in 62062ad
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? |
They are aware (see https://nodejs.org/api/vm.html#timeout-interactions-with-asynchronous-tasks-and-promises). |
@XmiliaH At least some timeout escapes issues caused by microtask scheduled by
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?
, but reduces the attack surface. |
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. Lines 121 to 140 in 62062ad
|
Following code will not time out.
The text was updated successfully, but these errors were encountered: