So called "secure" sandbox isn't secure at all

[Ginden]

compileCode('return function(){}.constructor("alert(this)")()')({})

[kolodny]

FWIW you can do something like Function.prototype.constructor = null but then you would really need to really make Function.prototype.constructor a getter and check if it's being called from the sandbox which I'm not sure is even possible. It's definitely a rabbit hole I wouldn't want to be responsible for though.

[Ginden]

You can potentially detect synchronous calls from sandbox, but you can't detect asynchronous calls - setTimeout, Promise#then, setInterval, requestAnimationFrame, fetch and thousands other ways to escape sandbox remain.

[solkimicreb]

Hi,

this is already fixed and tested in the next unreleased version, but I am waiting for more feedback before releasing it.

My solution to this:
I made a compiler.secure() function that iterates every literal type ('', 0, true, {}, [], () => {}) and freezes their prototype with Object.freeze(). This is optional as it provides more security but requires more attention for polyfilling these objects for example.

About async functions:
They just work. Async functions are still defined inside the with block. Meaning they will have a closure inside the with. Try and reach anything global from an async function, it won't work. This is also tested in the next version. (Side note: all of the methods you mention above are actually global functions which are inaccessible from the sandbox)

Final note:
This is part of an alpha project. I am sure there are more security holes, and I plan to fix them eventually. If you find anything else or you think that I am missing something important please share (:

Thx for the feedback!

[bd0]

This is also susceptible to simple string manipulation. For example, this will run outside the sandbox:

var func = nx.compiler.compileCode('} alert(window.location);{');
func({})

because it closes the with statement.

[kolodny]

@bd0

var allow = function(code) {
  try {
    eval('(function() {' + code + '})');
    return true;
  } catch (e) {
    return false;
  }
}

[bd0]

@kolodny That would suffer from the same problem, would it not?
allow('}); alert(window.location);({')
Shows the alert and evaluates to true.

[solkimicreb]

@bd0
The string manipulation seems like a big problem. I am thinking about moving the with statement outside of new Function somehow. Something like with (sandbox) { eval(src) }, but this also has several problems.

[solkimicreb]

@kolodny
I am not sure I understand that.

[solkimicreb]

@bd0

Another solution would be checking the string before passing it to new Function(), like:

let depth = 0
for (let char of src) {
  if (char === '{') depth++
  else if (char === '}') {
    if (depth === 0) throw new SyntaxError()
    else depth--
  }
}

[bd0]

@solkimicreb A depth check might help, but it's not foolproof. For example:
compileCode('/*{*/ } alert(window.location);{ /*}*/');
I don't know that there's a perfect solution. You could look into evaluating the code inside a sandboxed iframe, but that seems like a very heavy solution and is not without it's own problems.
http://www.html5rocks.com/en/tutorials/security/sandboxed-iframes/

[solkimicreb]

Yeah, thanks for the example. It seems like the best option would be to leave only the src as the argument: new Function(src), and move the rest (with) outside to the hard coded part.

Another thing I thought of is only allowing single expressions like: 'with (sandbox) { return' + src + ' }'. This way early closing would just return, but it would really limit the library.

[solkimicreb]

I will try to move the with outside of the string and post the new code here.

[IKoshelev]

Btw, for an src like console.log(this); , which would normally have this === global object, it will help to prepend src with " 'use strict;' " UPD. According to MDN, it can't be used with with.

P.S. I mentioned it briefly in the post, but just to not forget, var a = /1/; is a RegExp literal and will give access to its constructor and prototype.

[solkimicreb]

I figured out to use return eval('with (sandbox) { (function () { ${src} }) }').bind(sandbox, sandbox) instead of new Function ('with (sandbox) { ${src} }').bind(sandbox, sandbox).

This solves two issues: firstly the string manipulation mentioned by @bd0, secondly: the ability to use strict mode inside passed code mentioned by @IKoshelev. What are your opinions, do you think I am missing something?

Thanks for all the comments so far, I will be away for the weekend but get back to this topic on Monday (:

P.S. Thanks for the warning about the regexp literal. I will try to look for a solution that doesn't modify (freeze) global stuff if I can in the long term.

[bd0]

@solkimicreb At first glance it seems like it would suffer from the same problem, but I'd have to see it in context with the rest of the library. Something like:
compileCode('});} alert(window.location); {({')
and you'll end up with
eval('with (sandbox) { (function () { });} alert(window.location); {({ }) }').bind(sandbox, sandbox)

[kolodny]

You may be able to do something like

new Function('src', `return function(sandbox) { with (sandbox) { return eval(src) }}`)(src);

But you may want to make eval a local function that nulls itself after it's first invocation, since there's no way to hide that in the executing scope

[bd0]

@solkimicreb I've found something that seems to work, and is similar to @kolodny's suggestion. It's not perfect. The src has to be evaluated every time and I'm sure there are other issues I haven't thought of, but you don't have to worry about string manipulation.

function has(target, key) {
    // Only let eval execute once!
    if (key === 'eval' && 0 === this.evaled++) {
        return false;
    }
    return true;
}

function get(target, key, receiver) {
    if (key === Symbol.unscopables) {
        return undefined;
    }
    return Reflect.get(target, key, receiver)
}

function compileCode(src) {

    // Return a function that wraps the given data 
    // in a proxy and executes the src.
    return (function (dataObj) {
        var dataProxy = new Proxy(dataObj, {
            has,
            get,
            // Track # of times eval() has been called.
            evaled: 0
        });

        // Set the src property on the proxy so it 
        // is accessible inside the with scope.
        dataProxy.src = src;

        // Evaluate the src inside the scope.
        with(dataProxy) {
            return eval(src);
        }
    });
}

var data = {
    a: 1,
    b: 2
};

var code = compileCode('"a is " + a + ", location is " + location;');

// Should print "a is 1, location is undefined"
console.log(code(data));

// Test for eval escalation.  Can't allow eval to execute
// because it will gain access to the global scope.
var code2 = compileCode('eval.call(this, "window.location")');

try {
    // Should throw an exception.
    code2({});
}
catch(te) {
    console.log(te);
}

[solkimicreb]

Hi!

This is a proposal for nx-compile 2.0. I used all your amazing feedback to fix and test things.

The biggest changes:

• security is now optional: it starts as unsecure and can be switched to secure mode by nx.secure()
• fixed and tested every security issue raised here and on different forums so far

Thanks @bd0 , @kolodny ,@IKoshelev !

(P.S.: @bd0 I played around with your solution a lot. I also think moving the with outside to the hard burnt code would be the best, but then eval or Function would need to be exposed on the sandbox. As @kolodny mentioned these can be deleted later. A bigger issue with using eval is that it exposes local variables (utility vars from the nx-compile module). Using new Function() inside a with block is also not good, since new Function creates a function in global scope and escapes the with block. In the end I decided to keep the with block inside the evaluated string and solve the issue by a hacky return.)

[bd0]

@solkimicreb Yeah, eval is weird. Another quirk I found was that assigning eval to another variable, e.g. var evalIsEvil = eval; evalIsEvil(src); causes it to execute in the global scope rather than local scope, which is what necessitated having it called directly. I'm glad you found a solution though.

[JoshuaWise]

You don't need to use eval to protect against string manipulation. You can just check if the given code is valid by itself, before compiling it into your special function:

// Tests for syntax errors without running the code
new Function('"use strict"; ' + src);

// If the above operation didn't throw a syntax error, the operation below is safe
var code = new Function('sandbox', 'with (sandbox) {return (function () {"use strict"; ' + src + '}).call(this)}');

Using this method has three advantages:

• It catches syntax errors when the code is compiled, instead of when it is executed
• The source code is only evaluated once, providing much much better performance
• You can use "use strict"; at the top of your module.

[solkimicreb]

Yeah, it seems like a nice solution, I will think about using it thanks (:
I don't use eval, I use new Function in both v1 and v2 and I 'use strict' in v2.
My only argument against your above code is that it compiles the code twice which performs a bit worse. (It is also suspicious to me, I know it only compiles the code, but is there surely no way to reach and modify something inside it?)

[JoshuaWise]

Yes, in cases where each compiled code is only executed once, its performance will suffer. It's certainly more optimized for code that will be executed many times.

As for your second concern, I've only ever seen two forms of string manipulation attacks:

• Closing the previous code blocks, to execute unsandboxed code
  • Example: }).call(this); nonStrictModeCode(); (function () {. These types of attacks are not valid JavaScript on their own. They rely on the code that comes before and after to pass syntax checks. Therefore, if we test them in isolation, we can stop them in their tracks
• Using escape sequences to bypass blacklist checks.
  • For example, if you scan the source code for the word eval to prevent people from using eval in their compiled code, people can easily bypass this by doing things like this: e\u0076al("this")

Luckily, the second type of string manipulation is a non-issue for us, since we're not using string matching to check for dangerous code.

[solkimicreb]

@JoshuaWise After all the feedback and tests I think your pre-compiling solution is the way to go. Thanks for the idea!

[solkimicreb]

@JoshuaWise @kolodny @bd0 @IKoshelev I would like to add you to the contributors list. Let me know if you would like to be not included in it.

Thx for all the feedback and ideas so far!

[solkimicreb]

I am going to close this now as all the issues mentioned here are fixed in the merged v2.0.0 of nx-compile.