Skip to content

Commit 13c136c

Browse files
smartarellosmartarello
authored
Merge pull request #1 from ylelievre/1.6.9_SQL_Injection_fix
Fix SQL Injection on Limit
2 parents c117a9e + 7474dee commit 13c136c

File tree

4 files changed

+340
-7
lines changed

4 files changed

+340
-7
lines changed

‎runtime/lib/adapter/DBMySQL.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,9 @@ public function useQuoteIdentifier()
148148
*/
149149
public function applyLimit(&$sql, $offset, $limit)
150150
{
151+
$offset = (int) $offset;
152+
$limit = (int) $limit;
153+
151154
if ($limit > 0) {
152155
$sql .= " LIMIT " . ($offset > 0 ? $offset . ", " : "") . $limit;
153156
} elseif ($offset > 0) {

‎runtime/lib/query/Criteria.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1178,8 +1178,7 @@ public function isSingleRecord()
11781178
*/
11791179
public function setLimit($limit)
11801180
{
1181-
// TODO: do we enforce int here? 32bit issue if we do
1182-
$this->limit = $limit;
1181+
$this->limit = (int) $limit;
11831182

11841183
return $this;
11851184
}
@@ -1197,8 +1196,7 @@ public function getLimit()
11971196
/**
11981197
* Set offset.
11991198
*
1200-
* @param int $offset An int with the value for offset. (Note this values is
1201-
* cast to a 32bit integer and may result in truncatation)
1199+
* @param int $offset An int with the value for offset.
12021200
* @return Criteria Modified Criteria object (for fluent API)
12031201
*/
12041202
public function setOffset($offset)

‎test/testsuite/runtime/adapter/DBMySQLTest.php‎

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,194 @@ protected function getPdoMock()
9898

9999
return $con;
100100
}
101+
102+
/**
103+
* @dataProvider dataApplyLimit
104+
*/
105+
public function testApplyLimit($offset, $limit, $expectedSql)
106+
{
107+
$sql = '';
108+
109+
$db = new DBMySQL();
110+
$db->applyLimit($sql, $offset, $limit);
111+
112+
$this->assertEquals($expectedSql, $sql, 'Generated SQL does not match expected SQL');
113+
}
114+
115+
public function dataApplyLimit()
116+
{
117+
return array(
118+
119+
/*
120+
Offset & limit = 0
121+
*/
122+
123+
'Zero offset & limit' => array(
124+
'offset' => 0,
125+
'limit' => 0,
126+
'expectedSql' => ''
127+
),
128+
129+
/*
130+
Offset = 0
131+
*/
132+
133+
'32-bit limit' => array(
134+
'offset' => 0,
135+
'limit' => 4294967295,
136+
'expectedSql' => ' LIMIT 4294967295'
137+
),
138+
'32-bit limit as a string' => array(
139+
'offset' => 0,
140+
'limit' => '4294967295',
141+
'expectedSql' => ' LIMIT 4294967295'
142+
),
143+
144+
'64-bit limit' => array(
145+
'offset' => 0,
146+
'limit' => 9223372036854775807,
147+
'expectedSql' => ' LIMIT 9223372036854775807'
148+
),
149+
'64-bit limit as a string' => array(
150+
'offset' => 0,
151+
'limit' => '9223372036854775807',
152+
'expectedSql' => ' LIMIT 9223372036854775807'
153+
),
154+
155+
'Float limit' => array(
156+
'offset' => 0,
157+
'limit' => 123.9,
158+
'expectedSql' => ' LIMIT 123'
159+
),
160+
'Float limit as a string' => array(
161+
'offset' => 0,
162+
'limit' => '123.9',
163+
'expectedSql' => ' LIMIT 123'
164+
),
165+
166+
'Negative limit' => array(
167+
'offset' => 0,
168+
'limit' => -1,
169+
'expectedSql' => ''
170+
),
171+
'Non-numeric string limit' => array(
172+
'offset' => 0,
173+
'limit' => 'foo',
174+
'expectedSql' => ''
175+
),
176+
'SQL injected limit' => array(
177+
'offset' => 0,
178+
'limit' => '3;DROP TABLE abc',
179+
'expectedSql' => ' LIMIT 3'
180+
),
181+
182+
/*
183+
Limit = 0
184+
*/
185+
186+
'32-bit offset' => array(
187+
'offset' => 4294967295,
188+
'limit' => 0,
189+
'expectedSql' => ' LIMIT 4294967295, 18446744073709551615'
190+
),
191+
'32-bit offset as a string' => array(
192+
'offset' => '4294967295',
193+
'limit' => 0,
194+
'expectedSql' => ' LIMIT 4294967295, 18446744073709551615'
195+
),
196+
197+
'64-bit offset' => array(
198+
'offset' => 9223372036854775807,
199+
'limit' => 0,
200+
'expectedSql' => ' LIMIT 9223372036854775807, 18446744073709551615'
201+
),
202+
'64-bit offset as a string' => array(
203+
'offset' => '9223372036854775807',
204+
'limit' => 0,
205+
'expectedSql' => ' LIMIT 9223372036854775807, 18446744073709551615'
206+
),
207+
208+
'Float offset' => array(
209+
'offset' => 123.9,
210+
'limit' => 0,
211+
'expectedSql' => ' LIMIT 123, 18446744073709551615'
212+
),
213+
'Float offset as a string' => array(
214+
'offset' => '123.9',
215+
'limit' => 0,
216+
'expectedSql' => ' LIMIT 123, 18446744073709551615'
217+
),
218+
219+
'Negative offset' => array(
220+
'offset' => -1,
221+
'limit' => 0,
222+
'expectedSql' => ''
223+
),
224+
'Non-numeric string offset' => array(
225+
'offset' => 'foo',
226+
'limit' => 0,
227+
'expectedSql' => ''
228+
),
229+
'SQL injected offset' => array(
230+
'offset' => '3;DROP TABLE abc',
231+
'limit' => 0,
232+
'expectedSql' => ' LIMIT 3, 18446744073709551615'
233+
),
234+
235+
/*
236+
Offset & limit != 0
237+
*/
238+
239+
array(
240+
'offset' => 4294967295,
241+
'limit' => 999,
242+
'expectedSql' => ' LIMIT 4294967295, 999'
243+
),
244+
array(
245+
'offset' => '4294967295',
246+
'limit' => 999,
247+
'expectedSql' => ' LIMIT 4294967295, 999'
248+
),
249+
250+
array(
251+
'offset' => 9223372036854775807,
252+
'limit' => 999,
253+
'expectedSql' => ' LIMIT 9223372036854775807, 999'
254+
),
255+
array(
256+
'offset' => '9223372036854775807',
257+
'limit' => 999,
258+
'expectedSql' => ' LIMIT 9223372036854775807, 999'
259+
),
260+
261+
array(
262+
'offset' => 123.9,
263+
'limit' => 999,
264+
'expectedSql' => ' LIMIT 123, 999'
265+
),
266+
array(
267+
'offset' => '123.9',
268+
'limit' => 999,
269+
'expectedSql' => ' LIMIT 123, 999'
270+
),
271+
272+
array(
273+
'offset' => -1,
274+
'limit' => 999,
275+
'expectedSql' => ' LIMIT 999'
276+
),
277+
array(
278+
'offset' => 'foo',
279+
'limit' => 999,
280+
'expectedSql' => ' LIMIT 999'
281+
),
282+
array(
283+
'offset' => '3;DROP TABLE abc',
284+
'limit' => 999,
285+
'expectedSql' => ' LIMIT 3, 999'
286+
),
287+
);
288+
}
101289
}
102290

103291
// See: http://stackoverflow.com/questions/3138946/mocking-the-pdo-object-using-phpunit

‎test/testsuite/runtime/query/CriteriaTest.php‎

Lines changed: 147 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1102,16 +1102,160 @@ public function testClear()
11021102
$this->assertFalse($c->getUseTransaction(), 'useTransaction is false by default');
11031103
}
11041104

1105-
public function testLimit()
1105+
public function testDefaultLimit()
11061106
{
11071107
$c = new Criteria();
11081108
$this->assertEquals(0, $c->getLimit(), 'Limit is 0 by default');
1109+
}
11091110

1110-
$c2 = $c->setLimit(1);
1111-
$this->assertEquals(1, $c->getLimit(), 'Limit is set by setLimit');
1111+
/**
1112+
* @dataProvider dataLimit
1113+
*/
1114+
public function testLimit($limit, $expected)
1115+
{
1116+
$c = new Criteria();
1117+
$c2 = $c->setLimit($limit);
1118+
1119+
$this->assertSame($expected, $c->getLimit(), 'Correct limit is set by setLimit()');
11121120
$this->assertSame($c, $c2, 'setLimit() returns the current Criteria');
11131121
}
11141122

1123+
public function dataLimit()
1124+
{
1125+
return array(
1126+
'Negative value' => array(
1127+
'limit' => -1,
1128+
'expected' => -1
1129+
),
1130+
'Zero' => array(
1131+
'limit' => 0,
1132+
'expected' => 0
1133+
),
1134+
1135+
'Small integer' => array(
1136+
'limit' => 38427,
1137+
'expected' => 38427
1138+
),
1139+
'Small integer as a string' => array(
1140+
'limit' => '38427',
1141+
'expected' => 38427
1142+
),
1143+
1144+
'Largest 32-bit integer' => array(
1145+
'limit' => 2147483647,
1146+
'expected' => 2147483647
1147+
),
1148+
'Largest 32-bit integer as a string' => array(
1149+
'limit' => '2147483647',
1150+
'expected' => 2147483647
1151+
),
1152+
1153+
'Largest 64-bit integer' => array(
1154+
'limit' => 9223372036854775807,
1155+
'expected' => 9223372036854775807
1156+
),
1157+
'Largest 64-bit integer as a string' => array(
1158+
'limit' => '9223372036854775807',
1159+
'expected' => 9223372036854775807
1160+
),
1161+
1162+
'Decimal value' => array(
1163+
'limit' => 123.9,
1164+
'expected' => 123
1165+
),
1166+
'Decimal value as a string' => array(
1167+
'limit' => '123.9',
1168+
'expected' => 123
1169+
),
1170+
1171+
'Non-numeric string' => array(
1172+
'limit' => 'foo',
1173+
'expected' => 0
1174+
),
1175+
'Injected SQL' => array(
1176+
'limit' => '3;DROP TABLE abc',
1177+
'expected' => 3
1178+
),
1179+
);
1180+
}
1181+
1182+
public function testDefaultOffset()
1183+
{
1184+
$c = new Criteria();
1185+
$this->assertEquals(0, $c->getOffset(), 'Offset is 0 by default');
1186+
}
1187+
1188+
/**
1189+
* @dataProvider dataOffset
1190+
*/
1191+
public function testOffset($offset, $expected)
1192+
{
1193+
$c = new Criteria();
1194+
$c2 = $c->setOffset($offset);
1195+
1196+
$this->assertSame($expected, $c->getOffset(), 'Correct offset is set by setOffset()');
1197+
$this->assertSame($c, $c2, 'setOffset() returns the current Criteria');
1198+
}
1199+
1200+
public function dataOffset()
1201+
{
1202+
return array(
1203+
'Negative value' => array(
1204+
'offset' => -1,
1205+
'expected' => -1
1206+
),
1207+
'Zero' => array(
1208+
'offset' => 0,
1209+
'expected' => 0
1210+
),
1211+
1212+
'Small integer' => array(
1213+
'offset' => 38427,
1214+
'expected' => 38427
1215+
),
1216+
'Small integer as a string' => array(
1217+
'offset' => '38427',
1218+
'expected' => 38427
1219+
),
1220+
1221+
'Largest 32-bit integer' => array(
1222+
'offset' => 2147483647,
1223+
'expected' => 2147483647
1224+
),
1225+
'Largest 32-bit integer as a string' => array(
1226+
'offset' => '2147483647',
1227+
'expected' => 2147483647
1228+
),
1229+
1230+
'Largest 64-bit integer' => array(
1231+
'offset' => 9223372036854775807,
1232+
'expected' => 9223372036854775807
1233+
),
1234+
'Largest 64-bit integer as a string' => array(
1235+
'offset' => '9223372036854775807',
1236+
'expected' => 9223372036854775807
1237+
),
1238+
1239+
'Decimal value' => array(
1240+
'offset' => 123.9,
1241+
'expected' => 123
1242+
),
1243+
'Decimal value as a string' => array(
1244+
'offset' => '123.9',
1245+
'expected' => 123
1246+
),
1247+
1248+
'Non-numeric string' => array(
1249+
'offset' => 'foo',
1250+
'expected' => 0
1251+
),
1252+
'Injected SQL' => array(
1253+
'offset' => '3;DROP TABLE abc',
1254+
'expected' => 3
1255+
),
1256+
);
1257+
}
1258+
11151259
public function testDistinct()
11161260
{
11171261
$c = new Criteria();

0 commit comments

Comments
 (0)