21

I had this discussion with a high reputation PHP guy:

PDO has no use here. as well as mysql_real_escape_string. extremely poor quality.

This of course is cool, but I honestly don't know what's wrong with suggesting use of mysql_real_escape_string or PDO to fix this code:

<script type="text/javascript">
    var layer;

    window.location.href = "example3.php?layer="+ layer;

    <?php
        //Make a MySQL connection
        $query = "SELECT Category, COUNT(BUSNAME)
          FROM ".$_GET['layer']." GROUP BY Category";
        $result = mysql_query($query) or die(mysql_error());

Into this

$layer = mysql_real_escape_string($_GET['layer']);
$query = "SELECT Category, COUNT(BUSNAME)
FROM `".$layer."` GROUP BY Category";

, considering that the JavaScript code gets send client-side.

Improve this question
3
  • Can someone please post sample code how to fix this SQL-injection hole? Commented Apr 27, 2011 at 23:36
  • @nikic I see where you're going, but it does not look foolproof :-) Commented Apr 27, 2011 at 23:45
  • Yeah, I don't think it's foolproof either. The problem I see is this encoding related stuff, as I mentioned in my answer below. But I have no idea how these encoding based hacks work and thus don't know, how to prevent them. Commented Apr 27, 2011 at 23:50

3 Answers 3

Reset to default
41

Your advice is indeed incorrect.

mysql_real_escape_string() will not work for dynamic table names; it is designed to escape string data, delimited by quotes, only. It will not escape the backtick character. It's a small but crucial distinction.

So I could insert a SQL injection in this, I would just have to use a closing backtick.

PDO does not provide sanitation for dynamic table names, either.

This is why it is good not to use dynamic table names, or if one has to, comparing them against a list of valid values, like a list of tables from a SHOW TABLES command.

I wasn't really fully aware of this either, and probably guilty of repeating the same bad advice, until it was pointed out to me here on SO, also by Col. Shrapnel.

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

4 Comments

+1 Excellent answer. I certainly leaned something new today!
+1 This is something I have for long time missed in PDO. A PDO->quote(..., PDO::PARAM_IDENTIFIER) or something like that. This often diminished PDOs cross-DB approach, because different DBs have different quotation and escaping for that. In MySQL you most oftenly use the backtick and " is only an identifier quote in ANSI mode, but in other DBMS on the other hand " is the way to go. Hum, hum, hum :)
@nikic yeah! This is definitely a shortcoming of PDO. I don't understand it either.
+1 for the SHOW TABLES... You can cache the result to avoid calling it each time.
5

For the record, here's sample code for fixing this hole.

$allowed_tables = array('table1', 'table2');
$clas = $_POST['clas'];
if (in_array($clas, $allowed_tables)) {
    $query = "SELECT * FROM `$clas`";
}
Improve this answer

1 Comment

An explanation would be in order.
2

In order to answer how to actually fix the code:

'...FROM `' . str_replace('`', '``', $tableName) . '`...'

This duplicates all backticks in the table name (this is how escaping in MySQL is done).

One thing I'm not sure about, is whether this is "encoding-safe" (how does one call it correctly?). One typically recommends mysql_real_escape_string instead of addslashes, because the former takes the encoding of the MySQL connection into account. Maybe this problem applies here, too.

Improve this answer

6 Comments

@nikic, this will not prevent using table`` union select x,y from mysql.user as a tablename
@Johan: Why not? It will be inserted as: ... FROM `table```` union select x,y from mysql.user` ...
so leave out the backticks and inject table&#96; union select x,y from mysql.user
@Johan: What is &#96; supposed to mean? If that is a HTML entity equivalent to the backtick, then: HTML Entities work just in HTML, not in SQL. In SQL those are just normal characters.
@nikic, I know you want to pretend your code is secure, but trust me it isn't, depending on the charset/collation of your database connection metachars like the one above will get translated into special chars like backticks. The only way to be secure is to check the tablename/fieldname/columnname against a list of pre-approved names. If you don't believe me, ask @Pekka.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.