Sanitize and prevent SQLi in getObject calls expecting PK values

Also make isValidClause method static in xPDOQuery
modxcms · Dec 16, 2016 · 305f2ba75c09a396e33a3b8508f0110524e28301 · 305f2ba
1 parent 028cb50
commit 305f2ba75c09a396e33a3b8508f0110524e28301
Unified Split

Showing with 116 additions and 14 deletions.

+59 −0 test/xPDO/xPDO/xPDOObject.php

+2 −0 xpdo/changelog.txt

+1 −1 xpdo/om/xpdoobject.class.php

+22 −13 xpdo/om/xpdoquery.class.php

+32 −0 xpdo/xpdo.class.php
diff --git a/test/xPDO/xPDO/xPDOObject.php b/test/xPDO/xPDO/xPDOObject.php
@@ -296,6 +296,65 @@ public function testGetObjectByPK() {
        $this->assertTrue($result, "Error retrieving object by primary key");
    }
+  
+    /**
+  
+     * Test that an object can only be retrieved by primary key if type matches.
+  
+     *
+  
+     * @param string $class
+  
+     * @param mixed  $pkValue
+  
+     *
+  
+     * @depends testSaveObject
+  
+     * @dataProvider providerGetObjectByPKFailsOnTypeMismatch
+  
+     */
+  
+    public function testGetObjectByPKFailsOnTypeMismatch($class, $pkValue) {
+  
+        if (!empty(xPDOTestHarness::$debug)) print "\n" . __METHOD__ . " = ";
+  
+        $result = null;
+  
+        try {
+  
+            $result = $this->xpdo->getObject($class, $pkValue);
+  
+        } catch (Exception $e) {
+  
+            $this->xpdo->log(xPDO::LOG_LEVEL_ERROR, $e->getMessage(), '', __METHOD__, __FILE__, __LINE__);
+  
+        }
+  
+        $this->assertNull($result, "Object retrieved with invalid primary key type");
+  
+    }
+  
+    public function providerGetObjectByPKFailsOnTypeMismatch() {
+  
+        return array(
+  
+            array('Person', "stupid"),
+  
+            array('Phone', "crazy"),
+  
+            array('PersonPhone', "farfetched"),
+  
+            array('PersonPhone', 1),
+  
+            array('PersonPhone', "don't do it"),
+  
+            array('BloodType', 1),
+  
+        );
+  
+    }
+  
+
+  
+    /**
+  
+     * Test that an object can only be retrieved by primary key if SQL injection detected.
+  
+     *
+  
+     * @param string $class
+  
+     * @param mixed  $pkValue
+  
+     *
+  
+     * @depends testSaveObject
+  
+     * @dataProvider providerGetObjectByPKFailsOnSQLInjection
+  
+     */
+  
+    public function testGetObjectByPKFailsOnSQLInjection($class, $pkValue) {
+  
+        if (!empty(xPDOTestHarness::$debug)) print "\n" . __METHOD__ . " = ";
+  
+        $result = null;
+  
+        try {
+  
+            $result = $this->xpdo->getObject($class, $pkValue);
+  
+        } catch (Exception $e) {
+  
+            $this->xpdo->log(xPDO::LOG_LEVEL_ERROR, $e->getMessage(), '', __METHOD__, __FILE__, __LINE__);
+  
+        }
+  
+        $this->assertNull($result, "Object retrieved with SQL injection detected");
+  
+    }
+  
+    public function providerGetObjectByPKFailsOnSQLInjection() {
+  
+        return array(
+  
+            array('Person', "1;DROP TABLE `person`"),
+  
+            array('Phone', "1 UNION SELECT * FROM `phone` WHERE id = 2"),
+  
+            array('PersonPhone', array("1=1;DROP TABLE `person`")),
+  
+            array('BloodType', "AB+ UNION SELECT * FROM `blood_type` WHERE `type` = 'A'"),
+  
+            array('BloodType', "AB+/**/UNION SELECT * FROM `blood_type` WHERE `type` = 'A'"),
+  
+        );
+  
+    }
+  
+
    /**
     * Test using getObject by PK on multiple objects, including multiple PKs
     */

diff --git a/xpdo/changelog.txt b/xpdo/changelog.txt
@@ -1,5 +1,7 @@
 This file shows the changes in this release of xPDO.
+  
+- Sanitize and prevent SQLi in getObject calls expecting PK values
+  
+- Make isValidClause a public static method in xPDOQuery
 - Allow empty sort direction
 - Revert getCriteria change to force scalar params to be PK values
 - Make xPDOQuery::isValidClause a public method

diff --git a/xpdo/om/xpdoobject.class.php b/xpdo/om/xpdoobject.class.php
@@ -412,7 +412,7 @@ public static function load(xPDO & $xpdo, $className, $criteria, $cacheFlag= tru
        $fromCache= false;
        if ($className= $xpdo->loadClass($className)) {
            if (!is_object($criteria)) {
-  
-                $criteria= $xpdo->getCriteria($className, $criteria, $cacheFlag);
+  
+                $criteria = $xpdo->getCriteria($className, $criteria, $cacheFlag);
            }
            if (is_object($criteria)) {
                $criteria = $xpdo->addDerivativeCriteria($className, $criteria);

diff --git a/xpdo/om/xpdoquery.class.php b/xpdo/om/xpdoquery.class.php
@@ -102,6 +102,22 @@ abstract class xPDOQuery extends xPDOCriteria {
        'limit' => '',
    );
+  
+    /**
+  
+     * Make sure a clause is valid and does not contain SQL injection attempts.
+  
+     *
+  
+     * @param string $clause The string clause to validate.
+  
+     *
+  
+     * @return bool True if the clause is valid.
+  
+     */
+  
+    public static function isValidClause($clause) {
+  
+        $output = rtrim($clause, ' ;');
+  
+        $output = preg_replace("/\\\\'.*?\\\\'/", '{mask}', $output);
+  
+        $output = preg_replace('/\\".*?\\"/', '{mask}', $output);
+  
+        $output = preg_replace("/'.*?'/", '{mask}', $output);
+  
+        $output = preg_replace('/".*?"/', '{mask}', $output);
+  
+        return strpos($output, ';') === false && strpos(strtolower($output), 'union') === false;
+  
+    }
+  
+
    public function __construct(& $xpdo, $class, $criteria= null) {
        parent :: __construct($xpdo);
        if ($class= $this->xpdo->loadClass($class)) {
@@ -404,7 +420,7 @@ public function sortby($column, $direction= 'ASC') {
            $direction = '';
        }
-  
-        if (!$this->isValidClause($column)) {
+  
+        if (!static::isValidClause($column)) {
            $this->xpdo->log(xPDO::LOG_LEVEL_ERROR, 'SQL injection attempt detected in sortby column; clause rejected');
        } elseif (!empty($column)) {
            $this->query['sortby'][] = array('column' => $column, 'direction' => $direction);
@@ -777,7 +793,7 @@ public function parseConditions($conditions, $conjunction = xPDOQuery::SQL_AND)
                ,'conjunction' => $conjunction
            ));
        }
-  
-        elseif (($pktype == 'integer' && is_numeric($conditions)) || ($pktype == 'string' && is_string($conditions) && $this->isValidClause($conditions))) {
+  
+        elseif (($pktype == 'integer' && is_numeric($conditions)) || ($pktype == 'string' && is_string($conditions) && static::isValidClause($conditions))) {
            if ($pktype == 'integer') {
                $param_type= PDO::PARAM_INT;
            } else {
@@ -795,12 +811,14 @@ public function parseConditions($conditions, $conjunction = xPDOQuery::SQL_AND)
     * Determines if a string contains a conditional operator.
     *
     * @param string $string The string to evaluate.
-  
-     * @return boolean True if the string is a complete conditional SQL clause.
+  
+     *
+  
+     * @return bool True if the string is a complete conditional SQL clause.
+  
+     * @throws xPDOException If a SQL injection attempt is detected.
     */
    public function isConditionalClause($string) {
        $matched= false;
        if (is_string($string)) {
-  
-            if (!$this->isValidClause($string)) {
+  
+            if (!static::isValidClause($string)) {
                throw new xPDOException("SQL injection attempt detected: {$string}");
            }
            foreach ($this->_operators as $operator) {
@@ -813,15 +831,6 @@ public function isConditionalClause($string) {
        return $matched;
    }
-  
-    public function isValidClause($clause) {
-  
-        $output = rtrim($clause, ' ;');
-  
-        $output = preg_replace("/\\\\'.*?\\\\'/", '{mask}', $output);
-  
-        $output = preg_replace('/\\".*?\\"/', '{mask}', $output);
-  
-        $output = preg_replace("/'.*?'/", '{mask}', $output);
-  
-        $output = preg_replace('/".*?"/', '{mask}', $output);
-  
-        return strpos($output, ';') === false && strpos(strtolower($output), 'union') === false;
-  
-    }
-  
-
    /**
     * Builds conditional clauses from xPDO condition expressions.
     *

diff --git a/xpdo/xpdo.class.php b/xpdo/xpdo.class.php
@@ -838,6 +838,7 @@ public function getObjectLoader($className, $method) {
    */
    public function getObject($className, $criteria= null, $cacheFlag= true) {
        $instance= null;
+  
+        $this->sanitizePKCriteria($className, $criteria);
        if ($criteria !== null) {
            $instance = $this->call($className, 'load', array(& $this, $className, $criteria, $cacheFlag));
        }
@@ -2735,6 +2736,37 @@ public function getPDOType($value) {
        }
        return $type;
    }
+  
+
+  
+    /**
+  
+     * Sanitize criteria expected to represent primary key values.
+  
+     *
+  
+     * @param string $className The name of the class.
+  
+     * @param mixed  &$criteria A reference to the criteria being used.
+  
+     */
+  
+    protected function sanitizePKCriteria($className, &$criteria) {
+  
+        if (is_scalar($criteria)) {
+  
+            $pkType = $this->getPKType($className);
+  
+            if (is_string($pkType)) {
+  
+                if (is_string($criteria) && !xPDOQuery::isValidClause($criteria)) {
+  
+                    $criteria = null;
+  
+                } else {
+  
+                    switch ($pkType) {
+  
+                        case 'int':
+  
+                        case 'integer':
+  
+                            $criteria = (int)$criteria;
+  
+                            break;
+  
+                        case 'string':
+  
+                            if (is_int($criteria)) {
+  
+                                $criteria = (string)$criteria;
+  
+                            }
+  
+                            break;
+  
+                    }
+  
+                }
+  
+            } elseif (is_array($pkType)) {
+  
+                $criteria = null;
+  
+            }
+  
+        }
+  
+    }
 }
 /**