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.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Johan
  • 74,508
  • 24
  • 191
  • 319
  • Can someone please post sample code how to fix this SQL-injection hole? – Johan Apr 27 '11 at 23:36
  • @nikic I see where you're going, but it does not look foolproof :-) – Johan Apr 27 '11 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. – NikiC Apr 27 '11 at 23:50

3 Answers3

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.

Community
  • 1
  • 1
Pekka
  • 442,112
  • 142
  • 972
  • 1,088
  • 6
    +1 Excellent answer. I certainly leaned something new today! – Endophage Apr 27 '11 at 23:21
  • 3
    +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 Apr 27 '11 at 23:28
  • @nikic yeah! This is definitely a shortcoming of PDO. I don't understand it either. – Pekka Apr 27 '11 at 23:28
  • 1
    +1 for the SHOW TABLES... You can cache the result to avoid calling it each time. – Coyote Oct 03 '11 at 11:53
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`";
}
Johan
  • 74,508
  • 24
  • 191
  • 319
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.

NikiC
  • 100,734
  • 37
  • 191
  • 225
  • 1
    @nikic, this will not prevent using `table\`\` union select x,y from mysql.user` as a tablename – Johan May 31 '11 at 16:58
  • @Johan: Why not? It will be inserted as: `... FROM \`table\`\`\`\` union select x,y from mysql.user\` ...` – NikiC May 31 '11 at 17:32
  • so leave out the backticks and inject `table` union select x,y from mysql.user` – Johan May 31 '11 at 18:31
  • @Johan: What is ``` 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 May 31 '11 at 20:13
  • 1
    @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. – Johan May 31 '11 at 20:36
  • @Johan: I already wrote in my answer that this approach probably isn't encoding-safe. But I don't know enough about different encodings in order to give you a definite answer here. If the encoding of the database and connection is known to be, say UTF-8, this may indeed be a safe way to do things. If you don't have character set information, then there obviously arise problems ;) – NikiC Jun 01 '11 at 05:18