Recently the existance of the OWASP PHP Top 5 List was announced in several places. After I read it I was shocked about it's content and immediately asked OWASP who is responsible for it, and that I am shocked about it's content (especially the blatant advertisement in it).
The reply I got was very unprofessional and contained multiple indignities. In my answer I disproved several of the accusations and told them that after their mail I cannot take them serious anymore and that I am not interested in helping them anymore. They continued by writing dirty blog entries about me...
However, as usual it is my duty to protect the PHP community from getting harmed by their self-proclaimed teachers. I am not going to dissect the OWASP article line by line, although it needs a complete rewrite. I am simply not willing to waste my time. I will just present the most dangerous problem in the article. It should be enough to convince people that the article should be avoided. For the audience it is maybe interesting, that the article was, according to it's author reviewed by Amit Klein, Chris Shiflett, Laura Thomson and other security specialists from SANS.
It is up to my audience to decide what they think about these people after reading the rest of this blog entry...
The problem I want to elaborate here is placed in P3 of the OWASP article, where the topic SQL Injection
is discussed. The section starts with the following statement:
SQL injection is one of the oldest attacks against web applications. However, as it is well known, there are many techniques to defend against it
I fully agree with this sentence. I even agree with the list of methods that follows, although I cannot accept, that prepared statements are considered more secure than the use of mysql_real_escape_string(). If both techniques are used correctly they are both secure. (Unless there is a security vulnerability in their implementation).
So far so good... The section continues with some statements or personal opinions that you can agree or disagree... The article gets interesting however when OWASP describes how to defend against SQL Injection. They present two examples, one using prepared statements and one using just escaping and number conversion functions.
When I read it the first time, I was not prepared for the shock I got. I have seen a lot of very bad code during my years of auditing but I have never seen such a scary security hole in security teaching material before. (Well not true, I have disclosed similiar scary holes in the advertised sources of this article...)
Let's start with the SQL statement they wanted to protect. (The point of origin is not mentioned in the article).
$sql = "SELECT * FROM ". $_POST['DBHOST_TBL_PREFIX']. "themes WHERE themeid='$themeid'"; mysql_query($sql, $conn);
If you are unfamiliar with SQL Injection all you need to know here is that manipulation of the parameter $themeid or by changing the POST variable DBHOST_TBL_PREFIX it is possible to influence what SQL query is executed on the server. This can lead in this example to information disclosure. (Actually, beside the fact that it is a stupid idea to give the user the chance to influence the tablename at all, some SQL purists will also criticize the use of * instead of listing the required fields)
Now I will present the offical OWASP solutions to this problem to you. You will find both examples that follow in their PHP Top 5 List. Well until they get removed. Do not use them, they are dangerous.
Solution 1 - Escaping/Number Conversion
$sql = "SELECT * FROM “. mysql_real_escape_string($_POST['DBHOST_TBL_PREFIX']). ”themes WHERE themeid='”.intval($themeid)."'";
Solution 2 - Using prepared statements
$sql = "SELECT * FROM “. $mysqli->escape_string($_POST['DBHOST_TBL_PREFIX']). ”themes WHERE themeid=?”;
The educated eye will see, that $themeid is explicitly converted to a number in the first solution and bound as integer parameter to the prepared statement in the second solution. Additionally one can see, that the author tries to handle the more dangerous SQL Injection through the user supplied table prefix by escaping it with the database escaping function. Unfortunately that is based on a huge misunderstanding of the database escaping function. The database escaping functions are meant to escape data-strings in SQL queries. They are NOT meant for escaping f.e. SQL keywords, database or table names. And even if they were, all string escaping in SQL queries are worthless if the escaped data is not enclosed in apostroph characters (').
There is no function to convert user supplied data to valid table names. The best solution (if you really want user supplied tablenames... you dont want this, do you?) for the problem above would be a whitelist approach and only allow a restricted charset.
$sql = "SELECT * FROM “. preg_replace("/[^A-Za-z0-9_]/", "", $_POST['DBHOST_TBL_PREFIX']). ”themes WHERE themeid=?”;
The corrected solution is now SQL injection safe up to the degree that was intended by the query designer. Please keep in mind, that I strongly disagree with giving the user the right to choose what table prefix he wants through a POST variable. Depending on the database design, this could result in further information disclosure if it is possible for the attacker to specify a tablename he should not have access to. Additionally it is trivial to supply a name to a nonexisting table. This could trigger an SQL error, that depending on the configuration is printed into the output.
Finally I want to thank you for reading through this demonstration of blatant holes in security teaching material that is advertised as best practice by some people. Maybe this helps you to understand, why I strongly disagree with some of these people. If you have any further questions, then don't hesitate to contact me.