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

Discontinued #533

Open
XmiliaH opened this issue Jul 9, 2023 · 64 comments
Open

Discontinued #533

XmiliaH opened this issue Jul 9, 2023 · 64 comments

Comments

@XmiliaH
Copy link
Collaborator

XmiliaH commented Jul 9, 2023

Do to security issues that cannot be properly addressed I (XmiliaH) will stop maintaining this library.

For a replacement look into isolated-vm.

@McSheldon
Copy link

Sorry to hear that @XmiliaH @patriksimek
As a library user, where can we find more info on this & test if our app is impacted?
Is the vulnerability disclosed publicly? If you have more update please share here or privately to mcsheldon@proton.me

The migration to isolated-vm won't be a drop in replacement unfortunately.

@AppSecSeanner
Copy link

@XmiliaH can you disclose if there are any non-published escape vulnerabilities against VM2? Did someone disclose a sandbox escape thats too hard to fix with current architecture? It would be helpful to know if we can expect someone to publish a PoC or CVE soon since this was announced.

@XmiliaH
Copy link
Collaborator Author

XmiliaH commented Jul 10, 2023

Xion (SeungHyun Lee) of KAIST Hacking Lab found the vulnerabilities I am not able to fix without changing the whole sandboxing strategy. So there are currently non-published escape vulnerabilities that affects everyone who uses this library to run untrusted code. I do not know if and when they intend to make the vulerability public.

@AppSecSeanner
Copy link

Thank you for the quick reply @XmiliaH - This is very helpful information. It is unfortunate that you can not address them but understandable.

This is a lot to ask but do you know if isolated-vm would be likely vulnerable to similar vulnerabilities?

@XmiliaH
Copy link
Collaborator Author

XmiliaH commented Jul 10, 2023

The problems that piled up are caused by node intercepting calls from the sandbox and therefore arguments not being wrapped in a Proxy. As isolated-vm does create the sandbox on their own and does not rely on the vm module there shouldn't be hooks added by node that causes such issues.

@leesh3288
Copy link

Xion (SeungHyun Lee) of KAIST Hacking Lab found the vulnerabilities I am not able to fix without changing the whole sandboxing strategy. So there are currently non-published escape vulnerabilities that affects everyone who uses this library to run untrusted code. I do not know if and when they intend to make the vulerability public.

I am willing to disclose the specifics including PoC sooner than later, but we would either need to publish a CVE or deprecate the library on npm in advance (preferrably both for clarity) so that scanners/alerts like npm audit or Synk catches it.

@XmiliaH
Copy link
Collaborator Author

XmiliaH commented Jul 10, 2023

I cannot create security advisories, so I cannot publish a CVE. However, I might be able to deprecate the library on npm but would prefer @patriksimek to do both.

@patriksimek
Copy link
Owner

@XmiliaH I will take care of that tomorrow, along with some more messaging.

@AppSecSeanner
Copy link

Thanks @patriksimek and @XmiliaH - I see there is an unpatched RCE CVE for isolated-vm. It may not be a viable option for the community to move to. Are you aware of any other sandboxing alternatives?

Since no VM2 patch will be possible, has KAIST Hacking Lab communicated any responsible disclosure timeline? Like 90 days or similar to give the community time to move to another solution?

@leesh3288
Copy link

@AppSecSeanner The linked RCE in isolated-vm is only applicable when untrusted v8 cache data is passed through CachedDataOptions which is known to be exploitable. Unless you are either exposing isolated-vm into untrusted code or passing untrusted value into CachedDataOptions, as far as I understand the documentation it is meant to be safe. Spotify Backstage is one example that switched from vm2 to isolated-vm as mentioned in their blog, where the devs mention this issue: backstage/backstage#18315 (comment)

As for the responsible disclosure timeline, I have reported this issue in May 23, 2023 to vm2 maintainers. Maintainers and I have looked into this issue but have found no satisfactory solution, and ultimately decided to phase out the library. Specific disclosure timelines on my side has not been explicitly communicated in advance as the maintainers were very prompt and clear with their responses, and I did not feel the need for a specific disclosure policy.

The library has now been officially deprecated, and we are preparing security advisories for the issues. The problem here is that publishing public advisories (CVEs) for the bug ASAP is usually good practice, but in this case I believe that even without an explicit PoC given the description itself is sufficient to write one. We are communicating to roll out an advisory while dealing with this issue but currently do not have fixed deadlines.

@AppSecSeanner
Copy link

Thank you @leesh3288 - This is very helpful information and I appreciate the transparency. I am not familiar with isolated-vm but what you shared makes sense, it looks like CachedDataOptions just exposes ExternalCopy some way.

Thank you also for sharing the backstage blog and related information, this will be helpful to address concerns for anyone else in the community using Snyk.

@patriksimek
Copy link
Owner

Advisories have been published, and the library has been deprecated. We agreed with @leesh3288 to disclose the PoC after 28 days from today to give teams time to migrate their projects.

I am re-posting here what I put into the README.


Dear community,

It's been a truly remarkable journey for me since the vm2 project started nine years ago. The original intent was to devise a method for running untrusted code in Node, with a keen focus on maintaining in-process performance. Proxies, an emerging feature in JavaScript at that time, became our tool of choice for this task.

From the get-go, we recognized the arduous task that lay ahead, as we tried to safeguard against the myriad of escape scenarios JavaScript presented. However, the thrill of the chase kept us going, hopeful that we could overcome these hurdles.

Through the years, this project has seen numerous contributions from passionate individuals. I wish to extend my deepest gratitude to all of you. Special thanks go to @XmiliaH, whose unwavering dedication in maintaining and improving this library over the last 4 years was instrumental to its sustained relevance.

Unfortunately, the growing complexity of Node has brought us to a crossroads. We now find ourselves facing an escape so complicated that fixing it seems impossible. And this isn't about one isolated issue. Recent reports have highlighted that sustaining this project in its current form is not viable in the long term.

Therefore, we must announce the discontinuation of this project.

You may wonder, "What now?"

While this may seem like an end, I see it as an opportunity for you to transition your projects and adapt to a new solution. We would recommend migrating your code to the isolated-vm, a library which employs a slightly different, yet equally effective, approach to sandboxing untrusted code.

Thank you all for your support and understanding during this journey.

Warm Regards,
Patrik Simek

@itamarsher
Copy link

Thank you, @leesh3288, for your security finding and @patriksimek and the rest of the maintainers for your continuous contribution to open source.
My name is Itamar Sher, and I'm a co-founder at Seal Security, a stealth-mode startup that aims to help our users mitigate the risk of open source vulnerabilities.
We are working on an open source repository of security hotfixes that'll help mitigate the risk from known vulnerabilities without forcing you to update or replace your vulnerable dependencies.
As the library is deprecated, we'd love to help develop a security hotfix and release it to the community to enable a smoother transition to other alternatives. We realize this might be challenging due to the complexity of the fix, but we still wish to try.
I'm happy to communicate with you directly to push this through before the public disclosure.

@Uzlopak
Copy link

Uzlopak commented Jul 13, 2023

Well. Couldnt you atleast consider providing a api compatible wrapper around isolated-vm? Then migrating would maybe easier and faster, for now.

@netroy
Copy link

netroy commented Jul 13, 2023

@Uzlopak While that would be really nice, considering this project's license, the project maintainers aren't liable to provide us with a migration path.

not to mention that it would be not easy to create a fully-compatible wrapper on top of isolated-vm, as they'd have to recreate a full module-support (require), and possibly a bunch of node.js specific APIs, as V8 isolates don't have any node.js specific APIs.

It might be better to look at how you were using vm2, and if you don't have a need for module-support, it might make sense to migrate your application directly, instead of creating an unnecessary abstraction.
If you need some small set of modules like crypto or fetch, it might be possible to create shims using pure JS implementations instead.

@Uzlopak
Copy link

Uzlopak commented Jul 13, 2023

Well. It should be atleast be motivated that there is a potential migration guide to the recommended module. Doesnt mean that the maintainers should do it but maybe somebody who uses vm2 and does the migration can atleast provide a PR with a migration guide.

I personally dont use vm2 right now. But i saw in the security risk overview of our enterprise that it is popping up. I told my coworkers that vm2 is notorious for critical security issues because there is always an asian dev, who finds some way to break out of the sandbox, just to visit the npm page and see that vm2 got deprecated.

@AdmWalker
Copy link

@heberallred I believe that is the main problem the escape allows for many different opportunities and while you could shut down access to key methods this would be highly opinionated and that would go against the general purpose of this library and from that I understand the reason for being discontinued.

As with yourself we already execute the VM in a more restrictive process that has additional isolations but still it isn't ideal so applying these extra restrictions in the code. Before this came to light we were looking at using a preprocessor to prevent code we don't want.

@alecgregory
Copy link

For my purposes I was able to transfer to endo for secure execution of untrusted code. Just referencing here in case it works for others.

@danlupascu
Copy link

@XmiliaH I'm just curious how come now you can easily find a bunch of security vulnerabilities that you couldn't find before? I mean there are years of work on this library, and no one could find these vulnerabilities before?

@XmiliaH
Copy link
Collaborator Author

XmiliaH commented Dec 27, 2023

I did not find a bunch of the vulnerabilities. I just fixed the ones reported. As for the last one I was able to find it after taking some time to check the fixes of other forks.

@danlupascu
Copy link

I thought you found them, but that's not important, I mean, how is it possible that they're all coming up now, after years of work?

@XmiliaH
Copy link
Collaborator Author

XmiliaH commented Dec 27, 2023

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.

@netroy
Copy link

netroy commented Dec 27, 2023

@XmiliaH is the topic of Sandboxing still interesting to you? have you had a look at https://github.com/tc39/proposal-ses ?

@XmiliaH
Copy link
Collaborator Author

XmiliaH commented Dec 27, 2023

I already looked at it some years ago.

@danlupascu
Copy link

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.

@XmiliaH
Copy link
Collaborator Author

XmiliaH commented Dec 27, 2023

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.

@rklhui
Copy link

rklhui commented Dec 28, 2023

@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.

@danlupascu
Copy link

danlupascu commented Dec 30, 2023

@rklhui this wouldn't help, there are a lot of ways they can be called, like require('child' + '_' + 'process').

@Uzlopak
Copy link

Uzlopak commented Dec 30, 2023

What about monkey patching child_process?

Or just fork node repository and remove all the critical code you dont need.

@danlupascu
Copy link

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
Copy link

rklhui commented Jan 5, 2024

@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?

@maxless
Copy link

maxless commented Jan 8, 2024

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.

@Uzlopak
Copy link

Uzlopak commented Jan 8, 2024

Probably you just need to patch lib/internal/process/pre_execution.js

@Jobians
Copy link

Jobians commented Feb 2, 2024

Recommend alternative
https://github.com/fulcrumapp/v8-sandbox/

@Kikobeats
Copy link

Kikobeats commented Sep 1, 2024

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.

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

No branches or pull requests