Issue #829464 by Berdir, klausi, sepgil | Heine: Fixed orderby() should verify that the sort direction is always ASC or DESC.

CHANGELOG.txt

@@ -1,6 +1,9 @@
 
 Drupal 7.33, xxxx-xx-xx (development version)
 -----------------------
+- Security improvement: Made the database API's orderBy() method sanitize the
+  sort direction ("ASC" or "DESC") for queries built with db_select(), so that
+  calling code does not have to.
 - Changed the RDF module to consistently output RDF metadata for nodes and
   comments near where the node is rendered in the HTML (minor markup and data
   structure change).

includes/database/select.inc

@@ -377,7 +377,8 @@ interface SelectQueryInterface extends QueryConditionInterface, QueryAlterableIn
    * @param $field
    *   The field on which to order.
    * @param $direction
-   *   The direction to sort. Legal values are "ASC" and "DESC".
+   *   The direction to sort. Legal values are "ASC" and "DESC". Any other value
+   *   will be converted to "ASC".
    * @return SelectQueryInterface
    *   The called object.
    */
@@ -1384,6 +1385,8 @@ class SelectQuery extends Query implements SelectQueryInterface {
   }
 
   public function orderBy($field, $direction = 'ASC') {
+    // Only allow ASC and DESC, default to ASC.
+    $direction = strtoupper($direction) == 'DESC' ? 'DESC' : 'ASC';
     $this->order[$field] = $direction;
     return $this;
   }

includes/tablesort.inc

@@ -46,10 +46,9 @@ class TableSort extends SelectQueryExtender {
       // Based on code from db_escape_table(), but this can also contain a dot.
       $field = preg_replace('/[^A-Za-z0-9_.]+/', '', $ts['sql']);
 
-      // Sort order can only be ASC or DESC.
+      // orderBy() will ensure that only ASC/DESC values are accepted, so we
-      $sort = drupal_strtoupper($ts['sort']);
+      // don't need to sanitize that here.
-      $sort = in_array($sort, array('ASC', 'DESC')) ? $sort : '';
+      $this->orderBy($field, $ts['sort']);
-      $this->orderBy($field, $sort);
     }
     return $this;
   }

modules/simpletest/tests/database_test.test

@@ -1947,6 +1947,15 @@ class DatabaseSelectOrderedTestCase extends DatabaseTestCase {
 
     $this->assertEqual($num_records, 4, 'Returned the correct number of rows.');
   }
+
+  /**
+   * Tests that the sort direction is sanitized properly.
+   */
+  function testOrderByEscaping() {
+    $query = db_select('test')->orderBy('name', 'invalid direction');
+    $order_bys = $query->getOrderBy();
+    $this->assertEqual($order_bys['name'], 'ASC', 'Invalid order by direction is converted to ASC.');
+  }
 }
 
 /**