Securing JavaScript eval function
Asked Answered
S

6

14

We want to give our users the ability to execute self created JavaScript code within our application. For this we need to use eval to evaluate the code. To reduce all security concerns to a minimum (if not zero), our idea is to prevent the usage of any window or document function within the code. So no XMLHttpRequest or anything similar.

This is the code:

function secure_eval(s) {
    var ret;

    (function(){
        var copyXMLHttpRequest = XMLHttpRequest; // save orginal function in copy

        XMLHttpRequest = undefined; // make orignal function unavailable

        (function() {
            var copyXMLHttpRequest; // prevent access to copy

            try {
                ret = eval(s)
            } catch(e) {
                console.log("syntax error or illegal function used");
            }

        }())
        XMLHttpRequest = copyXMLHttpRequest; // restore original function
    }())
    return ret;
}

This works as follows:

secure_eval('new XMLHttpRequest()'); // ==> "illegal function used"

Now I have several questions:

  1. Is this pattern the right way to secure eval?
  2. What functions of window and document are the ones which are considered harmful?
  3. To ship around question 2. I tried to mask all (native) functions of window But I am not able to enumerate them:

This does not list XMLHttpRequest for instance:

for( var x in window) {
    if( window[x] instanceof Function) {
        console.log(x);
    }
}

Is there a way to get a list of all native functions of window and document?

EDIT:

One of my ideas is to perform the eval within a Worker and prevent access to XMLHttpRequest and document.createElement (see my solution above). This would have (to my mind) the following consequences:

  • no access to the original document
  • no access to the original window
  • no chance to communicate with external resources (no ajax, no scripts)

Do you see any drawback or leaks here?

EDIT2:

In the meantime I have found this question which answer solves many of my problems plus a couple of things I did not even think about (i.e. browser dead lock with "while(true){}".

Sigmund answered 21/10, 2014 at 8:35 Comment(9)
Take a look at ADsafe. Note however that users can execute any code they want by just opening up the JavaScript console (Ctrl + Shift + I).Gloxinia
That I cannot prevent the console option is clear. I just want to secure the option that the code I execute with eval does not come from the user but "from anywhere else...". In parallel I do anything to prevent that this even happens!Sigmund
Rather than blacklisting all bad functions, you can create a whitelist of all good functions. And then check whether any other function is bieng invoked or not.Calipash
This checking could be complicated. Think of the following: var u = window; u["al"+"ert"]("hello");Sigmund
Earlier similar question with good answers: #2674195Dubonnet
Also see #544033.Admirable
You could try doing the eval on the server. This thing might help: npmjs.org/package/localeval.Admirable
@torazaburo Not bad, but unfortunately I want to call client-side services from within the scripts.Sigmund
What exactly are you doing? If each user should be allowed to execute code that he created himself, there is absolutely no security issue that needs to be taken care of. Every user has that right out of the box, and you cannot prevent him doing that anyway.Whooper
D
20

Your code does not actually prevent the use of XMLHttpRequest. I can instantiate an XMLHttpRequest object with these methods:

secure_eval("secure_eval = eval"); // Yep, this completely overwrites secure_eval.
secure_eval("XMLHttpRequest()");

Or:

secure_eval("new (window.open().XMLHttpRequest)()")

Or:

secure_eval("new (document.getElementById('frame').contentWindow.XMLHttpRequest)()")

This 3rd method relies on the presence of an iframe in the HTML of the page, which someone could add by manipulating the DOM in their browser. I do such manipulations every now and then with Greasemonkey to remove annoyances or fix broken GUIs.

This took me about 5 minutes to figure out, and I am not by any means a security guru. And these are only the holes I was able to find quickly, there are probably others, that I don't know about. The lesson here is that it is really really really hard to secure code through eval.

Using A Worker

Ok, so using a Worker to run the code is going to take care of the 2nd and 3rd cases above because there's no window accessible in a Worker. And... hmm.. the 1st case can be handled by shadowing secure_eval inside its scope. End of story? If only...

If I put secure_eval inside a web worker and run the following code, I can reacquire XMLHttpRequest:

secure_eval("var old_log = console.log; console.log = function () { foo = XMLHttpRequest; old_log.apply(this, arguments); };");
console.log("blah");
console.log(secure_eval("foo"));

The principle is to override a function that is used outside secure_eval to capture XMLHttpRequest by assigning it to a variable that will be deliberately leaked to the global space of the worker, wait until that function is used by the worker outside secure_eval, and then grab the saved value. The first console.log above simulates the use of the tampered function outside secure_eval and the 2nd console.log shows that the value was captured. I've used console.log because why not? But really any function in the global space could be modified like this.

Actually, why wait until the worker may use some function we tampered with? Here's another, better, quicker way to do access XMLHttpRequest:

secure_eval("setTimeout(function () { console.log(XMLHttpRequest);}, 0);");

Even in a worker (with a pristine console.log), this will output the actual value of XMLHttpRequest to the console. I'll also note that the value of this inside the function passed to setTimeout is the global scope object (i.e. window when not in a worker, or self in a worker), unaffected by any variable shadowing.

What About the Other Question Mentioned in This Question?

What about the solution here? Much much better but there is still a hole when run in Chrome 38:

makeWorkerExecuteSomeCode('event.target.XMLHttpRequest', 
    function (answer) { console.log( answer ); });

This will show:

function XMLHttpRequest() { [native code] }

Again, I'm no security guru or cracker bent on causing trouble. There are probably still more ways I'm not thinking about.

Dives answered 7/11, 2014 at 1:5 Comment(6)
Very good point concerning the hole in Chrome 38, but that is rather easy to fill: Just put a closure around the try/catch in onmessage and redefine event with var event; within the closure.Sigmund
Yeah, but what else have I not found? BTW, I found a way to reinstate console but that does not lead to XMLHttpRequest so I've not mentioned it.Dives
makeWorkerExecuteSomeCode('var c = self.__proto__.__proto__.__lookupGetter__("console").call(self); c.log("FOO");', function (answer) { console.log(answer) }); The original console object is reinstated as c in this fragment. Works on FF but not Chrome.Dives
But this is only possible, because self is whitelisted, isn't it? If I do not need self, I can remove it from the list and another hole is gone.Sigmund
It also works with this and global as far as I can tell from very quick tests. And I've just tested removing global from the whitelist and it bombs on FF and Chrome. (By "it bombs" I mean the worker is not usable at all.)Dives
what do you mean by "bombs"? I think you cannot remove global from whitelist, because it is needed in the masking functions. Better idea is to redefine it also in the try/catch-closure (like event).Sigmund
W
2

I'll try and answer your questions in order here.

Is this pattern the right way to secure eval?

This part is slightly subjective. I don't see any major security drawbacks to this. I tried several ways to access XMLHttpRequest, but i couldn't:

secure_eval('XMLHttpRequest')
secure_eval('window.XMLHttpRequest')
secure_eval('eval("XMLHttpRequest")()')
secure_eval('window.__proto__.XMLHttpRequest') // nope, it's not inherited

However, it will be a lot if you want to blacklist more things.

What functions of window and document are the ones which are considered harmful?

That depends on what you consider "harmful". Is it bad if the DOM is accessible at all? Or what about WebKit desktop notifications, or speech synthesis?

You'll have to decide this based on your specific use case.

To ship around question 2. I tried to mask all (native) functions of window, but I am not able to enumerate them:

That's because most of the methods are non-enumerable. To enumerate, you can use Object.getOwnPropertyNames(window):

var globals = Object.getOwnPropertyNames(window);
for (var i = 0; i < globals.length; i++) {
    if( window[globals[i]] instanceof Function) {
        console.log(globals[i]);
    }
}

One of my ideas is to perform the eval within a Worker and prevent access to XMLHttpRequest and document.createElement (see my solution above).

This sounds like a good idea.

Wolenik answered 4/11, 2014 at 16:59 Comment(1)
Your answer goes in the right direction. In the meantime I have improved my pattern follwing the accepted answer here. To my mind this is the right answer to my questions.Sigmund
C
2

I stumbled across a really, really nice blog article about the notorious Eval here. The article does discuss in detail. You won't be able to alleviate all security concerns, but you can prevent Cross-Script Attacks by building tokens for the input. This would in theory prevent malicious code that could be harmful from being introduced.

Your only other hurdle will be Man-In-The-Middle Attacks. I'm not sure if that would be possible, as you can't trust input and output.

The Mozilla Developer Network does explicitly state:

eval() is a dangerous function, which executes the code it's passed with the privileges of the caller. If you run eval() with a string that could be affected by a malicious party, you may end up running malicious code on the user's machine with the permissions of your webpage / extension. More importantly, third party code can see the scope in which eval() was invoked, which can lead to possible attacks in ways to which the similar Function is not susceptible.

eval() is also generally slower than the alternatives, since it has to invoke the JS interpreter, while many other constructs are optimized by modern JS engines.

There are safer (and faster!) alternatives to eval() for common use-cases.

I'm slightly against Eval and truly try to use it when warranted.

Carolyncarolyne answered 4/11, 2014 at 17:8 Comment(0)
I
2

There was a question long ago much like this. So I dusted off some old code and fixed it up.

It essentially works by taking advantage of the with keyword and providing it with a frozen empty object. The prototype of the empty object is filled with null properties, the keys of which match the names global variables like self, window and their enumerable property keys; The prototype object is also frozen. eval is then called within the with statement (Almost the same way that scripts run with an implicit with(window){} block if I understand correctly). When you try to access window or its properties you get redirected (via the with block) to null versions (with same key) found in empty object (or rather the empty object's prototype):

function buildQuarantinedEval(){
    var empty=(function(){
        var exceptionKeys = [
                "eval", "Object", //need exceptions for these else error. (ie, 'Exception: redefining eval is deprecated')
                "Number", "String", "Boolean", "RegExp", "JSON", "Date", "Array", "Math",
                "this",
                "strEval"
        ];
        var forbiddenKeys=["window","self"];
        var forbidden=Object.create(null);
        [window,this,self].forEach(function(obj){
            Object.getOwnPropertyNames(obj).forEach(function(key){
                forbidden[key]=null;
            });
            //just making sure we get everything
            Object.keys(obj).forEach(function(key){
                forbidden[key]=null;                    
            });
            for(var key in obj){
                forbidden[key]=null;
            }
        });
        forbiddenKeys.forEach(function(key){
            forbidden[key]=null;
        });
        exceptionKeys.forEach(function(key){
            delete forbidden[key];
        });
        Object.freeze(forbidden);
        var empty=Object.create(forbidden);
        Object.freeze(empty);
        return empty;
    })();
    return function(strEval){
        return (function(empty,strEval){
            try{
                with(empty){
                    return eval(strEval);
                }               
            }
            catch(err){
                return err.message;
            }
        }).call(empty,empty,strEval);
    };
}

Setup by building a function/closure that evaluates some expression:

var qeval=buildQuarantinedEval();
qeval("'some expression'");     //evaluate

Tests:

var testBattery=[
    "'abc'","8*8","console","window","location","XMLHttpRequest",
    "console","eval('1+1+1')","eval('7/9+1')","Date.now()","document",
    "/^http:/","JSON.stringify({a:0,b:1,c:2})","HTMLElement","typeof(window)",
    "Object.keys(window)","Object.getOwnPropertyNames(window)",
    "var result; try{result=window.location.href;}catch(err){result=err.message;}; result;",
    "parseInt('z')","Math.random()",
    "[1,2,3,4,8].reduce(function(p,c){return p+c;},0);"
];
var qeval=buildQuarantinedEval();
testBattery.map(function(code){
    const pad="                  ";
    var result= qeval(code);
    if(typeof(result)=="undefined")result= "undefined";
    if(result===null)result= "null";
    return (code+pad).slice(0,16)+": \t"+result;
}).join("\n");

Results:

/*
'abc'           :   abc
8*8             :   64
console         :   null
window          :   null
location        :   null
XMLHttpRequest  :   null
console         :   null
eval('1+1+1')   :   3
eval('7/9+1')   :   1.7777777777777777
Date.now()      :   1415335338588
document        :   null
/^http:/        :   /^http:/
JSON.stringify({:   {"a":0,"b":1,"c":2}
HTMLElement     :   null
typeof(window)  :   object
Object.keys(wind:   window is not an object
Object.getOwnPro:   can't convert null to object
var result; try{:   window is null
parseInt('z')   :   parseInt is not a function
Math.random()   :   0.8405481658901747
[1,2,3,4,8].redu:   18
*/

Notes: This technique can fail when some properties of window are defined late (after initializing/creating our quarantined eval function). In the past, I've noticed some property keys are not enumerated until after you access the property, after which Object.keys or Object.getOwnPropertyNames will finally be able grab their keys. On the other hand this technique can also be quite aggressive in blocking objects/functions you do not want blocked (an example would be like parseInt); In these cases, you'll need to manually add global objects/functions that you do want into the exceptionKeys array.

*edit* Additional considerations: How well this all performs depends entirely on how well the mask matches that of the property keys of the window object. Any time you add an element to the document and give it a new ID, you just inserted a new property into the global window object, potentially allowing our 'attacker' to grab it and break out of the quarantine/firewall we've setup (i.e. access element.querySelector then eventually window obj from there). So the mask (i.e., the variable forbidden) either needs to be updated constantly perhap with watch method or rebuilt each time; The former conflicts with the necessity of the mask to have a frozen interface, and the latter is kinda expensive having to enumerate all the keys of window for each evaluation.

Like I said earlier, this is mostly old code I was working on, then abandoned, that was quickly fixed up on short order. So it's not by any means thoroughly tested. I'll leave that to you.


and a jsfiddle


Indigestible answered 7/11, 2014 at 4:59 Comment(4)
Your code is defeated with this qeval("setTimeout(function () {this.console.log(this.XMLHttpRequest);}, 0);");Dives
@Dives Hmmm, that's funny. I'm getting "(error) setTimeout is not a function" here. (Firefox w/ Developer Scratchpad). I'll check again with a userscript. Then again, it depends on whether or not you can enumerate all the keys of window, which may or may not be reliable (when I've checked in the past there were certain property keys that didn't get enumerated for some reason).Indigestible
@Dives Okay, I'm getting the same results as you now (via a userscript), certain property keys are obtained consistently by Object.keys/Object.getOwnPropertyNames in one environment but not in another. The results are quite finicky depending on where the code is executed. I suppose if one could reliably get all the keys...Indigestible
@Dives Well I tweaked the code to more exhaustively get the window property names (via Object.getOwnPropertyNames, Object.keys, and for...in) and it appears to have corrected the issue in both userscipt and the developer scratchpad. At this point it almost feels like playing a game of whack-a-mole. And I'm fairly certain you can still get to window props/methods or some other sensitive objects (perhaps by polluting the global variable space, looking for ID'd elements, or tampering with various prototypes that are still exposed). Fun.Indigestible
S
2

I have stated it yet in my question, but to make it more clear I will post it as an answer also:

I think the accepted answer on this question is the correct and only way to completely isolate and constrain eval().

It is also secure against these hacks:

(new ('hello'.constructor.constructor)('alert("hello from global");'))()

(function(){return this;})().alert("hello again from global!");

while(true){} // if no worker --> R.I.P. browser tab

Array(5000000000).join("adasdadadasd") // memory --> boom!
Sigmund answered 7/11, 2014 at 7:59 Comment(1)
My earlier comment on this answer was incorrect. However, I found something else.Dives
S
0

I have small idea about secure eval for small or limited things if you know well what u going to use eval in you can create white list and black list and excute only the strings that has the valid but it good for small covered app for example calculator has few options (x, y) and (+,*,-,/) if i added this characters in white list and add check for script length and study what excepted length of the script run it can be secure and no one can pass that

const x = 5;
const y = 10;

function secureEval(hack_string){
  // 0 risk eval calculator
  const whiteList = ['',' ', 'x', 'y','+','*','/','-'];
  for (let i=0; i<hack_string.length; i++){
    if (!whiteList.includes(hack_string[i])){
      return 'Sorry u can not hack my systems';
    }
  }
  return 'good code system identify result is : ' + eval(hack_string);
}
// bad code
document.getElementById("secure_demo").innerHTML = secureEval('x * y; alert("hacked")');

document.getElementById("demo").innerHTML = secureEval('x * y');
<!DOCTYPE html>
<html>
<body>

<h1>Secure Eval</h1>

<p id="secure_demo"></p>

<p id="demo"></p>
</body>
</html>
Sorce answered 16/3, 2022 at 7:43 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.