sandbox can be broken out of

[yorickvP]

Using JavaScriptStackTraceApi, it is possible to break out of the sandbox and get full access to the node.js api. Removing Error.captureStackTrace alone does not fix this issue, and I haven't found a pure javascript fix, yet. (freezing Error.prepareStackTrace, maybe?). I have a POC, but am hesitant to release it.

[gf3]

Ugh. Thanks for bringing this to my attention, @yorickvP. I don't have much time to investigate right now, but if you happen to come across a solution please let me know.

[yorickvP]

In oftn-bot, we ported the sandbox to C++ so that, when it's broken out of, there is no API to go anywhere. https://github.com/oftn/oftn-bot/commit/892a34dda5dfd77b499b2c913801c2b599b31342

[gf3]

@yorickvP That makes sense, and is probably the safest route.

[asvarga]

Should I not use this tool to run untrusted js on the server then? Are you working on a fix?

[liebig]

Does this problem still exist?

[gf3]

Yes—working on a fix right now though. Should be out next week sometime (hopefully).

[dashed]

Noting here that from my findings in https://github.com/bcoe/sandcastle/issues/31#issuecomment-45405647, the Error.prepareStackTrace seems to be fixed as of node v0.11.13 with v8 3.25.30. But not node v0.10.28 with v8 3.14.5.9.

[dashed]

@yorickvP out of curiosity, would doing the following still not fix this issue?

delete Error.prepareStackTrace;
delete Error.captureStackTrace;

[dashed]

Hmm. Apparently deleting those functions still doesn't fix the exploit in node v0.10.28.

[dashed]

One could freeze the Error object since an exploit relies on being able to add properties to it:

"use strict";
delete Error.prepareStackTrace;
delete Error.captureStackTrace;
Object.freeze(Error);

@yorickvP thoughts?

[yorickvP]

Freezing the Error object should work, until someone manages to acquire another Error object (for example using an exposed 'vm' module or iframe), so the sandbox user would have to be a bit careful. The exploit is probably fixed in v0.11 (if they're enforcing no .getThis on callers of strict functions and every part of the user code that calls the sandbox code is strict). I agree with @ndob that porting to C++ would be the best long-term solution.

[rosslazer]

All,

Maybe this is a good time to set up a disclosure policy for this project. I think that you guys should set up a PGP key and have a designated email for vulnerabilities. There might be people using this in production that are being hit with this 0 day because of the disclosure. Just a thought.

[GeorgeBailey]

Does this problem exist for Node 0.12 users?

[milesgillham]

I think the problem exists intrinsically as it stands. You would need to be very careful how you use it in production regardless.

On 2 March 2015 at 13:10, George Bailey notifications@github.com wrote:

Does this problem still exist?

— Reply to this email directly or view it on GitHub https://github.com/gf3/sandbox/issues/29#issuecomment-76652787.

[0o-de-lally]

@gf3 I know you are very busy. Just looking for an update on this. Is this still an issue?

[gf3]

@keyscores this is still an issue in the current version. as of last year the version of V8 packaged with node was not current enough to implement a secure sandbox, however this may have changed recently. hopefully i'll have some time to investigate in the near future

[0o-de-lally]

@gf3 thanks for maintaining this package. Out of curiosity have you tested this bug happens with vm.runInNewContext as well?

[yorickvP]

AFAIK, the stack traces can't get over a "use strict" function, so that might be a good patch.

[yorickvP]

Looking at the code now, it can still be broken out of because the errors can escape the saferunner and then throw upon inspection (v8 source code solves this by wrapping the inspection in another try-catch block). Best would be to just add a global strict mode, and maybe serialize the errors over JSON too. Note that objects can define a toJSON method for custom JSON serialization just to ruin your day, but I don't think that's a security risk.

[0o-de-lally]

@yorickvP Can you possibly write one/some tests for this? I'm willing to work a bit on this if I have a clear test case in the code.

[yorickvP]

@keyscores https://gist.github.com/yorickvP/f3b5cf3f97f65f05c9ad5b94a6716098 I think this catches all of them based on this exploit. The fix is probably some sprinkling with 'use strict' and a backup error handler that doesn't inspect the error object.

[0o-de-lally]

Thanks @yorickvP that's very helpful. I'll work on conforming it to the mocha tests for this repo. Unless you have already?