tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 14:05:37 +02:00
Code Issues Projects Releases Wiki Activity

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

Browse Source
This commit is contained in:
Aaron Carlino 2019-01-11 11:55:05 +13:00
parent 11b87a430c
commit 6bf9542d66
7 changed files with 137 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
model/DataObject.php
View File

@ -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

15
model/ManyManyList.php
View File

@ -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);
}
}

13
model/fieldtypes/DBField.php
View File

@ -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;
}
}

6
model/fieldtypes/Money.php
View File

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

5
model/fieldtypes/PolymorphicForeignKey.php
View File

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

47
tests/model/DBFieldTest.php
View File

@ -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());

25
tests/model/DataObjectTest.php
View File

@ -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',
);
}