Skip to content

Commit

Permalink
SQL injection fix: Cast limit to integer when setting via Criteria::s…
Browse files Browse the repository at this point in the history 
…etLimit() (#1465)

* Cast limit to integer when setting via Criteria::setLimit()

This is a followup to a fix for SQL injections with LIMIT clauses in MySQL [1]. That fix only applied to the MySQL adapter, and other existing or future adapters could still be at risk.

By coercing limit inputs to integers upon setting them, we can avoid SQL injection vulnerabilities with `limit()` across all database adapters.

The original code comments implied that integer coercion could be problematic with 32-bit integers, but unit tests in this PR prove otherwise. Even 64-bit integers seem to work fine.

[1] #1464

* Add missing tests for setOffset()

* Remove note about 32-bit truncation

Unit tests show that even 64-bit integers aren't truncated.
  • Loading branch information
@mpetrovich @marcj
mpetrovich authored and marcj committed Feb 19, 2018
1 parent cd23d73 commit 4c309e3
Show file tree
Hide file tree
Showing 2 changed files with 149 additions and 7 deletions.
6 changes: 2 additions & 4 deletions src/Propel/Runtime/ActiveQuery/Criteria.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1259,8 +1259,7 @@ public function isSingleRecord()
*/
public function setLimit($limit)
{
// TODO: do we enforce int here? 32bit issue if we do
$this->limit = $limit;

This comment has been minimized.

Sign in to view

Copy link
@RutrackerOrg

RutrackerOrg Mar 15, 2018

omg
$this->limit = (int) $limit;

return $this;
}
Expand All @@ -1278,8 +1277,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 truncation)
* @param int $offset An int with the value for offset.
* @return $this|Criteria Modified Criteria object (for fluent API)
*/
public function setOffset($offset)
Expand Down
150 changes: 147 additions & 3 deletions tests/Propel/Tests/Runtime/ActiveQuery/CriteriaTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1064,16 +1064,160 @@ public function testClear()
$this->assertFalse($c->getUseTransaction(), 'useTransaction is false by default');
}

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

/**
* @dataProvider dataLimit
*/
public function testLimit($limit, $expected)
{
$c = new Criteria();
$c2 = $c->setLimit($limit);

$c2 = $c->setLimit(1);
$this->assertEquals(1, $c->getLimit(), 'Limit is set by setLimit');
$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 testCombineAndFilterBy()
{
$params = [];
Expand Down

0 comments on commit 4c309e3

Please sign in to comment.