Merge pull request #25 from bcoe/exploit-fix

Fixed two major exploits pointed out in #21

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;
+exports.Sandbox = Sandbox;

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,15 +110,15 @@ SandCastle.prototype.startHeartbeat = function() {
 };
 
 SandCastle.prototype.runHeartbeatScript = function() {
-  
+
   // Only wait for one heartbeat script
   // to execute at a time.
   if (this.waitingOnHeartbeat) return;
   this.waitingOnHeartbeat = true;
 
   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;
+exports.SandCastle = SandCastle;

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"

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!'}});
+  });
+});

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);
+}

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');
+}

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();
     }
   });
-
-});
+});

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();
   });
-
 });