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

Node.js sandbox is broken #33

Open
Ginden opened this issue Jun 13, 2016 · 14 comments · May be fixed by #37
Open

Node.js sandbox is broken #33

Ginden opened this issue Jun 13, 2016 · 14 comments · May be fixed by #37

Comments

@Ginden
Copy link

@Ginden Ginden commented Jun 13, 2016

start.js file:

var jailed = require('../../../lib/jailed.js');
var api = {};
var plugin = new jailed.Plugin(__dirname + '/plugin.js', api);

plugin.js file:

var require = application.whenConnected.constructor('return process.mainModule.require')();
// do anything with true "require" here
@asvd

This comment has been minimized.

Copy link
Owner

@asvd asvd commented Jun 13, 2016

wow, thanks. will investigate

@asvd

This comment has been minimized.

Copy link
Owner

@asvd asvd commented Jun 13, 2016

What can one do here with the require() by the way? The point of the sandbox is to protect the main application.

@Ginden

This comment has been minimized.

Copy link
Author

@Ginden Ginden commented Jun 13, 2016

You can require('fs') or require('child_process') and do anything with full user permission (including dumping memory of application).
You can require('http') and overwrite prototypes to track requests.
Or anything else.

@gpascualg gpascualg linked a pull request that will close this issue Jul 3, 2016
@gpascualg

This comment has been minimized.

Copy link

@gpascualg gpascualg commented Jul 3, 2016

@Ginden Could you test #37 and see if any other method of breaking it exist?
@asvd You could also do constructor('return global') and would have much more than require alone. Let me know if I can do something else on the PR ;)

@asvd

This comment has been minimized.

Copy link
Owner

@asvd asvd commented Jul 3, 2016

Actually I was thinking about running a subprocess in a chrooted environment, and use an OS-level communication channel to avoid shared objects between parent and child processes :-)

@gpascualg

This comment has been minimized.

Copy link

@gpascualg gpascualg commented Jul 3, 2016

If that subprocess was NodeJS, you would still have access to require, and that basically means to the complete system. The application itself would be safe, of couse, but it would still leave the system open I believe.

Btw, the proposed solution might have some perfomance impact, as it is creating Proxies at each call, Maybe keeping them in a dictionary or something alike would be best (I don't have the time now to do it, maybe in a few days).

@asvd

This comment has been minimized.

Copy link
Owner

@asvd asvd commented Jul 3, 2016

On my opinion the performance impact is secondary as long as the sandbox is protected.

On security, I am only in doubt about getPrototypeOf() method which returns the prototype of an original object. Will need to check this.

@gpascualg

This comment has been minimized.

Copy link

@gpascualg gpascualg commented Jul 3, 2016

Yes, I indeed haven't had time to test it. I barely tested the constructor based exploit. Some more extensive tests should be done. I might be able to do them in 1-2 weeks.

@Ginden

This comment has been minimized.

Copy link
Author

@Ginden Ginden commented Jul 4, 2016

Maybe you should reuse/fork Google Caja for this?

@asvd

This comment has been minimized.

Copy link
Owner

@asvd asvd commented Jul 4, 2016

Caja is a separate project which works very differently (parses and evaluates code by itself). Users may choose it instead of Jailed of course.

@tommitytom

This comment has been minimized.

Copy link

@tommitytom tommitytom commented Sep 23, 2016

Any potential solution to this issue been discovered?

@lu4

This comment has been minimized.

Copy link

@lu4 lu4 commented Oct 30, 2016

There's this library:
https://www.npmjs.com/package/vm2

They seem to resolve this issue through usage of proxies

@gpascualg

This comment has been minimized.

Copy link

@gpascualg gpascualg commented Oct 30, 2016

@lu4 that's basically what I did here: #37
It simply needs some exhaustive testing, which I sadly had no time to do (I checked the basic cases, ie. constructor, and it seemed to work).

@zsf3

This comment has been minimized.

Copy link

@zsf3 zsf3 commented Dec 26, 2019

is there any update on this issue?

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

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.