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

doc: add note that vm module is not a security mechanism #11557

Closed
wants to merge 4 commits into from

Conversation

krydos
Copy link
Contributor

@krydos krydos commented Feb 25, 2017

the text added in this commit should warn users about
wrong idea that vm module can be secure to run unsafe scripts
in sandboxes

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc, vm

Hi,
As it mentioned in issue #11550 the VM module can be mistakenly used as a security mechanism and it should be great to have a note that VM module it is not about security.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem. labels Feb 25, 2017
@addaleax
Copy link
Member

I think it would be really helpful to explain why this is hard to get right. (Afaict, it is not strictly impossible to use a VM context for actual sandboxing, just something that can go wrong easily. But then again I haven’t tried to actually do that for real.)

doc/api/vm.md Outdated
@@ -14,6 +14,8 @@ const vm = require('vm');
JavaScript code can be compiled and run immediately or compiled, saved, and run
later.

*Note*: The vm module makes a wrong impression as a security mechanism. In fact, it is not at all, you shouldn't use it to run untrusted scripts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please line wrap at 80 chars.
Also, please avoid the use of informal pronouns like you.

Copy link
Member

@Trott Trott Feb 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something more like this?:

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

@krydos
Copy link
Contributor Author

krydos commented Feb 26, 2017

@jasnell thank you for the review, required changes were made.

@Trott completely agree. In you case sentence looks more like tech doc.

@addaleax I think it is good idea to add info about why it is not security mechanism. Since it can take more space I think it should be somewhere at the bottom of the page. Maybe with link from top of the page. Also I'm not really sure how to explain why it is not security mechanism, since sandboxes (contexts) really makes impression of safety and secure. Please share you thoughts.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 26, 2017

Note that the issue is a bit deeper here, e.g. https://www.npmjs.com/package/vm2 is built on top of the vm module and positions itself as a secure sandbox, which it is not.

Perhaps it's worth to mention that anything that is built on vm and runs untrusted code in the same process is not secure, but I am not exactly sure how that should be worded.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 26, 2017

@addaleax

Afaict, it is not strictly impossible to use a VM context for actual sandboxing, just something that can go wrong easily

I'm pretty sure that it's impossible to use vm module alone (without kvm or at least a separate process) for sandboxing without parsing and verifying the code in question.

Btw, see #3020 — that perhaps needs a fix on the documentation side.

doc/api/vm.md Outdated
@@ -14,6 +14,9 @@ const vm = require('vm');
JavaScript code can be compiled and run immediately or compiled, saved, and run
later.

*Note*: The vm module is not a security mechanism.
Do not use it to run untrusted scripts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/scripts/code/

@krydos
Copy link
Contributor Author

krydos commented Feb 27, 2017

@seishun sounds much better. Thank you. I pushed new changes.

@vkurchatkin
Copy link
Contributor

I'm pretty sure that it's impossible to use vm module alone (without kvm or at least a separate process) for sandboxing without parsing and verifying the code in question

Well, browsers do just that, so it should be possible

@ChALkeR
Copy link
Member

ChALkeR commented Feb 27, 2017

Well, browsers do just that, so it should be possible

No, it's not — the usecases are very different here. In the browser, you trigger a website open yourself, and if that crashes a tab or several ones (which is quite possible, btw.) — it's not a big deal actually. On a server — anyone could send you a malicious payload (without any additional action required) if you accept untrusted code and run it in vm. That can at least crash the process (e.g. by consuming all resources) and if that process is not a separate one — cause a DoS for the webserver.

Also (with lower significance, but this one of reasons why I would recommend not just a separate process, but a decent isolation): in the browser, the API surface that could be used by all the sites is pretty much limited, in Node.js — the main process has unrestricted access to the network/filesystem/etc. from the js code, so vm sandboxing becomes the only isolation layer between the untrusted code and a full system compromise. You shouldn't trust a single isolation layer when you can have two.

Also note that there are additional ways to shoot yourself in the foot here, like passing Buffer inside the vm context (that will allow untrusted code to read your data). I have seen people done that (in fact take a look at the vm2 npm module).

@vkurchatkin
Copy link
Contributor

No, it's not — the usecases are very different here.

You won't be happy if an iframe chrashes the whole tab, so it's very similar

You shouldn't trust a single isolation layer when you can have two.

In that case you probably shouldn't trust two if you can have three. On the other hand, why use two if one does the job?

@ChALkeR
Copy link
Member

ChALkeR commented Feb 27, 2017

You won't be happy if an iframe chrashes the whole tab, so it's very similar

That's quite possible today (try const x = []; for (;;) x.push(new ArrayBuffer(1e3)) for example), and the impact of a crashed tab that required a certain action from the user it affects (e.g. open a misbehaving page) is very different from crashing a webserver on user input without any action required from the server.

doc/api/vm.md Outdated
@@ -14,6 +14,9 @@ const vm = require('vm');
JavaScript code can be compiled and run immediately or compiled, saved, and run
later.

*Note*: The vm module is not a security mechanism.
Do not use it to run untrusted code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe bold this last line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123 Sounds great. I will update pull request pretty soon. Thank you!

@bnoordhuis
Copy link
Member

Well, browsers do just that, so it should be possible

Chrome goes to considerable lengths to run iframes in separate sandboxed processes. I'd say that underlines @ChALkeR's point of defense in depth.

It's possible to isolate contexts by giving them their own security token (something node doesn't currently do; in fact, it does the opposite) but that only protects you against prototype traversal, it's no good against a CPU or memory consumption denial-of-service.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but s/machanism/mechanism/ in the commit log.

@krydos krydos changed the title doc: add note that vm module is not a security machanism doc: add note that vm module is not a security mechanism Mar 5, 2017
the text added in this commit should warn users about
wrong idea that vm module can be secure to run unsafe scripts
in sandboxes
@krydos krydos force-pushed the issue-11550/vm-doc-update branch from bd274f7 to f86040c Compare March 5, 2017 12:59
@krydos
Copy link
Contributor Author

krydos commented Mar 5, 2017

I changed the typo in mechanism word of my first commit message and force pushed everything again. I hope it is ok, please correct me if I did something wrong.

jasnell pushed a commit that referenced this pull request Mar 17, 2017
the text added in this commit should warn users about
wrong idea that vm module can be secure to run unsafe scripts
in sandboxes

PR-URL: #11557
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell
Copy link
Member

jasnell commented Mar 17, 2017

Landed in 2e74b0d.

Thank you @krydos! I squashed the commits on landing.

@jasnell jasnell closed this Mar 17, 2017
@krydos krydos deleted the issue-11550/vm-doc-update branch March 19, 2017 15:23
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
the text added in this commit should warn users about
wrong idea that vm module can be secure to run unsafe scripts
in sandboxes

PR-URL: nodejs#11557
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
the text added in this commit should warn users about
wrong idea that vm module can be secure to run unsafe scripts
in sandboxes

PR-URL: nodejs#11557
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
the text added in this commit should warn users about
wrong idea that vm module can be secure to run unsafe scripts
in sandboxes

PR-URL: #11557
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
the text added in this commit should warn users about
wrong idea that vm module can be secure to run unsafe scripts
in sandboxes

PR-URL: #11557
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
the text added in this commit should warn users about
wrong idea that vm module can be secure to run unsafe scripts
in sandboxes

PR-URL: nodejs/node#11557
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet