Skip to content

Commit

Permalink
Merge pull request #1 from ylelievre/1.6.9_SQL_Injection_fix
Browse files Browse the repository at this point in the history 
Fix SQL Injection on Limit
  • Loading branch information
@smartarello
smartarello authored May 3, 2018
2 parents c117a9e + 7474dee commit 13c136c
Show file tree
Hide file tree
Showing 4 changed files with 340 additions and 7 deletions.
3 changes: 3 additions & 0 deletions runtime/lib/adapter/DBMySQL.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,9 @@ public function useQuoteIdentifier()
*/
public function applyLimit(&$sql, $offset, $limit)
{
$offset = (int) $offset;
$limit = (int) $limit;

if ($limit > 0) {
$sql .= " LIMIT " . ($offset > 0 ? $offset . ", " : "") . $limit;
} elseif ($offset > 0) {
Expand Down
6 changes: 2 additions & 4 deletions runtime/lib/query/Criteria.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1178,8 +1178,7 @@ public function isSingleRecord()
*/
public function setLimit($limit)
{
// TODO: do we enforce int here? 32bit issue if we do
$this->limit = $limit;
$this->limit = (int) $limit;

return $this;
}
Expand All @@ -1197,8 +1196,7 @@ public function getLimit()
/**
* Set offset.
*
* @param int $offset An int with the value for offset. (Note this values is
* cast to a 32bit integer and may result in truncatation)
* @param int $offset An int with the value for offset.
* @return Criteria Modified Criteria object (for fluent API)
*/
public function setOffset($offset)
Expand Down
188 changes: 188 additions & 0 deletions test/testsuite/runtime/adapter/DBMySQLTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,194 @@ protected function getPdoMock()

return $con;
}

/**
* @dataProvider dataApplyLimit
*/
public function testApplyLimit($offset, $limit, $expectedSql)
{
$sql = '';

$db = new DBMySQL();
$db->applyLimit($sql, $offset, $limit);

$this->assertEquals($expectedSql, $sql, 'Generated SQL does not match expected SQL');
}

public function dataApplyLimit()
{
return array(

/*
Offset & limit = 0
*/

'Zero offset & limit' => array(
'offset' => 0,
'limit' => 0,
'expectedSql' => ''
),

/*
Offset = 0
*/

'32-bit limit' => array(
'offset' => 0,
'limit' => 4294967295,
'expectedSql' => ' LIMIT 4294967295'
),
'32-bit limit as a string' => array(
'offset' => 0,
'limit' => '4294967295',
'expectedSql' => ' LIMIT 4294967295'
),

'64-bit limit' => array(
'offset' => 0,
'limit' => 9223372036854775807,
'expectedSql' => ' LIMIT 9223372036854775807'
),
'64-bit limit as a string' => array(
'offset' => 0,
'limit' => '9223372036854775807',
'expectedSql' => ' LIMIT 9223372036854775807'
),

'Float limit' => array(
'offset' => 0,
'limit' => 123.9,
'expectedSql' => ' LIMIT 123'
),
'Float limit as a string' => array(
'offset' => 0,
'limit' => '123.9',
'expectedSql' => ' LIMIT 123'
),

'Negative limit' => array(
'offset' => 0,
'limit' => -1,
'expectedSql' => ''
),
'Non-numeric string limit' => array(
'offset' => 0,
'limit' => 'foo',
'expectedSql' => ''
),
'SQL injected limit' => array(
'offset' => 0,
'limit' => '3;DROP TABLE abc',
'expectedSql' => ' LIMIT 3'
),

/*
Limit = 0
*/

'32-bit offset' => array(
'offset' => 4294967295,
'limit' => 0,
'expectedSql' => ' LIMIT 4294967295, 18446744073709551615'
),
'32-bit offset as a string' => array(
'offset' => '4294967295',
'limit' => 0,
'expectedSql' => ' LIMIT 4294967295, 18446744073709551615'
),

'64-bit offset' => array(
'offset' => 9223372036854775807,
'limit' => 0,
'expectedSql' => ' LIMIT 9223372036854775807, 18446744073709551615'
),
'64-bit offset as a string' => array(
'offset' => '9223372036854775807',
'limit' => 0,
'expectedSql' => ' LIMIT 9223372036854775807, 18446744073709551615'
),

'Float offset' => array(
'offset' => 123.9,
'limit' => 0,
'expectedSql' => ' LIMIT 123, 18446744073709551615'
),
'Float offset as a string' => array(
'offset' => '123.9',
'limit' => 0,
'expectedSql' => ' LIMIT 123, 18446744073709551615'
),

'Negative offset' => array(
'offset' => -1,
'limit' => 0,
'expectedSql' => ''
),
'Non-numeric string offset' => array(
'offset' => 'foo',
'limit' => 0,
'expectedSql' => ''
),
'SQL injected offset' => array(
'offset' => '3;DROP TABLE abc',
'limit' => 0,
'expectedSql' => ' LIMIT 3, 18446744073709551615'
),

/*
Offset & limit != 0
*/

array(
'offset' => 4294967295,
'limit' => 999,
'expectedSql' => ' LIMIT 4294967295, 999'
),
array(
'offset' => '4294967295',
'limit' => 999,
'expectedSql' => ' LIMIT 4294967295, 999'
),

array(
'offset' => 9223372036854775807,
'limit' => 999,
'expectedSql' => ' LIMIT 9223372036854775807, 999'
),
array(
'offset' => '9223372036854775807',
'limit' => 999,
'expectedSql' => ' LIMIT 9223372036854775807, 999'
),

array(
'offset' => 123.9,
'limit' => 999,
'expectedSql' => ' LIMIT 123, 999'
),
array(
'offset' => '123.9',
'limit' => 999,
'expectedSql' => ' LIMIT 123, 999'
),

array(
'offset' => -1,
'limit' => 999,
'expectedSql' => ' LIMIT 999'
),
array(
'offset' => 'foo',
'limit' => 999,
'expectedSql' => ' LIMIT 999'
),
array(
'offset' => '3;DROP TABLE abc',
'limit' => 999,
'expectedSql' => ' LIMIT 3, 999'
),
);
}
}

// See: http://stackoverflow.com/questions/3138946/mocking-the-pdo-object-using-phpunit
Expand Down
150 changes: 147 additions & 3 deletions test/testsuite/runtime/query/CriteriaTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1102,16 +1102,160 @@ public function testClear()
$this->assertFalse($c->getUseTransaction(), 'useTransaction is false by default');
}

public function testLimit()
public function testDefaultLimit()
{
$c = new Criteria();
$this->assertEquals(0, $c->getLimit(), 'Limit is 0 by default');
}

$c2 = $c->setLimit(1);
$this->assertEquals(1, $c->getLimit(), 'Limit is set by setLimit');
/**
* @dataProvider dataLimit
*/
public function testLimit($limit, $expected)
{
$c = new Criteria();
$c2 = $c->setLimit($limit);

$this->assertSame($expected, $c->getLimit(), 'Correct limit is set by setLimit()');
$this->assertSame($c, $c2, 'setLimit() returns the current Criteria');
}

public function dataLimit()
{
return array(
'Negative value' => array(
'limit' => -1,
'expected' => -1
),
'Zero' => array(
'limit' => 0,
'expected' => 0
),

'Small integer' => array(
'limit' => 38427,
'expected' => 38427
),
'Small integer as a string' => array(
'limit' => '38427',
'expected' => 38427
),

'Largest 32-bit integer' => array(
'limit' => 2147483647,
'expected' => 2147483647
),
'Largest 32-bit integer as a string' => array(
'limit' => '2147483647',
'expected' => 2147483647
),

'Largest 64-bit integer' => array(
'limit' => 9223372036854775807,
'expected' => 9223372036854775807
),
'Largest 64-bit integer as a string' => array(
'limit' => '9223372036854775807',
'expected' => 9223372036854775807
),

'Decimal value' => array(
'limit' => 123.9,
'expected' => 123
),
'Decimal value as a string' => array(
'limit' => '123.9',
'expected' => 123
),

'Non-numeric string' => array(
'limit' => 'foo',
'expected' => 0
),
'Injected SQL' => array(
'limit' => '3;DROP TABLE abc',
'expected' => 3
),
);
}

public function testDefaultOffset()
{
$c = new Criteria();
$this->assertEquals(0, $c->getOffset(), 'Offset is 0 by default');
}

/**
* @dataProvider dataOffset
*/
public function testOffset($offset, $expected)
{
$c = new Criteria();
$c2 = $c->setOffset($offset);

$this->assertSame($expected, $c->getOffset(), 'Correct offset is set by setOffset()');
$this->assertSame($c, $c2, 'setOffset() returns the current Criteria');
}

public function dataOffset()
{
return array(
'Negative value' => array(
'offset' => -1,
'expected' => -1
),
'Zero' => array(
'offset' => 0,
'expected' => 0
),

'Small integer' => array(
'offset' => 38427,
'expected' => 38427
),
'Small integer as a string' => array(
'offset' => '38427',
'expected' => 38427
),

'Largest 32-bit integer' => array(
'offset' => 2147483647,
'expected' => 2147483647
),
'Largest 32-bit integer as a string' => array(
'offset' => '2147483647',
'expected' => 2147483647
),

'Largest 64-bit integer' => array(
'offset' => 9223372036854775807,
'expected' => 9223372036854775807
),
'Largest 64-bit integer as a string' => array(
'offset' => '9223372036854775807',
'expected' => 9223372036854775807
),

'Decimal value' => array(
'offset' => 123.9,
'expected' => 123
),
'Decimal value as a string' => array(
'offset' => '123.9',
'expected' => 123
),

'Non-numeric string' => array(
'offset' => 'foo',
'expected' => 0
),
'Injected SQL' => array(
'offset' => '3;DROP TABLE abc',
'expected' => 3
),
);
}

public function testDistinct()
{
$c = new Criteria();
Expand Down

0 comments on commit 13c136c

Please sign in to comment.