[SS-2018-021] Fix potential SQL vulnerability in non-scalar value hyrdation

src/ORM/DataObject.php

@@ -1453,6 +1453,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
      * @param string $baseTable Base table
      * @param string $now Timestamp to use for the current time
      * @param bool $isNewRecord If this is a new record
+     * @throws InvalidArgumentException
      */
     protected function writeManipulation($baseTable, $now, $isNewRecord)
     {
@@ -1471,6 +1472,20 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
             $manipulation[$baseTable]['command'] = 'update';
         }
 
+        // Make sure none of our field assignment are arrays
+        foreach ($manipulation as $tableManipulation) {
+            if (!isset($tableManipulation['fields'])) {
+                continue;
+            }
+            foreach ($tableManipulation['fields'] as $fieldValue) {
+                if (is_array($fieldValue)) {
+                    throw new InvalidArgumentException(
+                        'DataObject::writeManipulation: parameterised field assignments are disallowed'
+                    );
+                }
+            }
+        }
+
         // Perform the manipulation
         DB::manipulate($manipulation);
     }
@@ -2630,6 +2645,18 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
                 throw new InvalidArgumentException('DataObject::setField: passed an object that is not a DBField');
             }
 
+            if (!empty($val) && !is_scalar($val)) {
+                $dbField = $this->dbObject($fieldName);
+                if ($dbField && $dbField->scalarValueOnly()) {
+                    throw new InvalidArgumentException(
+                        sprintf(
+                            'DataObject::setField: %s only accepts scalars',
+                            $fieldName
+                        )
+                    );
+                }
+            }
+
             // if a field is not existing or has strictly changed
             if (!isset($this->record[$fieldName]) || $this->record[$fieldName] !== $val) {
                 // TODO Add check for php-level defaults which are not set in the db

src/ORM/FieldType/DBComposite.php

@@ -309,4 +309,9 @@ abstract class DBComposite extends DBField
             ];
         }
     }
+
+    public function scalarValueOnly()
+    {
+        return false;
+    }
 }

src/ORM/FieldType/DBField.php

@@ -335,18 +335,16 @@ abstract class DBField extends ViewableData implements DBIndexable
      * will be escaped automatically by the prepared query processor, so it
      * should not be escaped or quoted at all.
      *
-     * The field values could also be in paramaterised format, such as
-     * array('MAX(?,?)' => array(42, 69)), allowing the use of raw SQL values such as
-     * array('NOW()' => array()).
-     *
-     * @see SQLWriteExpression::addAssignments for syntax examples
-     *
      * @param $value mixed The value to check
      * @return mixed The raw value, or escaped parameterised details
      */
     public function prepValueForDB($value)
     {
-        if ($value === null || $value === "" || $value === false) {
+        if ($value === null ||
+            $value === "" ||
+            $value === false ||
+            ($this->scalarValueOnly() && !is_scalar($value))
+        ) {
             return null;
         } else {
             return $value;
@@ -653,4 +651,15 @@ DBG;
         }
         return null;
     }
+
+    /**
+     * Whether or not this DBField only accepts scalar values.
+     *
+     * Composite DBFields can override this method and return `false` so they can accept arrays of values.
+     * @return boolean
+     */
+    public function scalarValueOnly()
+    {
+        return true;
+    }
 }

src/ORM/FieldType/DBString.php

@@ -94,7 +94,7 @@ abstract class DBString extends DBField
     public function prepValueForDB($value)
     {
         // Cast non-empty value
-        if (strlen($value)) {
+        if (is_scalar($value) && strlen($value)) {
             return (string)$value;
         }
 

src/ORM/ManyManyList.php

@@ -299,6 +299,20 @@ class ManyManyList extends RelationList
             $manipulation[$this->joinTable]['fields'][$this->localKey] = $itemID;
             $manipulation[$this->joinTable]['fields'][$this->foreignKey] = $foreignID;
 
+            // Make sure none of our field assignments are arrays
+            foreach ($manipulation as $tableManipulation) {
+                if (!isset($tableManipulation['fields'])) {
+                    continue;
+                }
+                foreach ($tableManipulation['fields'] as $fieldValue) {
+                    if (is_array($fieldValue)) {
+                        throw new InvalidArgumentException(
+                            'ManyManyList::add: parameterised field assignments are disallowed'
+                        );
+                    }
+                }
+            }
+
             DB::manipulate($manipulation);
         }
     }

tests/php/ORM/DBFieldTest.php

@@ -2,17 +2,32 @@
 
 namespace SilverStripe\ORM\Tests;
 
+use SilverStripe\Assets\Image;
 use SilverStripe\ORM\FieldType\DBBigInt;
 use SilverStripe\ORM\FieldType\DBBoolean;
+use SilverStripe\ORM\FieldType\DBCurrency;
+use SilverStripe\ORM\FieldType\DBDate;
+use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\ORM\FieldType\DBDecimal;
 use SilverStripe\ORM\FieldType\DBDouble;
+use SilverStripe\ORM\FieldType\DBEnum;
 use SilverStripe\ORM\FieldType\DBFloat;
+use SilverStripe\ORM\FieldType\DBForeignKey;
 use SilverStripe\ORM\FieldType\DBHTMLText;
+use SilverStripe\ORM\FieldType\DBHTMLVarchar;
+use SilverStripe\ORM\FieldType\DBInt;
+use SilverStripe\ORM\FieldType\DBLocale;
+use SilverStripe\ORM\FieldType\DBMoney;
+use SilverStripe\ORM\FieldType\DBMultiEnum;
+use SilverStripe\ORM\FieldType\DBPercentage;
+use SilverStripe\ORM\FieldType\DBPolymorphicForeignKey;
+use SilverStripe\ORM\FieldType\DBPrimaryKey;
 use SilverStripe\ORM\FieldType\DBString;
 use SilverStripe\ORM\FieldType\DBTime;
 use SilverStripe\ORM\FieldType\DBVarchar;
 use SilverStripe\ORM\FieldType\DBText;
 use SilverStripe\Dev\SapphireTest;
+use SilverStripe\ORM\FieldType\DBYear;
 
 /**
  * Tests for DBField objects.
@@ -189,6 +204,54 @@ class DBFieldTest extends SapphireTest
         $this->assertEquals(PHP_INT_MAX, $bigInt->getValue());
     }
 
+    /**
+     * @dataProvider dataProviderPrepValueForDBArrayValue
+     */
+    public function testPrepValueForDBArrayValue($dbFieldName, $scalarValueOnly, $extraArgs = [])
+    {
+        $reflection = new \ReflectionClass($dbFieldName);
+        /**
+         * @var DBField
+         */
+        $dbField = $reflection->newInstanceArgs($extraArgs);
+        $dbField->setName('SomeField');
+        $payload = ['GREATEST(0,?)' => '2'];
+        $preparedValue = $dbField->prepValueForDB($payload);
+        $this->assertTrue(
+            !$scalarValueOnly || !is_array($preparedValue),
+            '`prepValueForDB` can not return an array if scalarValueOnly is true'
+        );
+        $this->assertEquals($scalarValueOnly, $dbField->scalarValueOnly());
+    }
+
+    public function dataProviderPrepValueForDBArrayValue()
+    {
+        return [
+            [DBBigInt::class, true],
+            [DBBoolean::class, true],
+            [DBCurrency::class, true],
+            [DBDate::class, true],
+            [DBDatetime::class, true],
+            [DBDecimal::class, true],
+            [DBDouble::class, true],
+            [DBEnum::class, true],
+            [DBFloat::class, true],
+            [DBForeignKey::class, true, ['SomeField']],
+            [DBHTMLText::class, true],
+            [DBHTMLVarchar::class, true],
+            [DBInt::class, true],
+            [DBLocale::class, true],
+            [DBMoney::class, false],
+            [DBMultiEnum::class, true, ['SomeField', ['One', 'Two', 'Three']]],
+            [DBPercentage::class, true],
+            [DBPolymorphicForeignKey::class, false, ['SomeField']],
+            [DBText::class, true],
+            [DBTime::class, true],
+            [DBVarchar::class, true],
+            [DBYear::class, true],
+        ];
+    }
+
     public function testExists()
     {
         $varcharField = new DBVarchar("testfield");

tests/php/ORM/DataObjectTest.php

@@ -17,7 +17,9 @@ use SilverStripe\ORM\FieldType\DBField;
 use SilverStripe\ORM\FieldType\DBPolymorphicForeignKey;
 use SilverStripe\ORM\FieldType\DBVarchar;
 use SilverStripe\ORM\ManyManyList;
+use SilverStripe\ORM\Tests\DataObjectTest\Company;
 use SilverStripe\ORM\Tests\DataObjectTest\Player;
+use SilverStripe\ORM\Tests\DataObjectTest\Team;
 use SilverStripe\View\ViewableData;
 use stdClass;
 
@@ -2221,4 +2223,20 @@ class DataObjectTest extends SapphireTest
             ['"DataObjectTest_TeamComment"."Name"' => 'does not exists']
         ));
     }
+
+    public function testSetFieldWithArrayOnScalarOnlyField()
+    {
+        $this->expectException(InvalidArgumentException::class);
+        $do = Company::singleton();
+        $do->FoundationYear = '1984';
+        $do->FoundationYear = array('Amount' => 123, 'Currency' => 'CAD');
+        $this->assertEmpty($do->FoundationYear);
+    }
+
+    public function testSetFieldWithArrayOnCompositeField()
+    {
+        $do = Company::singleton();
+        $do->SalaryCap = array('Amount' => 123456, 'Currency' => 'CAD');
+        $this->assertNotEmpty($do->SalaryCap);
+    }
 }

tests/php/ORM/DataObjectTest/Company.php

@@ -10,7 +10,9 @@ class Company extends DataObject implements TestOnly
     private static $table_name = 'DataObjectTest_Company';
 
     private static $db = [
-        'Name' => 'Varchar'
+        'Name' => 'Varchar',
+        'MarketValue' => 'Money',
+        'FoundationYear' => 'Year'
     ];
 
     private static $has_one = [

tests/php/ORM/DataObjectTest/Team.php

@@ -10,6 +10,8 @@ use SilverStripe\ORM\ManyManyList;
 /**
  * @property string Title
  * @property string DatabaseField
+ * @property array SalaryCap
+ * @property string FoundationYear
  * @method Player Captain()
  * @method Player Founder()
  * @method Player HasOneRelationship()
@@ -27,7 +29,7 @@ class Team extends DataObject implements TestOnly
 
     private static $db = array(
         'Title' => 'Varchar',
-        'DatabaseField' => 'HTMLVarchar'
+        'DatabaseField' => 'HTMLVarchar',
     );
 
     private static $has_one = array(