SQL injection fix: Coerce offset and limit values to integers (#1054)

* Use HTTPS repository URL for PEAR Fixes this installation error: PEAR repository from http://pear.php.net could not be loaded. Your configuration does not allow connections to http://pear.php.net/channel.xml. See https://getcomposer.org/doc/06-config.md#secure-http for details. * Fix fatal error in some versions of PHP `$buildScopeVars` is initialized as a string but later appended to as an array via `$buildScopeVars[] = …`. This throws a fatal error in some versions of PHP. The fix was to re-initialize it to an array before appending values. * Include PHPUnit in dev dependencies This avoids having to download it separately, especially since the tests only work with PHPUnit < 6 * Cast limit to integer when setting via Criteria::setLimit() This prevents SQL injection via limit() and is a similar fix as the one added to Propel2 [1]. See that pull request for details. [1] propelorm/Propel2#1465 * Coerce offset and limit values to integers for MySQL LIMIT clause When constructing a MySQL LIMIT clause, values for the offset and limit are coerced to integers. This prevents arbitrary SQL from being injected via a query limit. Example: UserQuery::create()->limit('1;DROP TABLE users')->find(); Previously, this would have injected `DROP TABLE users` into the generated SQL. Now, the limit value would be coerced to the integer `1`. This is similar to the fix for Propel2 [1]. Fixes #1052 [1] propelorm/Propel2#1464 * Update Travis configuration to use installed phpunit From dev dependencies. * Update Travis config to support PHP 5.3 correctly Per https://docs.travis-ci.com/user/reference/trusty#PHP-images * Fix encoding issues in tests h/t @smhg
propelorm · Nov 4, 2019 · b720932 · b720932
1 parent 3f7a284
commit b720932
Show file tree

Hide file tree

Showing 8 changed files with 350 additions and 11 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -7,7 +7,6 @@ branches:
     - gh-pages
 
 php:
-  - 5.3
   - 5.4
   - 5.5
   - hhvm
@@ -26,8 +25,11 @@ before_script:
 
   - ./test/reset_tests.sh
 
-script: phpunit
+script: vendor/bin/phpunit
 
 matrix:
+  include:
+    - php: 5.3
+      dist: precise
   allow_failures:
     - php: hhvm
diff --git a/composer.json b/composer.json
@@ -18,7 +18,8 @@
     "phing/phing": "~2.4"
   },
   "require-dev": {
-    "pear-pear.php.net/PEAR_PackageFileManager2": "@stable"
+    "pear-pear.php.net/PEAR_PackageFileManager2": "@stable",
+    "phpunit/phpunit": "~4.0||~5.0"
   },
   "extra": {
     "branch-alias": {
@@ -28,7 +29,7 @@
   "repositories": [
     {
       "type": "pear",
-      "url": "http://pear.php.net"
+      "url": "https://pear.php.net"
     }
   ],
   "bin": ["generator/bin/propel-gen", "generator/bin/propel-gen.bat"]

diff --git a/generator/lib/behavior/sortable/SortableBehavior.php b/generator/lib/behavior/sortable/SortableBehavior.php
@@ -96,6 +96,7 @@ public function generateScopePhp()
 
             $methodSignature = array();
             $buildScope      = array();
+            $buildScopeVars  = array();
             $paramsDoc       = array();
 
             foreach ($this->getScopes() as $idx => $scope) {

diff --git a/runtime/lib/adapter/DBMySQL.php b/runtime/lib/adapter/DBMySQL.php
@@ -153,6 +153,9 @@ public function useQuoteIdentifier()
      */
     public function applyLimit(&$sql, $offset, $limit)
     {
+        $offset = (int) $offset;
+        $limit = (int) $limit;
+
         if ($limit > 0) {
             $sql .= " LIMIT " . ($offset > 0 ? $offset . ", " : "") . $limit;
         } elseif ($offset > 0) {

diff --git a/runtime/lib/query/Criteria.php b/runtime/lib/query/Criteria.php
@@ -1239,8 +1239,7 @@ public function isSingleRecord()
      */
     public function setLimit($limit)
     {
-        // TODO: do we enforce int here? 32bit issue if we do
-        $this->limit = $limit;
+        $this->limit = (int) $limit;
 
         return $this;
     }
@@ -1258,8 +1257,7 @@ public function getLimit()
     /**
      * Set offset.
      *
-     * @param int $offset An int with the value for offset.  (Note this values is
-     *                    cast to a 32bit integer and may result in truncation)
+     * @param int $offset An int with the value for offset.
      *
      * @return Criteria Modified Criteria object (for fluent API)
      */

diff --git a/test/fixtures/reverse/mysql/build/sql/schema.sql b/test/fixtures/reverse/mysql/build/sql/schema.sql
@@ -1,3 +1,5 @@
+SET NAMES utf8;
+
 DROP TABLE IF EXISTS book;
 DROP VIEW IF EXISTS view_book_titles;
 

diff --git a/test/testsuite/runtime/adapter/DBMySQLTest.php b/test/testsuite/runtime/adapter/DBMySQLTest.php
@@ -104,6 +104,194 @@ public function testQuotingIdentifiers()
         $db = new DBMySQL();
         $this->assertEquals('`Book ISBN`', $db->quoteIdentifier('Book ISBN'));
     }
+
+    /**
+     * @dataProvider dataApplyLimit
+     */
+    public function testApplyLimit($offset, $limit, $expectedSql)
+    {
+        $sql = '';
+
+        $db = new DBMySQL();
+        $db->applyLimit($sql, $offset, $limit);
+
+        $this->assertEquals($expectedSql, $sql, 'Generated SQL does not match expected SQL');
+    }
+
+    public function dataApplyLimit()
+    {
+        return array(
+
+            /*
+                Offset & limit = 0
+             */
+
+            'Zero offset & limit' => array(
+                'offset'      => 0,
+                'limit'       => 0,
+                'expectedSql' => ''
+            ),
+
+            /*
+                Offset = 0
+             */
+
+            '32-bit limit' => array(
+                'offset'      => 0,
+                'limit'       => 4294967295,
+                'expectedSql' => ' LIMIT 4294967295'
+            ),
+            '32-bit limit as a string' => array(
+                'offset'      => 0,
+                'limit'       => '4294967295',
+                'expectedSql' => ' LIMIT 4294967295'
+            ),
+
+            '64-bit limit' => array(
+                'offset'      => 0,
+                'limit'       => 9223372036854775807,
+                'expectedSql' => ' LIMIT 9223372036854775807'
+            ),
+            '64-bit limit as a string' => array(
+                'offset'      => 0,
+                'limit'       => '9223372036854775807',
+                'expectedSql' => ' LIMIT 9223372036854775807'
+            ),
+
+            'Float limit' => array(
+                'offset'      => 0,
+                'limit'       => 123.9,
+                'expectedSql' => ' LIMIT 123'
+            ),
+            'Float limit as a string' => array(
+                'offset'      => 0,
+                'limit'       => '123.9',
+                'expectedSql' => ' LIMIT 123'
+            ),
+
+            'Negative limit' => array(
+                'offset'      => 0,
+                'limit'       => -1,
+                'expectedSql' => ''
+            ),
+            'Non-numeric string limit' => array(
+                'offset'      => 0,
+                'limit'       => 'foo',
+                'expectedSql' => ''
+            ),
+            'SQL injected limit' => array(
+                'offset'      => 0,
+                'limit'       => '3;DROP TABLE abc',
+                'expectedSql' => ' LIMIT 3'
+            ),
+
+            /*
+                Limit = 0
+             */
+
+            '32-bit offset' => array(
+                'offset'      => 4294967295,
+                'limit'       => 0,
+                'expectedSql' => ' LIMIT 4294967295, 18446744073709551615'
+            ),
+            '32-bit offset as a string' => array(
+                'offset'      => '4294967295',
+                'limit'       => 0,
+                'expectedSql' => ' LIMIT 4294967295, 18446744073709551615'
+            ),
+
+            '64-bit offset' => array(
+                'offset'      => 9223372036854775807,
+                'limit'       => 0,
+                'expectedSql' => ' LIMIT 9223372036854775807, 18446744073709551615'
+            ),
+            '64-bit offset as a string' => array(
+                'offset'      => '9223372036854775807',
+                'limit'       => 0,
+                'expectedSql' => ' LIMIT 9223372036854775807, 18446744073709551615'
+            ),
+
+            'Float offset' => array(
+                'offset'      => 123.9,
+                'limit'       => 0,
+                'expectedSql' => ' LIMIT 123, 18446744073709551615'
+            ),
+            'Float offset as a string' => array(
+                'offset'      => '123.9',
+                'limit'       => 0,
+                'expectedSql' => ' LIMIT 123, 18446744073709551615'
+            ),
+
+            'Negative offset' => array(
+                'offset'      => -1,
+                'limit'       => 0,
+                'expectedSql' => ''
+            ),
+            'Non-numeric string offset' => array(
+                'offset'      => 'foo',
+                'limit'       => 0,
+                'expectedSql' => ''
+            ),
+            'SQL injected offset' => array(
+                'offset'      => '3;DROP TABLE abc',
+                'limit'       => 0,
+                'expectedSql' => ' LIMIT 3, 18446744073709551615'
+            ),
+
+            /*
+                Offset & limit != 0
+             */
+
+            array(
+                'offset'      => 4294967295,
+                'limit'       => 999,
+                'expectedSql' => ' LIMIT 4294967295, 999'
+            ),
+            array(
+                'offset'      => '4294967295',
+                'limit'       => 999,
+                'expectedSql' => ' LIMIT 4294967295, 999'
+            ),
+
+            array(
+                'offset'      => 9223372036854775807,
+                'limit'       => 999,
+                'expectedSql' => ' LIMIT 9223372036854775807, 999'
+            ),
+            array(
+                'offset'      => '9223372036854775807',
+                'limit'       => 999,
+                'expectedSql' => ' LIMIT 9223372036854775807, 999'
+            ),
+
+            array(
+                'offset'      => 123.9,
+                'limit'       => 999,
+                'expectedSql' => ' LIMIT 123, 999'
+            ),
+            array(
+                'offset'      => '123.9',
+                'limit'       => 999,
+                'expectedSql' => ' LIMIT 123, 999'
+            ),
+
+            array(
+                'offset'      => -1,
+                'limit'       => 999,
+                'expectedSql' => ' LIMIT 999'
+            ),
+            array(
+                'offset'      => 'foo',
+                'limit'       => 999,
+                'expectedSql' => ' LIMIT 999'
+            ),
+            array(
+                'offset'      => '3;DROP TABLE abc',
+                'limit'       => 999,
+                'expectedSql' => ' LIMIT 3, 999'
+            ),
+        );
+    }
 }
 
 // See: http://stackoverflow.com/questions/3138946/mocking-the-pdo-object-using-phpunit