Merge pull request #110 from moufmouf/bugfix/109

SQL injection in OrderBy clause fix

doc/quickstart.md

@@ -551,6 +551,30 @@ class UserDao extends UserBaseDao {
 }
 ```
 
+**Important:** TDBM does its best to protect you from SQL injection. In particular, it will only allow column names in the "ORDER BY" clause. This means you are safe to pass input from the user directly in the ORDER BY parameter.
+
+```php
+// This is actually safe
+$resultSet = $this->find(null, [], $_GET['order']);
+```
+
+```php
+// This will throw an exception because only columns are allowed, not expressions
+$resultSet = $this->find(null, [], 'RAND()');
+```
+
+If you want to pass an expression to the ORDER BY clause, you will need to tell TDBM to stop checking for SQL injections. You do this by passing a `UncheckedOrderBy` object as a parameter:
+
+```php
+// This is actually OK. We tell TDBM that we know what we are doing.
+$resultSet = $this->find(null, [], new UncheckedOrderBy('RAND()'));
+```
+
+```php
+// This is the worst you can do, you are opening a SQL injection on the order query parameter.
+$resultSet = $this->find(null, [], new UncheckedOrderBy($_GET['order']));
+```
+
 
 Restricting results fetched using limits and offsets
 ----------------------------------------------------

src/Mouf/Database/TDBM/TDBMInvalidArgumentException.php

@@ -0,0 +1,7 @@
+<?php
+
+namespace Mouf\Database\TDBM;
+
+class TDBMInvalidArgumentException extends \InvalidArgumentException
+{
+}

src/Mouf/Database/TDBM/TDBMService.php

@@ -1298,19 +1298,19 @@ private function getTableGroupName(array $relatedTables)
      *
      * Finally, if filter_bag is null, the whole table is returned.
      *
-     * @param string            $mainTable             The name of the table queried
-     * @param string|array|null $filter                The SQL filters to apply to the query (the WHERE part). All columns must be prefixed by the table name (in the form: table.column)
-     * @param array             $parameters
-     * @param string|null       $orderString           The ORDER BY part of the query. All columns must be prefixed by the table name (in the form: table.column)
-     * @param array             $additionalTablesFetch
-     * @param int               $mode
-     * @param string            $className             Optional: The name of the class to instantiate. This class must extend the TDBMObject class. If none is specified, a TDBMObject instance will be returned
+     * @param string                       $mainTable             The name of the table queried
+     * @param string|array|null            $filter                The SQL filters to apply to the query (the WHERE part). Columns from tables different from $mainTable must be prefixed by the table name (in the form: table.column)
+     * @param array                        $parameters
+     * @param string|UncheckedOrderBy|null $orderString           The ORDER BY part of the query. Columns from tables different from $mainTable must be prefixed by the table name (in the form: table.column)
+     * @param array                        $additionalTablesFetch
+     * @param int                          $mode
+     * @param string                       $className             Optional: The name of the class to instantiate. This class must extend the TDBMObject class. If none is specified, a TDBMObject instance will be returned
      *
      * @return ResultIterator An object representing an array of results
      *
      * @throws TDBMException
      */
-    public function findObjects($mainTable, $filter = null, array $parameters = array(), $orderString = null, array $additionalTablesFetch = array(), $mode = null, $className = null)
+    public function findObjects(string $mainTable, $filter = null, array $parameters = array(), $orderString = null, array $additionalTablesFetch = array(), $mode = null, string $className = null)
     {
         // $mainTable is not secured in MagicJoin, let's add a bit of security to avoid SQL injection.
         if (!preg_match('/^[a-zA-Z_][a-zA-Z0-9_]*$/', $mainTable)) {
@@ -1321,7 +1321,7 @@ public function findObjects($mainTable, $filter = null, array $parameters = arra
 
         $parameters = array_merge($parameters, $additionalParameters);
 
-        list($columnDescList, $columnsList) = $this->getColumnsList($mainTable, $additionalTablesFetch, $orderString);
+        list($columnDescList, $columnsList, $orderString) = $this->getColumnsList($mainTable, $additionalTablesFetch, $orderString);
 
         $sql = 'SELECT DISTINCT '.implode(', ', $columnsList).' FROM MAGICJOIN('.$mainTable.')';
 
@@ -1352,19 +1352,19 @@ public function findObjects($mainTable, $filter = null, array $parameters = arra
     }
 
     /**
-     * @param string            $mainTable   The name of the table queried
-     * @param string            $from        The from sql statement
-     * @param string|array|null $filter      The SQL filters to apply to the query (the WHERE part). All columns must be prefixed by the table name (in the form: table.column)
-     * @param array             $parameters
-     * @param string|null       $orderString The ORDER BY part of the query. All columns must be prefixed by the table name (in the form: table.column)
-     * @param int               $mode
-     * @param string            $className   Optional: The name of the class to instantiate. This class must extend the TDBMObject class. If none is specified, a TDBMObject instance will be returned
+     * @param string                       $mainTable   The name of the table queried
+     * @param string                       $from        The from sql statement
+     * @param string|array|null            $filter      The SQL filters to apply to the query (the WHERE part). All columns must be prefixed by the table name (in the form: table.column)
+     * @param array                        $parameters
+     * @param string|UncheckedOrderBy|null $orderString The ORDER BY part of the query. All columns must be prefixed by the table name (in the form: table.column)
+     * @param int                          $mode
+     * @param string                       $className   Optional: The name of the class to instantiate. This class must extend the TDBMObject class. If none is specified, a TDBMObject instance will be returned
      *
      * @return ResultIterator An object representing an array of results
      *
      * @throws TDBMException
      */
-    public function findObjectsFromSql(string $mainTable, string $from, $filter = null, array $parameters = array(), string $orderString = null, $mode = null, string $className = null)
+    public function findObjectsFromSql(string $mainTable, string $from, $filter = null, array $parameters = array(), $orderString = null, $mode = null, string $className = null)
     {
         // $mainTable is not secured in MagicJoin, let's add a bit of security to avoid SQL injection.
         if (!preg_match('/^[a-zA-Z_][a-zA-Z0-9_]*$/', $mainTable)) {
@@ -1399,7 +1399,7 @@ public function findObjectsFromSql(string $mainTable, string $from, $filter = nu
         }, $columnDescList)).' FROM '.$from;
 
         if (count($allFetchedTables) > 1) {
-            list($columnDescList, $columnsList) = $this->getColumnsList($mainTable, [], $orderString);
+            list($columnDescList, $columnsList, $orderString) = $this->getColumnsList($mainTable, [], $orderString);
         }
 
         // Let's compute the COUNT.
@@ -1417,7 +1417,6 @@ public function findObjectsFromSql(string $mainTable, string $from, $filter = nu
 
         if (!empty($orderString)) {
             $sql .= ' ORDER BY '.$orderString;
-            $countSql .= ' ORDER BY '.$orderString;
         }
 
         if (stripos($countSql, 'GROUP BY') !== false) {
@@ -1465,15 +1464,15 @@ public function findObjectsFromSql(string $mainTable, string $from, $filter = nu
      *
      * Note: MySQL dictates that ORDER BYed columns should appear in the SELECT clause.
      *
-     * @param string      $mainTable
-     * @param array       $additionalTablesFetch
-     * @param string|null $orderBy
+     * @param string                       $mainTable
+     * @param array                        $additionalTablesFetch
+     * @param string|UncheckedOrderBy|null $orderBy
      *
      * @return array
      *
      * @throws \Doctrine\DBAL\Schema\SchemaException
      */
-    private function getColumnsList(string $mainTable, array $additionalTablesFetch = array(), string $orderBy = null)
+    private function getColumnsList(string $mainTable, array $additionalTablesFetch = array(), $orderBy = null)
     {
         // From the table name and the additional tables we want to fetch, let's build a list of all tables
         // that must be part of the select columns.
@@ -1488,25 +1487,47 @@ private function getColumnsList(string $mainTable, array $additionalTablesFetch
         $columnsList = [];
         $columnDescList = [];
         $sortColumn = 0;
+        $reconstructedOrderBy = null;
 
         // Now, let's deal with "order by columns"
         if ($orderBy !== null) {
+            if ($orderBy instanceof UncheckedOrderBy) {
+                $securedOrderBy = false;
+                $orderBy = $orderBy->getOrderBy();
+                $reconstructedOrderBy = $orderBy;
+            } else {
+                $securedOrderBy = true;
+                $reconstructedOrderBys = [];
+            }
             $orderByColumns = $this->orderByAnalyzer->analyzeOrderBy($orderBy);
 
             // If we sort by a column, there is a high chance we will fetch the bean containing this column.
             // Hence, we should add the table to the $additionalTablesFetch
             foreach ($orderByColumns as $orderByColumn) {
-                if ($orderByColumn['type'] === 'colref' && $orderByColumn['table'] !== null) {
-                    $additionalTablesFetch[] = $orderByColumn['table'];
+                if ($orderByColumn['type'] === 'colref') {
+                    if ($orderByColumn['table'] !== null) {
+                        $additionalTablesFetch[] = $orderByColumn['table'];
+                    }
+                    if ($securedOrderBy) {
+                        $reconstructedOrderBys[] = ($orderByColumn['table'] !== null ? $orderByColumn['table'].'.' : '').$orderByColumn['column'].' '.$orderByColumn['direction'];
+                    }
                 } elseif ($orderByColumn['type'] === 'expr') {
                     $sortColumnName = 'sort_column_'.$sortColumn;
                     $columnsList[] = $orderByColumn['expr'].' as '.$sortColumnName;
                     $columnDescList[] = [
                         'tableGroup' => null,
                     ];
                     ++$sortColumn;
+
+                    if ($securedOrderBy) {
+                        throw new TDBMInvalidArgumentException('Invalid ORDER BY column: "'.$orderByColumn['expr'].'". If you want to use expression in your ORDER BY clause, you must wrap them in a UncheckedOrderBy object. For instance: new UncheckedOrderBy("col1 + col2 DESC")');
+                    }
                 }
             }
+
+            if ($reconstructedOrderBy === null) {
+                $reconstructedOrderBy = implode(', ', $reconstructedOrderBys);
+            }
         }
 
         foreach ($additionalTablesFetch as $additionalTable) {
@@ -1539,7 +1560,7 @@ private function getColumnsList(string $mainTable, array $additionalTablesFetch
             }
         }
 
-        return [$columnDescList, $columnsList];
+        return [$columnDescList, $columnsList, $reconstructedOrderBy];
     }
 
     /**

src/Mouf/Database/TDBM/UncheckedOrderBy.php

@@ -0,0 +1,43 @@
+<?php
+
+namespace Mouf\Database\TDBM;
+
+/**
+ * Use this object to inject any SQL string in an order by clause in $dao->find methods.
+ *
+ * By default, TDBM is conservative and prevents an ORDERBY clause to be anything other than a sort on columns.
+ * This is done to prevent SQL injections.
+ *
+ * If you need to order on an expression, you can wrap your ORDERBY clause in this class.
+ *
+ * For instance:
+ *
+ * $this->find(null, null, new UncheckedOrderBy('RAND()'));
+ *
+ * Note: you understand that arguments passed inside the `UncheckedOrderBy` constructor are NOT protected and
+ * can be used for an SQL injection based attack. Therefore, you understand that you MUST NOT put input from the user
+ * in this constructor.
+ */
+class UncheckedOrderBy
+{
+    /**
+     * @var string
+     */
+    private $orderBy;
+
+    /**
+     * @param $orderBy
+     */
+    public function __construct(string $orderBy)
+    {
+        $this->orderBy = $orderBy;
+    }
+
+    /**
+     * @return string
+     */
+    public function getOrderBy() : string
+    {
+        return $this->orderBy;
+    }
+}

tests/Mouf/Database/TDBM/Dao/TestUserDao.php

@@ -3,6 +3,7 @@
 namespace Mouf\Database\TDBM\Dao;
 
 use Mouf\Database\TDBM\Test\Dao\Generated\UserBaseDao;
+use Mouf\Database\TDBM\UncheckedOrderBy;
 
 /**
  * The UserDao class will maintain the persistence of UserBean class into the users table.
@@ -76,6 +77,16 @@ public function getUsersByCountryName()
      * @return \Mouf\Database\TDBM\ResultIterator|\Mouf\Database\TDBM\Test\Dao\Bean\UserBean[]|\Mouf\Database\TDBM\Test\Dao\Generated\ResultArray
      */
     public function getUsersByReversedCountryName()
+    {
+        return $this->find(null, [], new UncheckedOrderBy('REVERSE(country.label) ASC'));
+    }
+
+    /**
+     * A test to check exceptions when providing expressions in ORDER BY clause.
+     *
+     * @return \Mouf\Database\TDBM\ResultIterator|\Mouf\Database\TDBM\Test\Dao\Bean\UserBean[]|\Mouf\Database\TDBM\Test\Dao\Generated\ResultArray
+     */
+    public function getUsersByInvalidOrderBy()
     {
         return $this->find(null, [], 'REVERSE(country.label) ASC');
     }

tests/Mouf/Database/TDBM/TDBMDaoGeneratorTest.php

@@ -1082,4 +1082,14 @@ public function testOrderByExpression()
 
         $this->assertEquals('Jamaica', $users[0]->getCountry()->getLabel());
     }
+
+    /**
+     * @depends testDaoGeneration
+     */
+    public function testOrderByException()
+    {
+        $userDao = new TestUserDao($this->tdbmService);
+        $this->expectException(TDBMInvalidArgumentException::class);
+        $users = $userDao->getUsersByInvalidOrderBy();
+    }
 }