4

I have an input onchange that converts numbers like 05008 to 5,008.00.

I am considering expanding on this, to allow simple calculations. For example, 45*5 would be converted automatically to 225.00.

I could use a character white-list ()+/*-0123456789., and then pass the result to eval, I think that these characters are safe to prevent any dangerous injections. That is assuming I use an appropriate try/catch, because a syntax error could be created.

  • Is this an OK white-list, and then pass it to eval?

  • Do recommend a revised white-list

  • Do you recommend a different approach (maybe there is already a function that does this)

    I would prefer to keep it lightweight. That is why I like the eval/white-list approach. Very little code.

What do you recommend?

700 Software
  • 85,281
  • 83
  • 234
  • 341
  • Are you passing the raw inputs to other users/database/somewhere else? Otherwise I wouldn't worry about users injecting themselves. – JJJ Mar 31 '12 at 15:45
  • 1
    It is possible for a link to be fashioned to the page that causes custom default values to be filled in. If a malicious somebody gets the user to click the link, I need to make sure it won't inject javascript code into my page, causing an XSS attack. I think that white-list will be okay, but I know that [JavaScript can be done without letters](http://security.stackexchange.com/questions/8263/javascript-written-only-with-brackets), so I wanted to run it by you guys first. And I often get better ideas whenever I do. – 700 Software Mar 31 '12 at 15:56
  • Just to clarify, the JavaScript's job is to normalize the input. The server just rejects anything unexpected. So the **server** will not be sending the raw input anywhere until it is converted to `double` - without any fancy interpretation, because that is all done on the client side. Server uses normal known safe parsing, JavaScript uses this fancy made-to-be-safe math function. – 700 Software Mar 31 '12 at 16:00

3 Answers3

3

That whitelist looks safe to me, but it's not such a simple question. In some browsers, for example, an eval-string like this:

/.(.)/(34)

is equivalent to this:

new RegExp('.(.)').exec('34')

and therefore returns the array ['34','4']. Is that "safe"?

So while the approach can probably be made to work safely, it might be a very tricky proposition. If you do go forward with this idea, I think you should use a much more aggressive approach to validate your inputs. Your principle should be "this is a member of a well-defined set of strings that is known to be 'safe'" rather than "this is a member of an ill-defined set of strings that excludes all strings known to be 'unsafe'". Furthermore, to avoid any risk of operators peeking through that you hadn't considered (such as ++ or += or whatnot), I think you should insert a space in front of every non-digit-non-dot character; and to avoid any risk of parentheses triggering a function call, I think you should handle them yourself by repeatedly replacing (...) with a space plus the result of evaluating ... (after confirming that that result is a number) plus a space.

(By the way, how come = is in your whitelist? I just can't figure out what that's useful for!)

ruakh
  • 175,680
  • 26
  • 273
  • 307
  • I believe that the worst it could do is freeze up the window by running some complex regex that is inefficient. At any rate, I will definitely require a number to come before the slash to prevent this risky thing from happening. – 700 Software Apr 02 '12 at 12:25
2

Given that extremely restrictive whitelist, I can't see any way of performing a malicious action beyond throwing an exception. The bracket trick won't work since it requires square brackets [].

Perhaps the safest option is to modify your page's default values parser to only accept numbers and throw out anything else. That way, potentially malicious code in a link will never make it to eval.

This only leaves the possibility of the user typing something malicious into a field, but why even bother worrying about that? The user already has access to a console (Dev Tools) they could use to execute arbitrary code.

Community
  • 1
  • 1
josh3736
  • 139,160
  • 33
  • 216
  • 263
0

An often overlooked issue with eval is that it causes problems for javascript minifiers.

Some minifiers like YUI take the safe route and stop renaming variables as soon as they see an eval statement. This means your javascript will work but your compressed file will be larger than it needs to be.

Other's like Google Closure Compiler will continue to rename variables but if you are not careful they can break your code. You should avoid passing strings with variable names in it to eval. so for example.

var input = "1+2*3";

var result = eval("input"); // unsafe

var result = eval(input); // safe
josh3736
  • 139,160
  • 33
  • 216
  • 263
thejosh
  • 137
  • 2
  • Your first point is a great thing to look out for; I hadn't even considered that the presence of `eval` might affect a minifier. The second point is a little more complex. Assuming that this is happening inside a function, `eval("input")` wouldn't work at all since `eval` code runs in the global scope; it wouldn't have access to the function's local `input` variable. (This is good; you probably wouldn't want `input` to be global anyway.) – josh3736 Apr 01 '12 at 01:45