Is there anything that can be put after the "ORDER BY" clause that can pose a security risk?

Question

Basically, what I want to do is this:

mysql_query("SELECT ... FROM ... ORDER BY $_GET[order]")

They can obviously easily create a SQL error by putting non-sense in there, but mysql_query only allows you to execute 1 query, so they can't put something like 1; DROP TABLE ....

Is there any damage a malicious user could do, other than creating a syntax error?

If so, how can I sanitize the query?

There's a lot of logic built on the $_GET['order'] variable being in SQL-like syntax, so I really don't want to change the format.

---

To clarify, $_GET['order'] won't just be a single field/column. It might be something like last_name DESC, first_name ASC.

Answer 1

Yes, SQL injection attacks can use an unescaped ORDER BY clause as a vector. There's an explanation of how this can be exploited and how to avoid this problem here:

http://josephkeeler.com/2009/05/php-security-sql-injection-in-order-by/

That blog post recommends using a white list to validate the ORDER BY parameter against, which is almost certainly the safest approach.

---

To respond to the update, even if the clause is complex, you can still write a routine that validates it against a whitelist, for example:

function validate_order_by($order_by_parameter) {
    $columns = array('first_name', 'last_name', 'zip', 'created_at');

    $parts = preg_split("/[\s,]+/", $order_by_parameter);

    foreach ($parts as $part) {
        $subparts = preg_split("/\s+/", $part);

        if (count($subparts) < 0 || count($subparts) > 2) {
           // Too many or too few parts.
           return false;
        }

        if (!in_array($subparts[0], $columns)) {
           // Column name is invalid.
           return false;
        }

        if (count($subparts) == 2 
            && !in_array(strtoupper($subparts[1]), array('ASC', 'DESC')) {
          // ASC or DESC is invalid
          return false;
        }
    }

    return true;
}

Even if the ORDER BY clause is complex, it's still made only out of values you supply (assuming you're not letting users edit it by hand). You can still validate using a white list.

I should also add that I normally don't like to expose my database structure in URLs or other places in the UI and will often alias the stuff in the parameters in the URLs and map it to the real values using a hash.

Answer 2

Don't count on the fact that a SQL injection at that point won't currently cause a problem; don't allow ANY SQL injection. If nothing else, a malicious attacker could define a very complex order that could cause serious slowdown of your DB.

Answer 3

I prefer to do whitelisting and treat the http parameter string as separate from the string that gets interpolated into the SQL query.

In the following PHP example, the array keys would be the values passed in as http parameters, some kind of symbolic label for different ordering schemes, according to your web interface. The array values would be what we want to interpolate into SQL for these corresponding ordering schemes, e.g. column names or expressions.

<?php

$orderby_whitelist = array(
  "name"    => "last_name, first_name",
  "date"    => "date_created",
  "daterev" => "date_created DESC",
  "DEFAULT" => "id"
);

$order = isset($_GET["order"]) 
  ? $_GET["order"] 
  : "DEFAULT";
$order_expr = array_key_exists($order, $orderby_whitelist) 
  ? $orderby_whitelist[$order] 
  : $orderby_whitelist["DEFAULT"];

mysql_query("SELECT ... FROM ... ORDER BY $order_expr")

This has advantages:

• You can defend against SQL injection even for cases where you can't use query parameters. If the client passes an unrecognized value, your code ignores it and uses a default order.

• You don't have to sanitize anything, because the keys and values in the array are both written by you, the programmer. Client input can only pick one of the choices you allow.

• Your web interface does not reveal your database structure.

• You can make custom orders that correspond to SQL expressions or alternative ASC/DESC, as I showed above.

• You can change database structure without changing your web interface, or vice versa.

I cover this solution in my presentation, SQL Injection Myths & Fallacies, and also in my book, SQL Antipatterns Volume 1: Avoiding the Pitfalls of Database Programming.

Answer 4

The logic built around the variable $_GET['order'] should be just as valid when used on a variable $order_sanitized. Why not just sanitize the input first and foremost, and then perform your logic to make it fit into the SELECT statement?

Answer 5

It still is a point of potential SQL injection. You are depending on the internal implementation of mysql_query. What if in a later version they change it to use multiple queries?

You can use mysql_real_escape_string.