Is this a bad use of Javascript's eval()?

Question

I've read a few things about Javascript's eval, so I think my use makes sense.

I'm in an information security class, and we're doing some statistical analysis on lottery cards. However, ours are bingo cards (and probably completely unlikely to be prey to this particular situation). However, our current process is severely buggy, so I had the idea of storing the card data on my server, and allow classmates to write javascript functions in a textarea and then push a button that would just eval(textarea.value). That would allow us to do some fairly useful computations on the data without too much problem. I also had the idea of "storing procedures" as text that they could load into the textarea and then evaluate.

Is this a bad area for eval()? Or a somewhat grey area? Also, what are some of the potential problems I could face? Obviously they could potentially bork their view of the page, but as long as I'm not doing anything stupid, a simple page reload should fix that, correct?

Thanks!

score 6 · Accepted Answer · edited Jun 20 '20 at 09:12

If the user input being eval()'d is always coming from the user, that should be fine, the only malicious JavaScript they can then run is on themselves (which they could do anyway).

If it speaks to server side code, make sure it is validated like any other external input.

You need to also ensure it can not be pre-filled from anything like a GET param etc.

Update

If you are only running basic math with variables, you could use a regex to ensure the value is clean from non arithmetic and then eval() it.

For example, your equations can have placeholders CARD1 and CARD2.

var textarea = document.getElementsByTagName('textarea')[0],
    placeholders = ['CARD1', 'CARD2'];

document.getElementsByTagName('button')[0].onclick = function() {

    var value = textarea.value;

    // Strip placeholders
    var placeholderStripped = value.replace(new RegExp(placeholders.join('|'), 'g'), '');

    var safe = placeholderStripped.match(/^[\d\+\-\/\*\(\)]+$/);

    if (safe == null) {

        alert('Not a valid equation');
        return;

    }

    for (var i = 0, placeholdersLength = placeholders.length; i < placeholdersLength; i++) {
        value = value.replace(new RegExp(placeholders[i], 'g'), document.getElementById(placeholders[i].toLowerCase()).value);
    }

    document.getElementById('result').innerHTML = eval(value);

}

jsFiddle.

Use simple placeholder names, and all caps for convenience. If you use more complicated names, you may need to use a regex escaping function before you join() them.
We allow numbers 0-9, +, -, /, *, ( and ). We don't allow for decimals currently. That can be added if needed.
When we finally eval() the input, it should only contain placeholder names (which will be substituted) and the basic arithmetic characters above.
The code posted above is highly dependent on the jsFiddle context. Of course, you will never allow placeholder names to be arbitrary - make them what they should be.

score 3 · Answer 2 · answered Mar 10 '11 at 01:13

3

I don't want to be melodramatic but this is probably one of the worst usages of eval() since you will be executing arbitrary, user-supplied JavaScript. This would allow a user to execute code on your domain which would make an XSS attack rediculously simple.

Edit: On second thought maybe I am being too dramatic. As long as you aren't persisting these functions and they are simply being eval'd on the client's machine then you aren't really allowing anything that the user can't already do in their local JavaScript console in the browser. Just be sure not to save these functions for other users to use later.

answered Mar 10 '11 at 01:13

Andrew Hare

344,730
71
640
635

I'm probably missing something, but isn't allowing `eval()` on a `textarea` fine so long as the code doesn't update any server side state or allow itself to be pre-filled? I mean, fine from a security standpoint at least. – alex Mar 10 '11 at 01:16
@alex - No, you are right on. I assumed these functions were being saved somewhere for reuse. – Andrew Hare Mar 10 '11 at 01:19
@Tomalak Geret'kal Yeah, of course, its use case would have to be JavaScript users who knew what they are doing :) – alex Mar 10 '11 at 01:19
@alex: hehe :) Good luck with that, eh! – Lightness Races in Orbit Mar 10 '11 at 01:21
If I *were* to persist the functions (i.e. so classmates to share), would it be reasonable to only populate the textarea, requiring the user to click the "evaluate" button? Of course the code being stored would be escaped, etc. - would simply filtering out all occurrences of alert, script, img, and css from the raw text be reasonable protection, or would that still allow for XSS techniques? – Wayne Werner Mar 10 '11 at 01:52
@Wayne Werner You could not safely filter out *bad* functions, mainly because JavaScript will execute `eval('a' + 'lert("whoops")')`, which won't be matched by string matching. – alex Mar 10 '11 at 01:55
So in this case, it's either don't store functions at all, or simply rely on the person clicking the "evaluate" button to be able to look at the function and say "ah yes, let's not click that"? – Wayne Werner Mar 10 '11 at 02:05
@Wayne Werner: Actually, you could overwrite the globals "alert", "eval", "XMLHttpRequest", etc. As long as you do this before the user code is evaluated, it should be (reasonably) safe. That combined with the user paying attention and not evaling anything weird-looking should be enough for your use case. Obviously be much more careful if you plan to do anything more ambitious (like a public website). – Pauan Nov 21 '12 at 03:02

score 1 · Answer 3 · answered Mar 10 '11 at 01:20

From a security standpoint this is OK, since the user can only break their own browser... which is nothing new.

However, from a GUI design standpoint it would be considered messy. If the user can provide input that breaks the interface — and of course with full scripting access they can — then that's considered the fault of the interface.

That said, you're not making a commercial product and it sounds like this will save you a lot of effort, so I'd just go for it.

Is this a bad use of Javascript's eval()?

3 Answers3

Update