PHP Logic Flaws.

Today I want to show you a vulnerability[1] found in IceBB by GiReX which was submitted to milw0rm yesterday. It is exactly such vulnerability that happens when programmers trying to invent their own security mechanisms without understanding all the pitfalls it can create. I thought it would be interesting to see how programmers are thinking and learning. Code block 1, that comes from IceBB, contains some interesting thought flow that led to SQL injection, exactly what the programmer wanted to prevent against.

94. function clean_string($v)


{


if(get_magic_quotes_gpc())


{


$v = stripslashes($v); <= magic quotes is OFF


}





//$v = htmlentities($v,ENT_QUOTES,'UTF-8'); <= first attempt


$v = htmlspecialchars($v,ENT_QUOTES);


$v = preg_replace("/&#0*([0-9]*);?/",'&#\1;',$v); <= new attempt





return $v;


106. }

When magic quotes are enabled, they first strip the slashes that are in the user supplied data. When the data is void from slashes, the code first attempt was to use htmlentities to prevent against SQL injection. Actually, one should use character entities encoding only on outputting data instead upon inserting data. Later on, it seems that this part was commented out for some reason, which led to the use of htmlspecialchars with the ENT_QUOTES constant that makes sure that all quotes gets translated to special chars. Then the data goes through a regular expression that places all data inside two single quotes.

So far so good? not quite. The problem lies in the understanding that backslashes can be used in MySQL to escape a character. In our case, we can insert a single backslash that escapes the last quote that was passed through preg_replace. This means that we now have an entry to inject a new SQL query due to the insertion of our backslash. Now, the general approach is to use mysql_real_escape_string() function, because it escapes all potential dangerous characters.

This is what happened in IceBB:

# "SELECT COUNT(*) as total FROM icebb_posts WHERE pauthor_id='{$icebb->input['author']}'"





# Setting author= in this query:


# "SELECT COUNT(*) as total FROM icebb_posts WHERE pauthor_id='' "

Since it generates an error, we can't do anything with it. But, here comes some creativity. When we use two parameters to inject, we can re-create a proper but injected query.

For example:

# GET /index.php?act=members&username=a&url=OR+1#

Became:

# "SELECT COUNT(*) as total FROM icebb_users WHERE user_group='a' AND username='OR 1#' AND id!=0 ORDER BY username ASC"

It probably looked quite safe to the programmer who wrote it, but again it shows that programmers start programming without even considering such risks. While this is a logic flaw, it also is due to bad programming behavior. The reason for this is that programmers must learn not to invent their own security mechanisms. It sounds tempting, but it always turned out bad. Before this vulnerability emerged, I posted on the Synapse Wiki another logic flaw that I found a couple of times. In Synapse we aim to find such logic flaws.

Take a look at the code below, and try to figure out what is wrong with it:

str_replace(" ' ", " '' ",$value);

Do you see a problem?

The str_replace function here assumes that a single quote must precede a space. But if we only enter a single quote without a preceding space, the function cannot replace it because it cannot find the pattern that is given. For example:

$value = str_replace(" ' ", " '' ",$value);


echo "select * from foo where id = ' ".$value." ' ";

If we enter the below query part without a preceding space, it will successfully inject our new SQL query.

'OR 1=1--

These vulnerabilities might seem simple, but I saw them many times when I performed source code review. I hope this sheds some light on simple but dangerous vulnerabilities, they can be prevented by programmers that are focused on secure programming. Granted, it takes some time to understand it for some, but it is worth it. Because, somewhere down the road it will be exploited by someone with more time than you have to program the code. If you consider to write code that is released for free and to a great audience, please remember that you have to make sure that it is secure for those who use it. There is simply no excuse to make such mistakes, especially when your code previously had SQL injection vulnerabilities and proposed the above code as your fix. Everyone makes mistakes, over time I had my fair share also. But that doesn't mean that one shouldn't stay wary of these things.

[1] http://www.milw0rm.com/exploits/6137