vm2 icon indicating copy to clipboard operation
vm2 copied to clipboard

Discontinued

Open XmiliaH opened this issue 1 year ago • 64 comments

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

For a replacement look into isolated-vm.

XmiliaH avatar Jul 09 '23 14:07 XmiliaH

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 [email protected]

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

McSheldon avatar Jul 10 '23 15:07 McSheldon

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

AppSecSeanner avatar Jul 10 '23 15:07 AppSecSeanner

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.

XmiliaH avatar Jul 10 '23 15:07 XmiliaH

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?

AppSecSeanner avatar Jul 10 '23 15:07 AppSecSeanner

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.

XmiliaH avatar Jul 10 '23 15:07 XmiliaH

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.

leesh3288 avatar Jul 10 '23 15:07 leesh3288

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.

XmiliaH avatar Jul 10 '23 15:07 XmiliaH

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

patriksimek avatar Jul 10 '23 17:07 patriksimek

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?

AppSecSeanner avatar Jul 10 '23 22:07 AppSecSeanner

@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: https://github.com/backstage/backstage/issues/18315#issuecomment-1596184737

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.

leesh3288 avatar Jul 11 '23 03:07 leesh3288

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.

AppSecSeanner avatar Jul 11 '23 14:07 AppSecSeanner

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

patriksimek avatar Jul 12 '23 12:07 patriksimek

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.

itamarsher avatar Jul 12 '23 15:07 itamarsher

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

Uzlopak avatar Jul 13 '23 13:07 Uzlopak

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

netroy avatar Jul 13 '23 13:07 netroy

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.

Uzlopak avatar Jul 13 '23 16:07 Uzlopak

VM2 authors and contributors, thank you for all your hard work; the scope of the task was immense. Despite acknowledging that the entire VM2 approach resembles patching a leaking bucket with unpredictable number of holes, I nevertheless appreciate the simplicity and flexibility of the VM2 API. I've shared some information about VM2 and isolated-vm in my blog post, Running untrusted JavaScript in Node.js

Like many other users of this library, I am exploring possible migration paths (considering the heavier containerization ones). Here is my perspective: CloudFlare employs Node.js Isolates to run CloudFlare Workers - the same technology that isolated-vm uses. Here is their open-source implementation: workerd - as far as I understand, each workerd process is designed to run for a long time and have callbacks inside.

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.

Cloudflare Workers can't import 3rd party packages, as well, so CloudFlare support suggests to use bundlers to embed js code of the external package into the worker code

Cloudflare has recently deployed Node.js lib compatibility for workerd (blog post)

Amazon uses Firecracker to run AWS Lambda functions. Firecracker seems to be a middle ground between running a Docker container and launching a V8 Isolate in terms of overhead and startup performance - from my understanding, in terms of startup time, Firecracker VMs are much faster than Docker, but obviously slower than launching Node.js Isolates (I plan to do some benchmarks if I have time for this)

Great Fly.io post on all the modern options to isolate the code execution: https://fly.io/blog/sandboxing-and-workload-isolation/

Fly.io also uses Firecracker to isolate the untrusted code.

restyler avatar Jul 18 '23 11:07 restyler

All, thanks as well for all the hard work and the great tool that was/is vm2. It seems that isolated-vm is a lot less well documented (and used) compared to vm2. We use vm2 to run 'integration' scripts that can be customised by end-users to get data from web services, make an Excel file and e-mail it out, to simple data transformation, etc. Running those scripts without 3rd party libraries would be a pain. I tried bundling libraries with a script, and then executing them, but to no avail - and then you also run the risk of bundling 'too much' - creating your own risks.

Is there really no alternative other than running your own firecracker instance - which seems a lot more high-maintenance compared to using vm2?

dannyhaak avatar Jul 21 '23 09:07 dannyhaak

Yeah, i got alot of hate for my comment for a migration guide or a thin layer wrapper.

I also checked the shameless advertisement of @restyler for his useless blog post. Nothing of relevance there for migrating from vm2 to isolated-vm.

People need now a migration guide. As @dannyhaak wrote: The isolated-vm is not documented properly.

I dont say, that the maintainers of this module should provide one. I say, they should atleast encourage someone who is migrating to create a simple migration.md and provide it as a PR. Doesnt need to be complete. But other devs, who want to migrate and think that something is missing in the migration.md should also encouraged to add more content to the migration.md.

Again: People need a migration guide.

I would do it myself, but I have no time for a deep dive into vm2 api and isolated-vm api.

Uzlopak avatar Jul 21 '23 12:07 Uzlopak

As discussed with @leesh3288, we decided to postpone the disclosure for another 28 days to the 5th of September.

A tip from @leesh3288 as we also briefly touched on the problem of isolated-vm being a native dependency:

As an alternative for isolated-vm, a "pure" solution I have seen is https://github.com/TooTallNate/proxy-agents/pull/224 which uses a whole QuickJS engine compiled into WASM to run insecure code inside it - this might be a feasible solution on some cases where native libraries are not desirable or just not working.

patriksimek avatar Jul 24 '23 16:07 patriksimek

I tried quickjs-emscripten before but its unfortunately much slower than vm2/isolated-vm... e.g. looping 10 million times: 30ms vs 2.6s

this might be a feasible solution on some cases where native libraries are not desirable

The latest isolated-vm release uses precompiled binaries now, should be less of an installation hassle for users (especially windows which often doesn't have a compiler toolchain and python)

j4k0xb avatar Jul 24 '23 16:07 j4k0xb

FWIW, I've done a migration from vm2 to isolated-vm and the most useful information I found came from this comment and the linked code. Basically, you need to think about which code executes where and how the data (ie. method arguments, return values) is transferred. And whether you need promises or not etc.

HTH someone else a little bit with their migration or at least to gain confidence that a migration is feasible in principle.

mklinke avatar Jul 26 '23 08:07 mklinke

do not know if and when they intend to make the vulnerability public.

According to the GitHub Advisory Database PoC for both will be disclosed "on or after the 5th of September [2023]".

Here's the source of this info: https://github.com/advisories/GHSA-g644-9gfx-q4q4 https://github.com/advisories/GHSA-cchq-frgv-rjh5

mirekphd avatar Aug 15 '23 16:08 mirekphd

The exploit has been published by @leesh3288 and can be found here : https://gist.github.com/leesh3288/e4aa7b90417b0b0ac7bcd5b09ac7d3bd

S0obi avatar Sep 10 '23 21:09 S0obi

@S0obi there is a second one: https://gist.github.com/leesh3288/f693061e6523c97274ad5298eb2c74e9

pierluigilenoci avatar Sep 11 '23 08:09 pierluigilenoci

Maybe rewrite Symbol in sandbox: https://github.com/zhennann/egg-born-utils/blob/master/lib/common/tools.js

const sandbox = Object.assign({}, scope, {
      Symbol: {
        toStringTag: Symbol.toStringTag,
        for: key => {
          if (['nodejs.util.inspect.custom', 'Symbol.species'].includes(key)) return null;
          return Symbol.for(key);
        },
      },
      process: null,
    });
    const vm = new NodeVM({
      console: 'inherit',
      sandbox,
      require: false,
      nesting: false,
      eval: false,
      wasm: false,
      wrapper,
    });

zhennann avatar Sep 16 '23 02:09 zhennann

Proxying Error and Symbol is what I'll try to fix for now. This works in our use case but it does destroy the native error references which could cause issues for other implementations. We've tested isolated-vm, it also has minor issues and is way slower in comparison, not too sure why, so for us and for now continuing with vm2's approach is the best way forward although it may not be the best way for everyone or forever.

If this approach can not work @leesh3288 please point me to why, seems like most issues is a leaky Error object (and I'm sure there are more Symbol related hacks we'll need to test and cater for).

weareu avatar Sep 22 '23 06:09 weareu

Let me explain exactly what I did.

Proxy -> LocalError type. This does allow to refactor all LocalError/Error define property overrides and simplify in future but for now I kept the change limited and clean without major refactoring, this pulls error through safe handling. Like I said, destroys native error, so this may be an issue, but this should future proof against other error issues. There may be more error props that need to be supported. Proxy -> Symbol type. This intercepts inspect and species related symbol calls.

Added two new unit tests 'promise attack' and 'inspect attack'.

@leesh3288 any ideas as to why this approach will not work and what I've missed.

https://github.com/weareu/vm2/commit/017f73b98fed23ee2e01cb375b7a660087eca886#diff-f7e32c78b248c2f91cb64d680f7ae71878ca3038f33daf52ff24c8a26a09848a

weareu avatar Sep 26 '23 08:09 weareu

@weareu Did you get any feedback on this? Are you planning to package your fix?

dannyhaak avatar Oct 05 '23 13:10 dannyhaak

@dannyhaak None of the forks which tried to fix these issues did it correctly. All of them are still vulnerable to some sort of breakout. If it would be that easy we whouldn't have discontinued this library.

XmiliaH avatar Oct 05 '23 13:10 XmiliaH