7

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.

KBriggs
  • 1,220
  • 2
  • 18
  • 43
  • The function is called to evaluate a user-input mathematical expression which gets mapped into the code's native variables just prior to this sequence of calls. – KBriggs Jul 15 '13 at 00:34
  • based on your comment, I think it's better to change the logic, you can do it by other ways, for example: provide math symbols and then process it later not directly by eval() –  Jul 15 '13 at 00:35
  • Yea, I know that it's probably better to avoid eval() entirely. I was just hoping that I could avoid redesigning part of the code instead of just modding the existing. – KBriggs Jul 15 '13 at 00:37
  • Your inherited code is using the blacklist approach at safeguarding. Which usually is the least advisable method. For mathematical expressions it's however pretty easy to *whitelist* allowed expressions. Use `preg_match` and optionally a recursive pattern to allow formulas/parens etc. – mario Jul 15 '13 at 00:41
  • possible duplicate of [how to translate math espression in a string to integer](http://stackoverflow.com/questions/8164168/how-to-translate-math-espression-in-a-string-to-integer) – mario Jul 15 '13 at 00:44
  • possible duplicate of http://stackoverflow.com/questions/6979831/validate-user-inputted-php-code-before-passing-it-to-eval – mario Jul 15 '13 at 00:45
  • Off the top of my head, `system/*comment*/('evil-command')`. There are probably a hundred other exploits. `eval` requires extreme care and is almost always better not used; `eval` plus blacklisting === death and fail. – bobince Jul 15 '13 at 20:15

3 Answers3

5

  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, '+-/*');
nice ass
  • 16,471
  • 7
  • 50
  • 89
2

Matching what is allowed instead of removing some characters is the best approach here.

I see that you do not filter ` (backtick) that can be used to execute system commands. God only knows what else is also not prevented by trying to sanitize the string... No matter how many holes are found, there is no guarantee that there cannot be more.

Assuming that your language is not quite complex, it may not be that hard to implement it yourself without the use of eval.

bbonev
  • 1,406
  • 2
  • 16
  • 33
  • Good catch, and yes, I think I will despair of salvaging this particular code snippet and try to find another way. – KBriggs Jul 15 '13 at 00:46
0

The following code is our own attempt to answer the same sort of question:

$szCode = "whatever code you would like to submit to eval";

/* First check against language construct or instructions you don't allow such as (but not limited to) "require", "include", ..." : a simple string search will do */
if ( illegalInstructions( $szCode ) )
{
   die( "ILLEGAL" );
}

/* This simple regex detects functions (spend more time on the regex to
   fine-tune the function detection if needed) */
if ( preg_match_all( '/(?P<fnc>[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*) ?\(.*?\)/si',$szCode,$aFunctions,PREG_PATTERN_ORDER ) )
{
    /* For each function call */
    foreach( $aFunctions['fnc'] as $szFnc )
    {
        /* Check whether we can accept this function */
        if ( ! isFunctionAllowed( $szFnc ) )
        {
            die( "'{$szFnc}' IS ILLEGAL" );
        }   /* if ( ! q_isFncAllowed( $szFnc ) ) */
    }
}
/* If you got up to here ... it means that you accept the risk of evaluating
   the PHP code that was submitted */
eval( $szCode );
user2816761
  • 81
  • 1
  • 1