[SS-2018-021] Patch SQL Injection vulnerability when arrays are assigned to DataObject Fields

model/DataObject.php

@@ -1357,6 +1357,21 @@ 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)) {
+					user_error(
+						'DataObject::writeManipulation: parameterised field assignments are disallowed',
+						E_USER_ERROR
+					);
+				}
+			}
+		}
+
 		// Perform the manipulation
 		DB::manipulate($manipulation);
 	}
@@ -2658,6 +2673,18 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 				user_error('DataObject::setField: passed an object that is not a DBField', E_USER_WARNING);
 			}
 
+			$dbField = $this->dbObject($fieldName);
+			if ($dbField && $dbField->scalarValueOnly() && !empty($val) && !is_scalar($val)){
+			    $val = null;
+                user_error(
+                    sprintf(
+                        'DataObject::setField: %s only accepts scalars',
+                        $fieldName
+                    ),
+                    E_USER_WARNING
+                );
+            }
+
 			// 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

model/ManyManyList.php

@@ -270,6 +270,21 @@ class ManyManyList extends RelationList {
 			$manipulation[$this->joinTable]['fields'][$this->localKey] = $itemID;
 			$manipulation[$this->joinTable]['fields'][$this->foreignKey] = $foreignID;
 
+            // 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)) {
+                        user_error(
+                            'ManyManyList::add: parameterised field assignments are disallowed',
+                            E_USER_ERROR
+                        );
+                    }
+                }
+            }
+
 			DB::manipulate($manipulation);
 		}
 	}

model/fieldtypes/DBField.php

@@ -170,7 +170,7 @@ abstract class DBField extends ViewableData {
 	 * @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;
@@ -343,4 +343,15 @@ DBG;
 	public function __toString() {
 		return $this->forTemplate();
 	}
+
+    /**
+     * Whatever this DBField only accepts scalar values.
+     *
+     * Composite DBField to override this method and return `false`. So they can accept arrays of values.
+     * @return boolean
+     */
+	public function scalarValueOnly()
+    {
+        return true;
+    }
 }

model/fieldtypes/Money.php

@@ -306,4 +306,10 @@ class Money extends DBField implements CompositeDBField {
 	public function __toString() {
 		return (string)$this->getAmount();
 	}
+
+    public function scalarValueOnly()
+    {
+        return false;
+    }
+
 }

model/fieldtypes/PolymorphicForeignKey.php

@@ -190,4 +190,9 @@ class PolymorphicForeignKey extends ForeignKey implements CompositeDBField {
 	public function exists() {
 		return $this->getClassValue() && $this->getIDValue();
 	}
+
+    public function scalarValueOnly()
+    {
+        return false;
+    }
 }

tests/model/DBFieldTest.php

@@ -167,6 +167,53 @@ class DBFieldTest extends SapphireTest {
 		$this->assertEquals(PHP_INT_MAX, $bigInt->getValue());
 	}
 
+    /**
+     * @dataProvider dataProviderPrepValueForDBArrayValue
+     */
+    public function testPrepValueForDBArrayValue($dbFieldName, $scalarValueOnly, $extraArgs = array())
+    {
+        $reflection = new ReflectionClass($dbFieldName);
+        /** @var DBField $dbField  */
+        $dbField = $reflection->newInstanceArgs($extraArgs);
+        $dbField->setName('SomeField');
+        $payload = array('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 array(
+            array('BigInt', true),
+            array('Boolean', true),
+            array('Currency', true),
+            array('Date', true),
+            array('SS_Datetime', true),
+            array('DBLocale', true),
+            array('Decimal', true),
+            array('Double', true),
+            array('Enum', true),
+            array('Float', true),
+            array('ForeignKey', true, array('SomeField')),
+            array('HTMLText', true),
+            array('HTMLVarchar', true),
+            array('Int', true),
+            array('Money', false),
+            array('MultiEnum', true, array('SomeField', array('One', 'Two', 'Three'))),
+            array('Percentage', true),
+            array('PolymorphicForeignKey', false, array('SomeField')),
+            array('PrimaryKey', true, array('SomeField', singleton('Image'))),
+            array('Text', true),
+            array('Time', true),
+            array('Varchar', true),
+            array('Year', true),
+        );
+    }
+
 	public function testExists() {
 		$varcharField = new Varchar("testfield");
 		$this->assertTrue($varcharField->getNullifyEmpty());

tests/model/DataObjectTest.php

@@ -1754,6 +1754,25 @@ class DataObjectTest extends SapphireTest {
 		$this->assertEquals(PHP_INT_MAX, DataObjectTest_Staff::get()->byID($staff->ID)->Salary);
 	}
 
+
+    /**
+     * @expectedException PHPUnit_Framework_Error_Warning
+     */
+	public function testSetFieldWithArrayOnScalarOnlyField()
+    {
+        $do = singleton('DataObjectTest_CompositeDBField');
+        $do->NonCompositeField = 'Some Value';
+        $do->NonCompositeField = array('Amount' => 123, 'Currency' => 'CAD');
+        $this->assertEmpty($do->NonCompositeField);
+    }
+
+    public function testSetFieldWithArrayOnCompositeField()
+    {
+        $do = singleton('DataObjectTest_CompositeDBField');
+        $do->CompositeMoneyField = array('Amount' => 123, 'Currency' => 'CAD');
+        $this->assertNotEmpty($do->CompositeMoneyField);
+    }
+
 }
 
 class DataObjectTest_Sortable extends DataObject implements TestOnly {
@@ -2033,3 +2052,9 @@ class DataObjectTest_Bogey extends DataObject implements TestOnly {}
 
 DataObjectTest_Team::add_extension('DataObjectTest_Team_Extension');
 
+class DataObjectTest_CompositeDBField extends DataObject implements TestOnly {
+    private static $db = array(
+        'NonCompositeField' => 'Varchar',
+        'CompositeMoneyField' => 'Money',
+    );
+}