CODEWITHSUNDEEP
phpsql-injection
Johan
Johan

Reputation: 76703

How can I prevent SQL injection with dynamic tablenames?

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.

Upvotes: 21

Views: 4842

Answers (3)

Johan
Johan

Reputation: 76703

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`";
}

Upvotes: 5

NikiC
NikiC

Reputation: 101936

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.

Upvotes: 2

Pekka
Pekka

Reputation: 449613

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.

Upvotes: 41

Related Questions