docs: Clarification around real world risks and use cases of VM module

📗 API Reference Docs Problem

• Version: n/a

• Platform: n/a

• Subsystem: VM

Affected URL(s):

• https://nodejs.org/api/vm.html

Description

Apologies if this isn't the appropriate place for this; I wasn't sure if it fell under help versus docs versus somewhere else.

The official docs for the VM module state the following in the first line of it's description.

The vm module is not a security mechanism. Do not use it to run untrusted code.

The last line of the Example: Running an HTTP server within a VM section says:

This may introduce risks when untrusted code is executed, e.g. altering objects in the context in unwanted ways.

Now, I think it's fairly safe to say it's quite clear that blindly running arbitrary code through the vm module (or anything else) is most definitely not secure or safe. Although the second quote is a bit less definitive and things like the contextCodeGeneration option in runInNewContext are almost a bit of a teaser that might lead people to believe in some cases it is okay, but ultimately I digress :)

This issue is two-fold:

First, I'm curious if it's possible to have additional context (no pun intended) about when the vm module is useful in practice to the average developer. I think it's safe to say a lot of people misunderstand it, and in some cases that can have very bad security consequences.

Second, I'm curious if it's possible to elaborate on situations where using the vm module in restricted/well-defined situations it could be safe to run code under certain conditions. The following is a contrived example and one could argue it's not even an example of untrusted code, but I'm hoping it'll at least illustrate the idea.

Say for example you take an expression and parse it into an AST and validate that it only includes certain nodes. For sake of argument, let's say you only accept identifiers, numeric literals, and operators like <, >, ==, and &&. Now take the following example:

const vm = require('vm');

const code = 'apple == 5 && blueberry > 10';
const ctx = Object.create(null);

ctx.apple = 55;
ctx.blueberry = 33;

const myBool = vm.runInNewContext(code, ctx, { contextCodeGeneration: { strings: false } });

Now, you could certainly argue if you're parsing a string into an AST, validating the nodes, etc. that you aren't really running untrusted code at that point, but bear with me. Is this even a valid use case for the module? Is this even safe assuming you can validate the AST and the context you're passing in? Does this make you want to hit your head against your desk? :P

Ultimately, there are obviously other ways to do this, you're already parsing it out, you could just make your own logic to piece it back together without ever running the actual code itself, but this seems like a simple way to actually achieve evaluation of simple JS expressions, among other potential use cases, etc.

I think the vm module is one of the most misunderstood modules out there, because a lot of developers I've talked with either completely misunderstand what it does, how you'd use it, or why you'd even use it, along with the potential for horrible security consequences when used inappropriately. It'd be awesome if the docs could include some more information on the practical side of things and where the module really shines in real world usage. Telling people when not to use something is certainly value added, but also illustrating real world use cases for when you would want to use it can be equally valuable.

---

• I would like to work on this issue and
  submit a pull request.

[devsnek]

i think the primary use case of vm is tooling. projects like jest, for example, make extensive use of it. however as soon as you start getting into running code that you haven't vetted, you must take 100% of the burden of validation onto yourself. the amount of footguns in the vm module here is immense. For example the option you mentioned, contextCodeGeneration, does not in fact prevent all forms of eval, it just disables the forms of eval that are created by the new context. If you combine this option with a membrane (basically layers of proxies between the inner and outer context) and provide an override for import() and have a timeout and set the microtask queue option correctly and etc etc it does actually provide the ability to run untrusted code, but again that's on the person using it to do very carefully.

maybe something we could put in the documentation there is a link to https://github.com/laverdet/isolated-vm, which does actually provide a way to run untrusted code with the guarantee that it cannot escape.

I think you summed up the footgun aspect of it much better than I ever could :P The first thing the average person thinks when they see the vm module is that it's some type of sandbox for arbitrary code. Seeing as that's precisely what it's not (except for very explicitly and carefully crafted cases), the next question I generally hear is "well, then what's the point?".

I think adding a little context (again, no pun intended) even as simple as the bit about tooling or testing frameworks would be super useful to help people understand/digest what it could be used for. Linking to something like isolated-vm could be extremely worthwhile as well. Instead of just telling people not to do something, telling them, but wait, here's an actual safe way you can do <insert_random_usually_bad_idea_here> I think could go a long way.

I realize my initial post might be a little long-winded, but I think there's a solid opportunity here for clarifying while also pointing people in constructive directions. I'd be more than happy to help write something up, but at the same time, I'll admit I definitely wouldn't put myself in the boat that fully understands all the caveats here, so having someone that does being able to ELI5 to a broader audience is definitely needed.

[mcollina]

Here is an odd idea: maybe we should embed isolated-vm (or a very similar approach) into Node.js itself.

cc @bmeck

[cristianstaicu]

If you combine this option with a membrane (basically layers of proxies between the inner and outer context) and provide an override for import() and have a timeout and set the microtask queue option correctly and etc etc it does actually provide the ability to run untrusted code, but again that's on the person using it to do very carefully.

As discussed in the Hackerone report #1398777, I disagree with this statement. I think building a secure sandbox on top of this module is close to impossible because of its unpredictable/inconsistent behavior. IMHO, the statement "Do not use it to run untrusted code" should be emphasized further "Do not use it to run untrusted code or for building systems that run untrusted code". It should also be colored in red and underlined if possible. 😁

However, I agree that including a pointer to something like isolated-vm or Secure EcmaScript, or even vendoring them in would be a good idea.

[mcollina]

cc @laverdet

[Trott]

@mcollina We weren't sure why this was on the TSC agenda. (Awareness? Decision? Something else?) We pushed it to next week's agenda, but if you want to leave a sentence or two explaining what the ask of the TSC is here, that would be great.

[mcollina]

There are two fundamental questions:

1. Do we want to mark the vm module with a "Do not use it to run untrusted code or for building systems that run untrusted code" statement? There are plenty of modules in the ecosystem that claims this.

2. Should we work to embed isolated-vm? Is that the solution we want?

Note this ties with some of the discussion about primordials.

[laverdet]

Hi I'm the isolated-vm guy. I personally don't think the node team should attempt to implement and maintain secure environments in core node. Additionally, I think blessing isolated-vm via a link in the core documentation could be irresponsible. isolated-vm is a great tool but still needs to be utilized by seasoned experts to avoid numerous foot guns. I'm happy to jump on the TSC call to provide more color, but I'm also getting the sense that there's not actually a strong push here to get this kind of thing embedded.