safely using the eval function in php: modifying user input to avoid security issues

Question

I am taking over over some webgame code that uses the eval() function in php. I know that this is potentially a serious security issue, so I would like some help vetting the code that checks its argument before I decide whether or not to nix that part of the code. Currently I have removed this section of code from the game until I am sure it's safe, but the loss of functionality is not ideal. I'd rather security-proof this than redesign the entire segment to avoid using eval(), assuming such a thing is possible. The relevant code snip which supposedly prevents malicious code injection is below. $value is a user-input string which we know does not contain ";".

1 $value = eregi_replace("[ \t\r]","",$value);
2 $value = addslashes($value);
3 $value = ereg_replace("[A-z0-9_][\(]","-",$value);
4 $value = ereg_replace("[\$]","-",$value);
5 @eval("\$val = $value;");

Here is my understanding so far:

1) removes all whitespace from $value

2) escapes characters that would need it for a database call (why this is needed is not clear to me)

3) looks for alphanumeric characters followed immediately by \ or ( and replaces the combination of them with -. Presumably this is to remove anything resembling function calls in the string, though why it also removes the character preceding is unclear to me, as is why it would also remove \ after line 2 explicitly adds them.

4) replaces all instances of $ with - in order to avoid anything resembling references to php variables in the string.

So: have any holes been left here? And am I misunderstanding any of the regex above? Finally, is there any way to security-proof this without excluding ( characters? The string to be input is ideally a mathematical formula, and allowing ( would allow for manipulation of order of operations, which currently is impossible.

Answer 1

1. Evaluate the code inside a VM - see Runkit_Sandbox

2. Or create a parser for your math. I suggest you use the built-in tokenizer. You would need to iterate tokens and keep track of brackets, T_DNUMBER, T_LNUMBER, operators and maybe T_CONSTANT_ENCAPSED_STRING. Ignore everything else. Then you can safely evaluate the resulting expression.

3. A quick google search revealed this library. It does exactly what you want...

---

A simple example using the tokenizer:

$tokens = token_get_all("<?php {$input}");
$expr = '';

foreach($tokens as $token){

  if(is_string($token)){

    if(in_array($token, array('(', ')', '+', '-', '/', '*'), true))
      $expr .= $token;

   continue;   
  }

  list($id, $text) = $token;

  if(in_array($id, array(T_DNUMBER, T_LNUMBER)))
    $expr .= $text;
}

$result = eval("<?php {$expr}");

(test)

This will only work if the input is a valid math expression. Otherwise you'll get a parse error in your eval`d code because of empty brackets and stuff like that. If you need to handle this too, then sanitize the output expression inside another loop. This should take care of the most of the invalid parts:

while(strpos($expr, '()') !== false)
  $expr = str_replace('()', '', $expr);

$expr = trim($expr, '+-/*');