From 6bf9542d664ac7935691c8055505b7ad8ea26e9a Mon Sep 17 00:00:00 2001 From: Aaron Carlino Date: Fri, 11 Jan 2019 11:55:05 +1300 Subject: [PATCH] [SS-2018-021] Patch SQL Injection vulnerability when arrays are assigned to DataObject Fields --- model/DataObject.php | 27 +++++++++++++ model/ManyManyList.php | 15 +++++++ model/fieldtypes/DBField.php | 13 +++++- model/fieldtypes/Money.php | 6 +++ model/fieldtypes/PolymorphicForeignKey.php | 5 +++ tests/model/DBFieldTest.php | 47 ++++++++++++++++++++++ tests/model/DataObjectTest.php | 25 ++++++++++++ 7 files changed, 137 insertions(+), 1 deletion(-) diff --git a/model/DataObject.php b/model/DataObject.php index 762aef968..d778a6994 100644 --- a/model/DataObject.php +++ b/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); } @@ -2663,6 +2678,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 diff --git a/model/ManyManyList.php b/model/ManyManyList.php index 3ae404bc5..a64d00c34 100644 --- a/model/ManyManyList.php +++ b/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); } } diff --git a/model/fieldtypes/DBField.php b/model/fieldtypes/DBField.php index acc2ea7ec..c2264f115 100644 --- a/model/fieldtypes/DBField.php +++ b/model/fieldtypes/DBField.php @@ -178,7 +178,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; @@ -351,4 +351,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; + } } diff --git a/model/fieldtypes/Money.php b/model/fieldtypes/Money.php index b98569b16..574fdb66d 100644 --- a/model/fieldtypes/Money.php +++ b/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; + } + } diff --git a/model/fieldtypes/PolymorphicForeignKey.php b/model/fieldtypes/PolymorphicForeignKey.php index e6d1898fa..37b55060b 100644 --- a/model/fieldtypes/PolymorphicForeignKey.php +++ b/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; + } } diff --git a/tests/model/DBFieldTest.php b/tests/model/DBFieldTest.php index d19baea68..c0cb6653d 100644 --- a/tests/model/DBFieldTest.php +++ b/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()); diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index ca51d30dc..83e9c6bd5 100644 --- a/tests/model/DataObjectTest.php +++ b/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 { @@ -2035,3 +2054,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', + ); +} \ No newline at end of file