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.

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
– KBriggs, Commented Jul 15, 2013 at 0: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() — user1646111
– user1646111, Commented Jul 15, 2013 at 0: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
– KBriggs, Commented Jul 15, 2013 at 0: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
– mario, Commented Jul 15, 2013 at 0:41
possible duplicate of how to translate math espression in a string to integer — mario
– mario, Commented Jul 15, 2013 at 0:44

nice ass · Accepted Answer · 2013-07-15 01:29:47Z

5

Evaluate the code inside a VM - see Runkit_Sandbox
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.
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, '+-/*');

edited Jul 15, 2013 at 1:29

answered Jul 15, 2013 at 0:42

nice ass

16.8k8 gold badges54 silver badges92 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

KBriggs Over a year ago

I will look into the tokenizer option, thank you. That library you link is also interesting.

KBriggs Over a year ago

Just saw your edit: that is actually a remarkably simple solution! Thanks.

bbonev · Accepted Answer · 2013-07-15 00:42:51Z

3

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.

answered Jul 15, 2013 at 0:42

bbonev

1,4372 gold badges16 silver badges33 bronze badges

1 Comment

KBriggs Over a year ago

Good catch, and yes, I think I will despair of salvaging this particular code snippet and try to find another way.

user2816761 · Accepted Answer · 2017-03-26 10:45:47Z

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 );

Collectives™ on Stack Overflow

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

3 Answers 3

2 Comments

1 Comment

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

2 Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related