From 95505db7d666a75f249f65cb1af74dca01d39add Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Tue, 18 Dec 2018 16:46:56 +1300 Subject: [PATCH] [SS-2018-021] Fix potential SQL vulnerability in non-scalar value hyrdation --- src/ORM/DataObject.php | 27 ++++++++++ src/ORM/FieldType/DBComposite.php | 5 ++ src/ORM/FieldType/DBField.php | 23 ++++++--- src/ORM/FieldType/DBString.php | 2 +- src/ORM/ManyManyList.php | 14 ++++++ tests/php/ORM/DBFieldTest.php | 63 ++++++++++++++++++++++++ tests/php/ORM/DataObjectTest.php | 18 +++++++ tests/php/ORM/DataObjectTest/Company.php | 4 +- tests/php/ORM/DataObjectTest/Team.php | 4 +- 9 files changed, 150 insertions(+), 10 deletions(-) diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 881add45c..f6e49222d 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -1307,6 +1307,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) { @@ -1325,6 +1326,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); } @@ -2403,6 +2418,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 diff --git a/src/ORM/FieldType/DBComposite.php b/src/ORM/FieldType/DBComposite.php index a938f242f..d55700c05 100644 --- a/src/ORM/FieldType/DBComposite.php +++ b/src/ORM/FieldType/DBComposite.php @@ -309,4 +309,9 @@ abstract class DBComposite extends DBField ]; } } + + public function scalarValueOnly() + { + return false; + } } diff --git a/src/ORM/FieldType/DBField.php b/src/ORM/FieldType/DBField.php index 70aa13cb4..73d7172d4 100644 --- a/src/ORM/FieldType/DBField.php +++ b/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; + } } diff --git a/src/ORM/FieldType/DBString.php b/src/ORM/FieldType/DBString.php index 3732c0952..d40481dd7 100644 --- a/src/ORM/FieldType/DBString.php +++ b/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; } diff --git a/src/ORM/ManyManyList.php b/src/ORM/ManyManyList.php index d5495ee36..a576a33db 100644 --- a/src/ORM/ManyManyList.php +++ b/src/ORM/ManyManyList.php @@ -295,6 +295,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); } } diff --git a/tests/php/ORM/DBFieldTest.php b/tests/php/ORM/DBFieldTest.php index 8946d72c9..fb3bb6bf4 100644 --- a/tests/php/ORM/DBFieldTest.php +++ b/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"); diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index dbc46f090..cfda6085e 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -16,7 +16,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; @@ -2129,4 +2131,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); + } } diff --git a/tests/php/ORM/DataObjectTest/Company.php b/tests/php/ORM/DataObjectTest/Company.php index c17e47d40..a9107e263 100644 --- a/tests/php/ORM/DataObjectTest/Company.php +++ b/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 = [ diff --git a/tests/php/ORM/DataObjectTest/Team.php b/tests/php/ORM/DataObjectTest/Team.php index d9436a1fa..ee2937e2a 100644 --- a/tests/php/ORM/DataObjectTest/Team.php +++ b/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(