The quest for safe eval() of user input

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Hory
Forum Newbie
Posts: 8
Joined: Sat Dec 15, 2007 11:44 am

The quest for safe eval() of user input

Post by Hory »

I'm trying to do a sort of web-game that requires simple to average user scripting. I figured the simplest and most powerful language would be a "lite" version of PHP. A lot of people are saying eval() can never be secured if you allow user input, but I think there's a limit to the number of ways you could hack it, and they can be stopped.
Here's my general plan for a code "securizer":

1) Transform the code from the user into PHP tokens.
2) Verify if all the tokens are in the pre-defined "secure tokens" list ("T_FOREACH", for example, would be a safe token).
3) Verify if all the "function name" tokens (T_STRING) are in the pre-defined "safe functions" list ("rand", for example, would be a safe function).
4) Verify if any of the variable names are in the "unsafe variables" list (eg "$_SESSION"), or alternatively, only allow certain variable names.
5) Deny the possibilty for dynamic function calling (eg. "$variable ();"
6) Deny the possiblity for dynamic variable names (eg. "$$something", where, let's say, "$something = '_SESSION';");

Points 1 - 4, are pretty much easily doable. What I'm not sure about are 5) and 6). What are all the forms of dynamic function or variable naming, and how could they be stopped? If anyone is interested in helping develop a safe eval() class, please post your suggestions.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

There is little you can do to make it completely safe.

Likely the simplest is to disable nearly all functions and classes, or run their code in a sandbox.

If one's design requires eval() one needs a redesign a large portion of the time.
User avatar
Weirdan
Moderator
Posts: 5978
Joined: Mon Nov 03, 2003 6:13 pm
Location: Odessa, Ukraine

Post by Weirdan »

like feyd said, your best option (except redesign) would be to use sandboxed php interpreter, which is possible using runkit: http://ua2.php.net/manual/en/runkit.sandbox.php
User avatar
stereofrog
Forum Contributor
Posts: 386
Joined: Mon Dec 04, 2006 6:10 am

Post by stereofrog »

Well, if you're going to parse your language in any case, why should this language be php? Define the syntax for the language and create a parser/executor for it using pear ParserGenerator or similar tool. Much cleaner than trying to fool the php interpreter.
Hory
Forum Newbie
Posts: 8
Joined: Sat Dec 15, 2007 11:44 am

Post by Hory »

stereofrog wrote:Well, if you're going to parse your language in any case, why should this language be php? Define the syntax for the language and create a parser/executor for it using pear ParserGenerator or similar tool. Much cleaner than trying to fool the php interpreter.
1) I'd rather not reinvent the wheel, especially as there's nothing wrong with PHP's syntax.
2) I don't think I could recreate even 5% of the functionality offered by PHP (dozens of string functions, dozens of array functions more, mathematical functions, control structures, operators, etc.)
3) The users having to learn yet another language is just a disadvantage.
4) If this is a better solution then have others made safe "for the web" languages? I can't find anything worthy.

Thanks.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Oh, I think you should reinvent the wheel. For what you're planning, a very simple scripting language should do the trick, and would make it a lot easier for users to interface with "actions" that actually make sense in the game's context. Note that this is by no means a small undertaking; to save yourself trouble you can probably use PHP's parsing capabilities to adopt the same syntax, but actual execution needs to be well thought out and done on your own.
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Post by Kieran Huggins »

AC++
Hory
Forum Newbie
Posts: 8
Joined: Sat Dec 15, 2007 11:44 am

Post by Hory »

OK guys, check out the safer eval() class. Try to hack it, but please tell me if you do :).
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Try to hack it, but please tell me if you do
I did, in 5 minutes, what do I win :) (not in your logs, don't bother checking)
Your code produces lots and lots of warnings as well, do fix them, it's unprofessional.

The hole was too easy, I will now go and check the code further. I'll drop you a note when I'm done.
Hory
Forum Newbie
Posts: 8
Joined: Sat Dec 15, 2007 11:44 am

Post by Hory »

Mordred wrote:I did, in 5 minutes, what do I win :) (not in your logs, don't bother checking)
You get to make it harder for yourself to hack further versions :).
Your code produces lots and lots of warnings as well, do fix them, it's unprofessional.
Yeah, at some point in the future I will. For now I am concerned primarily about security, and I don't mind if users can see what's wrong with their code.
The hole was too easy, I will now go and check the code further. I'll drop you a note when I'm done.
Thanks. This is only the first release, hopefully it can be further improved.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

I was refering to warnings in your code, not the code that the user submits. When I switched to php5 there were fewer of them (apparently the tokenizer produces some different tokens), but they still persist.

There are also bugs in example.php, for example this:

Code: Select all

$code = htmlentities($_POST['code']);
Is not only the wrong thing to do, but it is also called in a wrong way, and on top of all that leads to a logical error later, if you try to eval something like

Code: Select all

if ($v>5) ...
Furthermore, it doesn't add security by limiting the syntax of the submitted source code (which I understand is an unintentional side effect, not an intended security measure, but still worth mentioning), as it can be bypassed by passing data by other means (see below).

Now for the main course:

1. A trivial bypass vulnerability occurs in evalSyntax(), this is what's wrong:

Code: Select all

$code = eval('if(0){' . $code . '}'); // Put $code in a dead code sandbox to prevent its execution
The attacker will simply submit code that closes the curly brace, execute and then end with a balancing opening brace.

Here's a sample backdooring attack (which may not work, depending on how the server is setup)

Code: Select all

;}$fp=fopen('1.php','w');fputs($fp,$_GET['p']);{
This will work unregarding the $execute parameter in checkScript(). Not only are you calling eval() at all, but you're also calling it when the user said you shouldn't.

2. Bypass of var access restriction:

Code: Select all

$v='x'; 
$ {$v}='4'; //note the space
3. Bypass of function call restriction:

Code: Select all

$v='print'; 
$v/**/(5);
-------

I think this is a great thing to play with, but I strongly advise (as did all posters before me) against using it in a production environment. There are too many factors that may lead to compromise in some unobvious way -- all because you're running eval().

As for using this as an user-accessible scripting language for a web game, I think this is an overkill, and you should simplify that part of the design of the game in question.
Hory
Forum Newbie
Posts: 8
Joined: Sat Dec 15, 2007 11:44 am

Post by Hory »

Thanks for the detailed analysis. I've made another quick release of Safer eval().
1. A trivial bypass vulnerability occurs in evalSyntax(), this is what's wrong:
My bad, I implemented a third-party script without thinking of how it can be hacked. Now it shouldn't be possible to put a closing bracket unless you already opened one, but I think that eventually, syntax checking will be placed after the "legal function checking" step, to avoid the risk of executing harmful functions.
2. Bypass of var access restriction:
Oops, an older version actually disallowed "stand-alone" dollar tokens, but I had removed that protection. I've put it back in now, in a different form: nothing will be allowed after a dollar sign unless it's alphanumeric (or the underline character).
3. Bypass of function call restriction:
Variable function names are a hard one to stop, because it's hard to determine if the opening parenthesis is for running a function, or something else. For this release, I've made comments illegal. Hope I'll find a better solution.
I think this is a great thing to play with, but I strongly advise (as did all posters before me) against using it in a production environment. There are too many factors that may lead to compromise in some unobvious way -- all because you're running eval().

As for using this as an user-accessible scripting language for a web game, I think this is an overkill, and you should simplify that part of the design of the game in question.
Well, as much as this "safe" eval() limits PHP, I still can't find a better user-controlled scripting language that could be implemented neatly on the web. I think that this is, for now, a gap in the world of software.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Hory wrote:Thanks for the detailed analysis. I've made another quick release of Safer eval().:
1. A trivial bypass vulnerability occurs in evalSyntax(), this is what's wrong:
Too quick, I'm afraid, it's still not fixed. Nothing is fixed until it's tested! (Admit it, you didn't. Not a good way to write secure code, I'm afraid). The bug is funny

Hory wrote:My bad, I implemented a third-party script without thinking of how it can be hacked. Now it shouldn't be possible to put a closing bracket unless you already opened one, but I think that eventually, syntax checking will be placed after the "legal function checking" step, to avoid the risk of executing harmful functions.
Now that's a good policy!

I don't think regexp parsing is a good way to do the other checks though. Stick to the tokens if you must, but there's always the chance that you've missed something. The same applies to the huge list of tokens and functions -- I'm sure you haven't checked them all what they do.

Maybe you should define your problem domain first, and then expand the feature set one by one. Which brings me to my next point:
Hory wrote: Well, as much as this "safe" eval() limits PHP, I still can't find a better user-controlled scripting language that could be implemented neatly on the web. I think that this is, for now, a gap in the world of software.
What gap? What is the purpose of this code? What problem does it solve? There are languages that are better suited for sandboxing, I'm sure, but even if PHP was the prime choice, you still have to explain to yourself why you want to do this.
Hory
Forum Newbie
Posts: 8
Joined: Sat Dec 15, 2007 11:44 am

Post by Hory »

Mordred wrote:Too quick, I'm afraid, it's still not fixed. Nothing is fixed until it's tested! (Admit it, you didn't. Not a good way to write secure code, I'm afraid). The bug is funny
Sure I did, even with the code you gave me, or something simpler like } echo 'xyz'; {, it should say "Syntax error".
Now that's a good policy!

I don't think regexp parsing is a good way to do the other checks though. Stick to the tokens if you must, but there's always the chance that you've missed something. The same applies to the huge list of tokens and functions -- I'm sure you haven't checked them all what they do.
Actually I've checked all the tokens (there were a few which had no explanation, sure) and all the functions from the main/useful categories.
Maybe you should define your problem domain first, and then expand the feature set one by one. Which brings me to my next point:

What gap?
Expanding the feature set starting from nothing? I want a better solution that writing my own parser and interpreter. And if there isn't one - that's why I'm saying there's a gap.
What is the purpose of this code? What problem does it solve? There are languages that are better suited for sandboxing, I'm sure, but even if PHP was the prime choice, you still have to explain to yourself why you want to do this.
The purpose is to allow simple and safe variable-focused user-controlled scripting in web applications (in this case - games). Even if I managed to find a web host with an interpreter for another language that would be more appropriate, that language would have its share of dangerous functions. In trying to restrict them, I'd be back to what I'm doing with PHP right now.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

First, I must apologize if I sound too harsh or patronizing, I somehow believe people should read my mind, instead of the few words I actually wrote. :)
Hory wrote:
Mordred wrote:Too quick, I'm afraid, it's still not fixed. Nothing is fixed until it's tested! (Admit it, you didn't. Not a good way to write secure code, I'm afraid). The bug is funny
Sure I did, even with the code you gave me, or something simpler like } echo 'xyz'; {, it should say "Syntax error".
Yes, it says "Syntax error".
Yes, it executes the script.
(Yes, after the ob_start() you shouldn't see anything. Try injecting a ob_end_clean() first)

I won't post POC code here, because it could directly damage your sf.net webspace, but you should be able to test it in your development environment.

There's a problem with one of the new if-s that's causing this.
Hory wrote:
Now that's a good policy!

I don't think regexp parsing is a good way to do the other checks though. Stick to the tokens if you must, but there's always the chance that you've missed something. The same applies to the huge list of tokens and functions -- I'm sure you haven't checked them all what they do.
Actually I've checked all the tokens (there were a few which had no explanation, sure) and all the functions from the main/useful categories.
To tell you the truth, there are some that I don't know what they do either. Does that mean you're sure they are safe? This means that you should disable them by default (until some script breaks), not just write a question mark next to them ;)
Hory wrote: Expanding the feature set starting from nothing? I want a better solution that writing my own parser and interpreter. And if there isn't one - that's why I'm saying there's a gap.
What is the purpose of this code? What problem does it solve? There are languages that are better suited for sandboxing, I'm sure, but even if PHP was the prime choice, you still have to explain to yourself why you want to do this.
The purpose is to allow simple and safe variable-focused user-controlled scripting in web applications (in this case - games). Even if I managed to find a web host with an interpreter for another language that would be more appropriate, that language would have its share of dangerous functions. In trying to restrict them, I'd be back to what I'm doing with PHP right now.
You're missing the point. I think you should not need user-scripting for a game (or an app). Noone but a handful of geeks would be able to use it. And some of them will be better at security than you (and me).

Even if you do, you'll soon find that writing a parser for a simple language, specific to the tasks you want the users to control, will be both more simple and more secure. PHP has too many quirks in both its syntax and API. Here' s a test for you: I found that one of your allowed functions is not safe. Which one? In what way unsafe? Run a clock and see how long it takes you to find it. Now imagine a user of your script that wants to expand the allowed set -- how would he know what is safe? How long would it take to make sure, even if he has the expertise?

And now something that may offend you, but not intended as insult: I think that you need more time learning about security before starting writing security-related libraries. I personally wouldn't trust myself with such a library (I would maybe organize a team with wider experience or ask for peer reviews or something). I took the liberty of checking your other code, and it betrays that you're missing some basic knowledge. This is not to say that you should stop doing what you do! On the contrary, use it as a tool for your improvement -- read more articles on the basic vulnerabilities, read advisories and exploits to see what happens in the real world. Then go back and read your code :)

Cheers and good luck.
Post Reply