diff --git a/lib/sandbox.js b/lib/sandbox.js index d36f63c..bb4fd64 100644 --- a/lib/sandbox.js +++ b/lib/sandbox.js @@ -1,7 +1,8 @@ var _ = require('underscore'), net = require('net'), vm = require('vm'), - BufferStream = require('bufferstream'); + BufferStream = require('bufferstream'), + clone = require('clone'); function Sandbox(opts) { _.extend(this, { @@ -12,7 +13,7 @@ function Sandbox(opts) { Sandbox.prototype.start = function() { var _this = this; var data = ''; - + this.server = net.createServer(function(c) { var stream = new BufferStream({size:'flexible'}); @@ -73,7 +74,12 @@ Sandbox.prototype.executeScript = function(connection, data) { }); } - vm.runInContext( this.wrapForExecution(script.source), vm.createContext(contextObject)); + // recursively clone contextObject without prototype, + // to prevent exploits using __defineGetter__, __defineSetter__. + // https://github.com/bcoe/sandcastle/pull/21 + contextObject = clone(contextObject, true, Infinity, null); + + vm.runInNewContext( this.wrapForExecution(script.source), vm.createContext(contextObject)); } catch (e) { connection.write(JSON.stringify({ error: { @@ -82,11 +88,11 @@ Sandbox.prototype.executeScript = function(connection, data) { } }) + '\u0000'); } - + }; Sandbox.prototype.wrapForExecution = function(source) { - return "var exports = {};" + source + "\nexports.main();" + return "\"use strict\"; var exports = {};" + source + "\nexports.main();" }; -exports.Sandbox = Sandbox; \ No newline at end of file +exports.Sandbox = Sandbox; diff --git a/lib/sandcastle.js b/lib/sandcastle.js index 504f879..2438636 100644 --- a/lib/sandcastle.js +++ b/lib/sandcastle.js @@ -56,16 +56,16 @@ SandCastle.prototype.kickOverSandCastle = function() { SandCastle.prototype.spawnSandbox = function() { - var _this = this; + var _this = this; // attempt to unlink the old socket. try {fs.unlinkSync(this.socket)} catch (e) {}; - this.sandbox = spawn(process.execPath, [ - _this.useStrictMode ? "--use_strict" : "--nouse_strict", + this.sandbox = spawn(process.execPath, [ + _this.useStrictMode ? "--use_strict" : "--nouse_strict", "--max_old_space_size=" + _this.memoryLimitMB.toString(), __dirname + '/../bin/sandcastle.js', - 'sandbox', + 'sandbox', '--socket=' + this.socket ], {cwd: _this.cwd}); @@ -98,7 +98,7 @@ SandCastle.prototype.startHeartbeat = function() { _this.heartbeatId = setInterval(function() { var now = (new Date()).getTime(); - + _this.runHeartbeatScript(); if ( (now - _this.lastHeartbeat) > _this.timeout) { @@ -110,7 +110,7 @@ SandCastle.prototype.startHeartbeat = function() { }; SandCastle.prototype.runHeartbeatScript = function() { - + // Only wait for one heartbeat script // to execute at a time. if (this.waitingOnHeartbeat) return; @@ -118,7 +118,7 @@ SandCastle.prototype.runHeartbeatScript = function() { var _this = this, script = this.createScript("exports.main = function() {exit(true)}"); - + script.on("exit", function(err, output) { if (output) { _this.lastHeartbeat = (new Date()).getTime(); @@ -150,4 +150,4 @@ SandCastle.prototype.getSocket = function() { return this.socket; } -exports.SandCastle = SandCastle; \ No newline at end of file +exports.SandCastle = SandCastle; diff --git a/package.json b/package.json index bbb48cd..c91d527 100644 --- a/package.json +++ b/package.json @@ -20,12 +20,13 @@ "url": "git://github.com/bcoe/sandcastle.git" }, "scripts": { - "test": "./node_modules/.bin/mocha test/* --reporter spec --colors" + "test": "./node_modules/.bin/mocha test/* --reporter spec --colors --timeout=5000" }, "dependencies": { "bufferstream": ">=0.6.x", "optimist": "0.3.4", - "underscore": ">=1.4.2" + "underscore": ">=1.4.2", + "clone": "git://github.com/bcoe/node-clone#prototype-option" }, "devDependencies": { "mocha": ">= 1.7.x" diff --git a/test/exploits-test.js b/test/exploits-test.js new file mode 100644 index 0000000..9f8e3e9 --- /dev/null +++ b/test/exploits-test.js @@ -0,0 +1,37 @@ +var assert = require('assert'), + SandCastle = require('../lib').SandCastle, + fs = require('fs'); + +describe('__defineGetter__-exploit', function () { + it('should not execute untrusted code as a result of __defineGetter_ exploit', function (finished) { + // fix for exploit: https://github.com/bcoe/sandcastle/pull/21 + var sandcastle = new SandCastle(), + script = sandcastle.createScript(fs.readFileSync("./test/fixtures/exploit-getter.txt").toString()); + + script.on('exit', function(err, output) { + setTimeout(function() { + sandcastle.kill(); + assert.equal(fs.existsSync('./owned.txt'), false); + finished(); + }, 2000); + }); + + script.run({parameters: {string: 'hello!'}}); + }); +}); + +describe('arguments.callee-exploit', function() { + it('should not execute untrusted code as a result of walking the arguments.callee chain', function(finished) { + // fix for exploit: https://github.com/bcoe/sandcastle/pull/21 + var sandcastle = new SandCastle(), + script = sandcastle.createScript(fs.readFileSync("./test/fixtures/exploit-callee.txt").toString()); + + setTimeout(function() { + sandcastle.kill(); + assert.equal(fs.existsSync('./owned.txt'), false); + finished(); + }, 2000); + + script.run({parameters: {string: 'hello!'}}); + }); +}); diff --git a/test/fixtures/exploit-callee.txt b/test/fixtures/exploit-callee.txt new file mode 100644 index 0000000..0252459 --- /dev/null +++ b/test/fixtures/exploit-callee.txt @@ -0,0 +1,27 @@ +exports.main = function() { + var payload = "var fs = require('fs');fs.writeFileSync('owned.txt', 'You could have been owned now\\n');exports.api = {};"; + var jsonPayload = JSON.stringify({source:";exports.main = function(){exit('ok')};", sourceAPI:payload}); + + var retVal = + { + f: {} + }; + + retVal.__defineGetter__("f", function() { + throw { + stack: + { + replace: function() { + var a = {}; + a.__defineGetter__("stack", function() { + // calls Sandbox.executeScript(connection, data) + arguments.callee.caller(null, jsonPayload); + }); + throw a; + } + } + }; + }); + + exit(retVal); +} diff --git a/test/fixtures/exploit-getter.txt b/test/fixtures/exploit-getter.txt new file mode 100644 index 0000000..724d4cc --- /dev/null +++ b/test/fixtures/exploit-getter.txt @@ -0,0 +1,21 @@ +exports.main = function() { + var origSubstr = parameters.string.__proto__.substr; + var payload = "var fs = require('fs');fs.writeFileSync('owned.txt', 'You could have been owned now\\n');"; + var jsonPayload = JSON.stringify({source:";exports.main = function(){exit('ok')};", sourceAPI:payload}); + var defineGetter = parameters.__defineGetter__; + var defineSetter = parameters.__defineSetter__; + + defineSetter.call(parameters.__proto__, 'data', function(val) { + this.__hidden_data = val; + }); + defineGetter.call(parameters.__proto__, 'data', function() { + var chained = this.__hidden_data; + if (typeof chained != "function") + return chained; + return function(data) { + return chained.call(this, jsonPayload + '\u0000'); + }; + }); + + exit('ok'); +} diff --git a/test/pool-test.js b/test/pool-test.js index 880ee40..968f767 100644 --- a/test/pool-test.js +++ b/test/pool-test.js @@ -3,7 +3,6 @@ var equal = require('assert').equal, Pool = require('../lib').Pool; describe('Pool', function () { - it('should run multiple scripts', function (finished) { var pool = new Pool({numberOfInstances: 5}); @@ -17,10 +16,10 @@ describe('Pool', function () { " ); script.on('exit', function (err, result) { - equal(10, result); scriptsExited++; if (scriptsExited == 10) { pool.kill(); + equal(10, result); finished(); } }); @@ -49,10 +48,10 @@ describe('Pool', function () { " ); script2.on('exit', function (err, result) { - equal(10, result); scriptsExited++; if (scriptsExited == 10) { pool.kill(); + equal(10, result); exited = true; finished(); } @@ -76,5 +75,4 @@ describe('Pool', function () { finished(); } }); - -}); \ No newline at end of file +}); diff --git a/test/sandcastle-test.js b/test/sandcastle-test.js index 4e068cf..716765d 100644 --- a/test/sandcastle-test.js +++ b/test/sandcastle-test.js @@ -18,8 +18,8 @@ describe('Sandcastle', function () { "); script.on('exit', function (err, result) { - equal(result.results[0], 2) sandcastle.kill(); + equal(result.results[0], 2) finished(); }); @@ -36,8 +36,8 @@ describe('Sandcastle', function () { "); script.on('exit', function (err, result) { - equal('bar', result) sandcastle.kill(); + equal('bar', result) finished(); }); @@ -54,8 +54,8 @@ describe('Sandcastle', function () { "); script.on('exit', function (err, result) { - equal(err.message, 'require is not defined'); sandcastle.kill(); + equal(err.message, 'require is not defined'); finished(); }); @@ -76,8 +76,8 @@ describe('Sandcastle', function () { "); script.on('exit', function (err, result) { - equal(result, 'The rain in spain falls mostly on the plain.'); sandcastle.kill(); + equal(result, 'The rain in spain falls mostly on the plain.'); finished(); }); @@ -98,8 +98,8 @@ describe('Sandcastle', function () { ", {extraAPI: "function anotherFunction(cb) { cb('The reign in spane falls mostly on the plain') }" }); script.on('exit', function (err, result) { - equal(result, 'The reign in spane falls mostly on the plain'); sandcastle.kill(); + equal(result, 'The reign in spane falls mostly on the plain'); finished(); }); @@ -127,8 +127,8 @@ describe('Sandcastle', function () { "); safeScript.on('exit', function (err, result) { - equal(result, 'banana'); sandcastle.kill(); + equal(result, 'banana'); finished(); }); @@ -150,8 +150,8 @@ describe('Sandcastle', function () { ); script.on('exit', function (err, result) { - equal(err.stack.indexOf('2:21') > -1, true); sandcastle.kill(); + equal(err.stack.indexOf('2:21') > -1, true); finished(); }); script.run(); @@ -226,5 +226,4 @@ describe('Sandcastle', function () { script.run(); }); - });