Open XmiliaH opened 1 year ago
Most of the new vulnerabilities are cause by node intercepting calls that should be handeled in native code which can be used to break out of the sandbox and I guess it was not found earlier since no one here looked into these cases.
@XmiliaH is the topic of Sandboxing still interesting to you? have you had a look at https://github.com/tc39/proposal-ses ?
I already looked at it some years ago.
Most of the new vulnerabilities are cause by node intercepting calls that should be handeled in native code which can be used to break out of the sandbox and I guess it was not found earlier since no one here looked into these cases.
This is crazy, I would think that this is something that the maintainers would look into since it's obviously very important - so important that it just washes away years of work from the maintainers, as well as everyone else that integrated this package into their projects.
I would love to look into all the details of node and v8, but I only have so much time. And in retrospect it is always easy to tell were one should have looked but when you have a whole code base it is not.
@XmiliaH Thanks for all the hard work on vm2.
I am an entire novice in terms of deep understandings of Javascript and the ECMAScript specification, I am wondering would it be a sound temporary patch if I disallow all the code with the string child_process
or even execSync
to run through vm2?
I know this does not fix anything regarding breakouts, but to the use case that I was working on, if the code contain these strings, someone must be doing something tricky and I have the total rights to block it.
@rklhui this wouldn't help, there are a lot of ways they can be called, like require('child' + '_' + 'process')
.
What about monkey patching child_process?
Or just fork node repository and remove all the critical code you dont need.
That could work. We tried restricting the sandbox and having a very limited sandbox (only Array and Object) globals are available. We tested it against all the posted vulnerabilities, and none of them were breaking the sandbox anymore.
This works perfectly for our use case because we need to run some very basic code, so it's fine.
@rklhui this wouldn't help, there are a lot of ways they can be called, like
require('child' + '_' + 'process')
.
true, how about if I override the require behaviour by extending the behaviour of native Module._load before running the vm2, like how https://github.com/gajus/override-require is doing. Then I could throw an error when the it is requiring things like "child_process" and stop this malignant code execution. Will this be a sound approach?
That could work. We tried restricting the sandbox and having a very limited sandbox (only Array and Object) globals are available. We tested it against all the posted vulnerabilities, and none of them were breaking the sandbox anymore.
This works perfectly for our use case because we need to run some very basic code, so it's fine.
Is there a way to list all that's available? I've found that I have to manually check every class according to the NodeJS documentation.
Probably you just need to patch lib/internal/process/pre_execution.js
Recommend alternative https://github.com/fulcrumapp/v8-sandbox/
I believe the replacement for vm2 is to use the Node.js permission model, which aims to restrict untrusted code boundaries: https://nodejs.org/api/permissions.html#permission-model
Still experimental but powerful, inspired by Deno, which is actually the most reasonable solution for sandboxing today.
Unfortunately, some environments need to achieve this exclusively using Node.sj, so I created https://github.com/Kikobeats/isolated-function which combines permission model and Node.js to limit resource usage.
Do to security issues that cannot be properly addressed I (XmiliaH) will stop maintaining this library.
For a replacement look into isolated-vm.