gf3 / sandbox

A nifty JavaScript sandbox for Node.js
ISC License
844 stars 123 forks source link

Leak + "use strict" #9

Closed bmeck closed 12 years ago

bmeck commented 12 years ago

The following shows a leak:

Sandbox=require('sandbox');
s=new Sandbox();
function dump(c){
  console.error(c)
};

s.run('('+f.toString()+')()',dump)
function f(){
  var p=f.caller.constructor("return process")();
//  p.stdout.write("{\"result\":"+JSON.stringify(p.env.USER)+"}");
  console.log(Object.keys(p));
//  p.stdout.write=function(){};
  return 1;
}

The calling function is leaking info, the way around this is to wrap any user script in a function that is created inside of the sandbox and only allow communication through closures. IE:

x=1

becomes

(function () {
var global = this;
// Keep it outside of strict mode
function UserScript(str) {
  return eval(str)
}
// place with a closure that is not exposed thanks to strict mode
return function(comm, src){
  // stop argument / caller attacks
  "use strict";
  var send = function (event) {
     comm.send(event, JSON.stringify([].slice.call(arguments,1)));
  }
  global.print = send.bind(global, 'stdout')
  global.console = {};
  global.console.log =send.bind(global, 'stdout')
  send('stdout',UserScript(src))
}
})()

Once you get the safe function you would then invoke it using f(comm,'x=1').

It is not pretty but prevents leaks.

bmeck commented 12 years ago

as a side note:

a test should also see if the global is leaking via (function(){return this})().process to ensure you are calling the safe function in the proper context.

rehanift commented 12 years ago

I believe you can also exploit a Node vm.runInNewContext hack to prevent this issue. AFAIK, all the built-in ECMAScript objects are cloned from the parent context into the sandbox context, so you can do Function.prototype.toString = function(){}; before calling vm.runInNewContext.

While this behavior seems to be implementation specific at the moment, the general idea of redefining Function#toString should still suffice.

bmeck commented 12 years ago

rehanift no, the Function.toString was just to simplify how the code looks rather than making a large string.

bmeck commented 12 years ago

see #10