Dreaded eval

[KyleMassacre]

For a little while now I have been trying to figure out how to kind of secure the staff crimes page. Now, im no where near decent at regex but thanks to DJK in one of his posts he recommended RegExr and its a pretty nifty little tool if you can understand regex unike myself. So basically I wanted to see if some of the gurus out there can take a look at this and tell me what they think. I tested this and for in my eyes "badwords" it worked pretty dern good. So basically what you do is open up staff_crimes.php and find the "$_POST['percform'] and replace it with this:

$_POST['percform'] = preg_replace("(TRUNCATE|MYSQL(I){0,1}|ALTER|CREATE|DELETE|DROP|EXEC(UTE){0,1}|INSERT( +INTO){0,1}|MERGE|SELECT|UPDATE|UNION( +ALL){0,1})i","", $_POST['percform'], -1 ); 

Now what this does is takes a few words that are bad in the wrong hands and gets rid of them. The only draw back I see with this is that the crime gets executed the player may be shown an error so maybe someone has a better idea but it can just be the cost of security.

***EDIT***

I should have maybe thought of just using preg_match() ;)

$percform= preg_match("(TRUNCATE|MYSQL(I){0,1}|ALTER|CREATE|DELETE|DROP|EXEC(UTE){0,1}|INSERT( +INTO){0,1}|MERGE|SELECT|UPDATE|UNION( +ALL){0,1})i","", $_POST['percform'], -1 );
if($percform)
{
   echo "Your input is invalid";
   $h->endpage();
}

whould that be better?

Edited June 9, 2013 by KyleMassacre

[Djkanna]

I would rather look into a method of changing it, than trying to patch it.

[KyleMassacre]

I would rather look into a method of changing it, than trying to patch it.

I agree, eval() probably shouldnt be used anywhere in a file especially in a web game where there are a lot of people whos whole goal is to try and exploit to show their friends they are real cool but at least this way they have a little bit of time to try and find another method if they are worried about anything

[Guest]

Kind of on topic, but how MCC uses eval() for the crimes, what would you suggest otherwise?

[KyleMassacre]

basically you need to do the math yourself instead of relying on eval, take this line for example:

$ec =
               "\$sucrate="
                       . str_replace(
                               array("LEVEL", "CRIMEXP", "EXP", "WILL", "IQ"),
                               array($ir['level'], $ir['crimexp'],
                                       $ir['exp'], $ir['will'], $ir['IQ']),
                               $r['crimePERCFORM']) . ";";

and by default your new crime page is like this:

((WILL*0.8)/2.5)+(LEVEL/4)

Now i havent tested it but im assuming you can just change it to something like this:

$sucrate= str_replace(
                               array("LEVEL", "CRIMEXP", "EXP", "WILL", "IQ"),
                               array($ir['level'], $ir['crimexp'],
                                       $ir['exp'], $ir['will'], $ir['IQ']),
                               $r['crimePERCFORM']) ";

which will print out like:

$sucrate = (($ir['will']*0.8)/2.5)+($ir['level']/4);

 

Then you should just be able to get rid of eval($ec);

Maybe mess around with it and see if it works of if you come up with something better

[Dominion]

It's been a while since I've looked at MCC, how is Eval being used? Is it just for the maths (e.g. (will*0.8/$level) ) ?

Edited June 9, 2013 by Dominion

[Guest]

basically you need to do the math yourself instead of relying on eval, take this line for example:

$ec =
               "\$sucrate="
                       . str_replace(
                               array("LEVEL", "CRIMEXP", "EXP", "WILL", "IQ"),
                               array($ir['level'], $ir['crimexp'],
                                       $ir['exp'], $ir['will'], $ir['IQ']),
                               $r['crimePERCFORM']) . ";";

and by default your new crime page is like this:

((WILL*0.8)/2.5)+(LEVEL/4)

Now i havent tested it but im assuming you can just change it to something like this:

$sucrate= str_replace(
                               array("LEVEL", "CRIMEXP", "EXP", "WILL", "IQ"),
                               array($ir['level'], $ir['crimexp'],
                                       $ir['exp'], $ir['will'], $ir['IQ']),
                               $r['crimePERCFORM']) ";

which will print out like:

$sucrate = (($ir['will']*0.8)/2.5)+($ir['level']/4);

 

Then you should just be able to get rid of eval($ec);

Maybe mess around with it and see if it works of if you come up with something better

 

Hmm good call, o'll have to look into it :), however eval() would still be the best option from what I can see as you never know what the user will input. By them being an admin, it is there job to have secure hosting, and passwords to avoid not getting hacked.

[KyleMassacre]

It's been a while since I've looked at MCC, how is Eval being used? Is it just for the maths (e.g. (will*0.8/$level) ) ?

eval($ec);

 

Hmm good call, o'll have to look into it :), however eval() would still be the best option from what I can see as you never know what the user will input. By them being an admin, it is there job to have secure hosting, and passwords to avoid not getting hacked.

From what I understand eval is NEVER the best option for anything especially for manually entered input

Caution

The eval() language construct is very dangerous because it allows execution of arbitrary PHP code. Its use thus is discouraged. If you have carefully verified that there is no other option than to use this construct, pay special attention not to pass any user provided data into it without properly validating it beforehand.

So if you want to keep it just please be aware. My OP from what I have seen will take out any nasty SQL commands that shouldnt be in there

[Dominion]

Assuming this is purely for the maths, and the problems are that everything is currently allowed why are you blacklisting things? Check for numbers, and key words (e.g. WILL) via a whitelist and block everything else. I don't understand why you're checking for words like CREATE via a blacklist.

That aside I think DJK was right, eval() shouldn't be used here.

[KyleMassacre]

Assuming this is purely for the maths, and the problems are that everything is currently allowed why are you blacklisting things? Check for numbers, and key words (e.g. WILL) via a whitelist and block everything else. I don't understand why you're checking for words like CREATE via a blacklist.

That aside I think DJK was right, eval() shouldn't be used here.

I decided to go with a blacklist because IMO its a bit easier, lets say for example I change around what I want to be used to my success rate now I would have to go and add more whitelist options on top of my $ec/$sucrate in docrime. This way its just a bit easier for myself to alter a little bit but keep my db safe from a bad guy. But yes, eval shouldnt be used and I think this is a little bit easier for the novice or beginner people to do that has a basic understanding of how this works. A quick fix isnt always the best and this is just a band-aid just to buy some time

[Djkanna]

We're currently debating viable methods of doing this, if you want to hop into IRC. :)

[Octarine]

Four years ago or so, this was done and dealt with creating a simple expression evaluator that handled basic operators, constants and even functions. It's not difficult to write, it's safe and secure and it works regardless of whether a hosting environment as disabled the eval() function or not.

[Dominion]

Four years ago or so, this was done and dealt with creating a simple expression evaluator that handled basic operators, constants and even functions. It's not difficult to write, it's safe and secure and it works regardless of whether a hosting environment as disabled the eval() function or not.

Could you give us a link or an example of what you're talking about?

[KyleMassacre]

please do to end the headache haha. A few of the guys: Ian, Dom, Nick, and DjKanna was going through a meltdown earlier lol

[Dominion]

There is nothing "evil" about using Eval, and if used properly it's a powerful tool. I simply feel it's not being used properly here at all.

[Guest]

please do to end the headache haha. A few of the guys: Ian, Dom, Nick, and DjKanna was going through a meltdown earlier lol

It was only a 4 hours discussion haha

[Djkanna]

please do to end the headache haha. A few of the guys: Ian, Dom, Nick, and DjKanna was going through a meltdown earlier lol

Meltdown no...Throwing **** at a wall and seeing if it sticks, yes.

[Alan]

The question is not whether evel() itself is safe, it's more a case of the data you pass to it. How does that data get there? Is it stored in the database? Can it be manipulated by players and/or staff. If your project is vulnerable, then it's not a wise function to use. So this itself suggests an initial action - secure your project first before considering looking at alternatives to eval().

Assuming you have got a secure product - and no, this does not (to the best of my knowledge) include any out-of-the-box installs of GRPG, NWE, McCodes or ezRPG, then you have to look at what you want to evaluate. Arbitrary code? Use eval() and hope that no update to your code allows any form of rogue data to get evaluated. If however you want to evaluate simple mathematical statements ie 1 + 2 * 3 / PI + sin(5.1) - then there is a pretty decent case for writing your own system.

How you choose to write it is dependent upon your level of skill, and whether you can be bothered to do it the proper way or take shortcuts. Personally, I'm in favor of a mix of the two - I skip certain error checking, but then as I'm the one using it on my product.. only a careless mistake would have knock on effects, and I test ... lots.

Initially, you need to take the expression and split it into tokens - just like PHP itself does. ie:

1 + 2 * 3 / PI + sin(5.1)

might become

TOKEN_INTEGER(1)
TOKEN_PLUS
TOKEN_INTEGER(2)
TOKEN_MULTIPLY
TOKEN_INTEGER(3)
TOKEN_DIVIDE
TOKEN_CONSTANT(PI)
TOKEN_PLUS
TOKEN_SYMBOL(sin)
TOKEN_OPEN_BRACKET
TOKEN_FLOAT(5.1)
TOKEN_CLOSE_BRACKET

Once you have this - the lexer stage - then you need the parser stage; and this is usually where the fun begins. There are many different ways of doing it - from the simple to the insanely complex. A simple method is to use two stacks, one for operators, one for values and push items onto the relevant stack one at a time, checking at each stage if you can reduce the stack. For example:

push to value stack TOKEN_INTEGER(1)
push to operator stack TOKEN_PLUS
push to value stack TOKEN_INTEGER(2)
push to operator stack TOKEN_MULTIPLY
push to value stack TOKEN_INTEGER(3)

at this stage we can multiple 2 x 3 to get 6, leaving 1 and 6 on the value stack and + on the operator stack

push to operator stack TOKEN_DIVIDE
push to value stack 3.141...

again, we can now reduce the stacks by dividing 6 by 3.141 to get 1.19, leaving 1 and 1.9 on the value stack and + on the operator stack.

etc etc.

You can try a very simple lexer out for yourself -- http://node86.com/pastebin/vmjjm/ (Please note the link will only be available for a short time).

[Dominion]

First off thank you very much for replying, and in such detail. I always look forward to seeing you post.

 

The question is not whether evel() itself is safe, it's more a case of the data you pass to it.

True with most things of this nature where programming is concerned and something more people need to remember.

Your particular solution is one we discussed (splitting the values and evaluation individually), although not in such detail, and if it was purely a math based operation using only numbers from the user input would have made perfect sense as a patch. Unfortunately we were stumped by mccodes somewhat free ability to use ANYTHING user based as part of the math for example -

5*WILL/EXP+6-LEVEL

Should in theory be a valid input. Checking this is... stupidly long. I think there are also several other user specific stats you could use.

With this in mind it would have taken us less time to recreate the system then to patch the current one, and in my opinion is why eval() simply shouldn't be used here. One alternative was to simply use a base sum (e.g. x*exp/$number) and substitute the values in. No need for eval(), and does the same thing... well actually you lose a little flexibility that you didn't need anyway.

Edited June 9, 2013 by Dominion

[Alan]

Variables are symbols, which a basic lexer like the one I presented returns. One task of the parser is to lookup the symbols in some form of table - not database table, just an array will suffice, so you could start with something akin to:

$symtab = new SymbolTable();
$symtab->defineConstant('PI', 3.14);
// more constant definitions here...

$symtab->defineFunction('sin', 'my_sin'); // sin, calls my_sin() function (see below)
$symtab->defineFunction('log', 'log10'); // log, calls the builtin log10 function
$symtab->defineFunction('floor'); // floor, calls the builtin floor function
// more function definitions here

$symtab->defineVariable('level', $ir['level']); // assuming the usual mccodes variable naming convention
$symtab->defineVariable('will', $ir['will']);
$symtab->defineVariable('exp', $ir['exp']);
// more variables from the user's array here

$parser = new Parse($symtab);
$result = $parser->evaluate('5*WILL/EXP+6-LEVEL');

If you are wondering, my_sin() would perhaps be defined as:

function my_sin( $degrees ) {
   return sin(deg2rad($degrees));
}

It soons becomes pretty apparent, that you are really creating a whitelist expression evaluation, something that is pretty much bullet-proof.

The symbol table itself can be very easy, an associative array containing the symbol, the type of symbol (constant, function or variable) and the value (or in the case of function, the name of the function). The parser of course is more fun to write, but is not difficult if you start small.

There are some interesting problems to solve in the parser, operator precedence, and possibly conversion from infix to postfix, but both are very well covered at your local corner Google, and in fact I've already touched on a solution which does not require conversion to postfix, saving one possibly vital step.

[Dominion]

This started out as a "fix what's there" conversation. The more we talk about it the more work is added, and the easier it would be to redo the entire thing. You're solution should be enforced on docrime.php.

In staff_crime.php (the original file we were trying to create a quick check for...) we could simply enforce a structure e.g.

8 * WILL / LEVEL + 4

explode() and check to ensure (against an array) each part is a number, operation(+/*-) or a set word (LEVEL,WILL etc)

It's late here so I can't give a better response then that tonight. Thank you very much for answering, and I'll probably come back to this (if not redo the whole system anyway) tomorrow. Might even be the first thing I've ever done for mcc...

Edited June 10, 2013 by Dominion

[rockwood]

PDO with bind values are not enough in this case ?

[a_bertrand]

We are not talking about SQL injection but PHP code injection. If you have an eval somewhere, you can run basically any code there. Of course it depends how the code reach this eval and who can put it there. If the code could be written by a user, then any code could be run by anyone. Having a parser like Alan said here, would be a great solution to limit what kind of code can be run. Of course it may be a bit more work than simply having some sort of checker of the code, yet it is for sure more safe as it will understand only the commands you develop in your parser.

[Alan]

Using explode() it extract the tokens is fine up to a point, but it's very restrictive. Most computer languages have some form of lexical analysis often created using 3rd party tools to define the token structure and output code. (lex, flex etc). The key here is to create something that is highly extendable, and can itself be used elsewhere.

A long time when I initially wrote this type of eval() system, in part after watching one enterprising member from these very boards inject a project's crime table with something that essentially wiped the entire database (both the project and the member in question will remain nameless), I realized it could also be used to create a requirements system where the statement become highly complex - almost like a language in itself. I still use this system in the occasional PHP project today, so it's well worth the time and effort.

@rockwood: no.

[rockwood]

Thanks for nice Answer