Skip to content

Commit 60ee42e

Browse files
GirgiasGirgias
committed
Merge branch 'PHP-8.4'
* PHP-8.4: ext/pdo: Fix a UAF when changing default fetch class ctor args
2 parents ab99693 + 7f321a1 commit 60ee42e

7 files changed

+421
-1
lines changed

‎ext/pdo/pdo_stmt.c

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1199,6 +1199,7 @@ PHP_METHOD(PDOStatement, fetchAll)
11991199
zend_class_entry *old_ce;
12001200
zval old_ctor_args, *ctor_args = NULL;
12011201
uint32_t old_arg_count;
1202+
HashTable *current_ctor = NULL;
12021203

12031204
ZEND_PARSE_PARAMETERS_START(0, 3)
12041205
Z_PARAM_OPTIONAL
@@ -1217,6 +1218,10 @@ PHP_METHOD(PDOStatement, fetchAll)
12171218

12181219
old_ce = stmt->fetch.cls.ce;
12191220
ZVAL_COPY_VALUE(&old_ctor_args, &stmt->fetch.cls.ctor_args);
1221+
if (Z_TYPE(old_ctor_args) == IS_ARRAY) {
1222+
/* Protect against destruction by marking this as immutable: we consider this non-owned temporarily */
1223+
Z_TYPE_INFO(stmt->fetch.cls.ctor_args) = IS_ARRAY;
1224+
}
12201225
old_arg_count = stmt->fetch.cls.fci.param_count;
12211226

12221227
do_fetch_opt_finish(stmt, 0);
@@ -1241,7 +1246,13 @@ PHP_METHOD(PDOStatement, fetchAll)
12411246
}
12421247

12431248
if (ctor_args && zend_hash_num_elements(Z_ARRVAL_P(ctor_args)) > 0) {
1244-
ZVAL_COPY_VALUE(&stmt->fetch.cls.ctor_args, ctor_args); /* we're not going to free these */
1249+
/* We increase the refcount and store it in case usercode has been messing around with the ctor args.
1250+
* We need to store current_ctor separately as usercode may change the ctor_args which will cause a leak. */
1251+
current_ctor = Z_ARRVAL_P(ctor_args);
1252+
ZVAL_COPY(&stmt->fetch.cls.ctor_args, ctor_args);
1253+
/* Protect against destruction by marking this as immutable: we consider this non-owned
1254+
* as destruction is handled via current_ctor. */
1255+
Z_TYPE_INFO(stmt->fetch.cls.ctor_args) = IS_ARRAY;
12451256
} else {
12461257
ZVAL_UNDEF(&stmt->fetch.cls.ctor_args);
12471258
}
@@ -1343,9 +1354,15 @@ PHP_METHOD(PDOStatement, fetchAll)
13431354
}
13441355

13451356
do_fetch_opt_finish(stmt, 0);
1357+
if (current_ctor) {
1358+
zend_array_release(current_ctor);
1359+
}
13461360

13471361
/* Restore defaults which were changed by PDO_FETCH_CLASS mode */
13481362
stmt->fetch.cls.ce = old_ce;
1363+
/* ctor_args may have been changed to an owned object in the meantime, so destroy it.
1364+
* If it was not, then the type flags update will have protected us against destruction. */
1365+
zval_ptr_dtor(&stmt->fetch.cls.ctor_args);
13491366
ZVAL_COPY_VALUE(&stmt->fetch.cls.ctor_args, &old_ctor_args);
13501367
stmt->fetch.cls.fci.param_count = old_arg_count;
13511368

‎ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch1.phpt

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
--TEST--
2+
PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetch()
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
$db = PDOTest::factory();
17+
18+
class Test {
19+
public string $val1;
20+
public string $val2;
21+
22+
public function __construct(mixed $v) {
23+
var_dump($v);
24+
if ($v instanceof PDOStatement) {
25+
$v->setFetchMode(PDO::FETCH_CLASS, 'Test', [$this->val2]);
26+
}
27+
}
28+
}
29+
30+
$db->exec('CREATE TABLE pdo_fetch_class_change_ctor_one(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))');
31+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_one VALUES(1, 'A', 'alpha')");
32+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_one VALUES(2, 'B', 'beta')");
33+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_one VALUES(3, 'C', 'gamma')");
34+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_one VALUES(4, 'D', 'delta')");
35+
36+
$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_change_ctor_one');
37+
$stmt->setFetchMode(PDO::FETCH_CLASS, 'Test', [$stmt]);
38+
39+
$stmt->execute();
40+
var_dump($stmt->fetch());
41+
42+
?>
43+
--CLEAN--
44+
<?php
45+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
46+
$db = PDOTest::factory();
47+
PDOTest::dropTableIfExists($db, "pdo_fetch_class_change_ctor_one");
48+
?>
49+
--EXPECTF--
50+
object(PDOStatement)#%d (1) {
51+
["queryString"]=>
52+
string(54) "SELECT val1, val2 FROM pdo_fetch_class_change_ctor_one"
53+
}
54+
object(Test)#%d (2) {
55+
["val1"]=>
56+
string(1) "A"
57+
["val2"]=>
58+
string(5) "alpha"
59+
}

‎ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch2.phpt

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
--TEST--
2+
PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetchObject()
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
$db = PDOTest::factory();
17+
18+
class Test {
19+
public string $val1;
20+
public string $val2;
21+
22+
public function __construct(mixed $v) {
23+
var_dump($v);
24+
if ($v instanceof PDOStatement) {
25+
$v->setFetchMode(PDO::FETCH_CLASS, 'Test', [$this->val2]);
26+
}
27+
}
28+
}
29+
30+
// TODO Rename pdo_fetch_class_change_ctor_two table to pdo_fetch_class_change_ctor_two in PHP-8.4
31+
$db->exec('CREATE TABLE pdo_fetch_class_change_ctor_two(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))');
32+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_two VALUES(1, 'A', 'alpha')");
33+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_two VALUES(2, 'B', 'beta')");
34+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_two VALUES(3, 'C', 'gamma')");
35+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_two VALUES(4, 'D', 'delta')");
36+
37+
$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_change_ctor_two');
38+
39+
$stmt->execute();
40+
var_dump($stmt->fetchObject('Test', [$stmt]));
41+
42+
?>
43+
--CLEAN--
44+
<?php
45+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
46+
$db = PDOTest::factory();
47+
PDOTest::dropTableIfExists($db, "pdo_fetch_class_change_ctor_two");
48+
?>
49+
--EXPECTF--
50+
object(PDOStatement)#%s (1) {
51+
["queryString"]=>
52+
string(54) "SELECT val1, val2 FROM pdo_fetch_class_change_ctor_two"
53+
}
54+
object(Test)#%s (2) {
55+
["val1"]=>
56+
string(1) "A"
57+
["val2"]=>
58+
string(5) "alpha"
59+
}

‎ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch3.phpt

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
--TEST--
2+
PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetchAll() (no args variation)
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
$db = PDOTest::factory();
17+
18+
class Test {
19+
public string $val1;
20+
public string $val2;
21+
22+
public function __construct(mixed $v) {
23+
var_dump($v);
24+
if ($v instanceof PDOStatement) {
25+
$v->setFetchMode(PDO::FETCH_CLASS, 'Test', [$this->val2]);
26+
}
27+
}
28+
}
29+
30+
$db->exec('CREATE TABLE pdo_fetch_class_change_ctor_three(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))');
31+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_three VALUES(1, 'A', 'alpha')");
32+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_three VALUES(2, 'B', 'beta')");
33+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_three VALUES(3, 'C', 'gamma')");
34+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_three VALUES(4, 'D', 'delta')");
35+
36+
$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_change_ctor_three');
37+
$stmt->setFetchMode(PDO::FETCH_CLASS, 'Test', [$stmt]);
38+
39+
$stmt->execute();
40+
var_dump($stmt->fetchAll());
41+
42+
?>
43+
--CLEAN--
44+
<?php
45+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
46+
$db = PDOTest::factory();
47+
PDOTest::dropTableIfExists($db, "pdo_fetch_class_change_ctor_three");
48+
?>
49+
--EXPECTF--
50+
object(PDOStatement)#%d (1) {
51+
["queryString"]=>
52+
string(56) "SELECT val1, val2 FROM pdo_fetch_class_change_ctor_three"
53+
}
54+
string(5) "alpha"
55+
string(5) "alpha"
56+
string(5) "alpha"
57+
array(4) {
58+
[0]=>
59+
object(Test)#%d (2) {
60+
["val1"]=>
61+
string(1) "A"
62+
["val2"]=>
63+
string(5) "alpha"
64+
}
65+
[1]=>
66+
object(Test)#%d (2) {
67+
["val1"]=>
68+
string(1) "B"
69+
["val2"]=>
70+
string(4) "beta"
71+
}
72+
[2]=>
73+
object(Test)#%d (2) {
74+
["val1"]=>
75+
string(1) "C"
76+
["val2"]=>
77+
string(5) "gamma"
78+
}
79+
[3]=>
80+
object(Test)#%d (2) {
81+
["val1"]=>
82+
string(1) "D"
83+
["val2"]=>
84+
string(5) "delta"
85+
}
86+
}

‎ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch4.phpt

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
--TEST--
2+
PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetchAll() (args in fetchAll)
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
$db = PDOTest::factory();
17+
18+
class Test {
19+
public string $val1;
20+
public string $val2;
21+
22+
public function __construct(mixed $v) {
23+
var_dump($v);
24+
if ($v instanceof PDOStatement) {
25+
$v->setFetchMode(PDO::FETCH_CLASS, 'Test', [$this->val2]);
26+
}
27+
}
28+
}
29+
30+
$db->exec('CREATE TABLE pdo_fetch_class_change_ctor_four(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))');
31+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_four VALUES(1, 'A', 'alpha')");
32+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_four VALUES(2, 'B', 'beta')");
33+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_four VALUES(3, 'C', 'gamma')");
34+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_four VALUES(4, 'D', 'delta')");
35+
36+
$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_change_ctor_four');
37+
38+
$stmt->execute();
39+
var_dump($stmt->fetchAll(PDO::FETCH_CLASS, 'Test', [$stmt]));
40+
41+
?>
42+
--CLEAN--
43+
<?php
44+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
45+
$db = PDOTest::factory();
46+
PDOTest::dropTableIfExists($db, "pdo_fetch_class_change_ctor_four");
47+
?>
48+
--EXPECTF--
49+
object(PDOStatement)#%d (1) {
50+
["queryString"]=>
51+
string(55) "SELECT val1, val2 FROM pdo_fetch_class_change_ctor_four"
52+
}
53+
string(5) "alpha"
54+
string(5) "alpha"
55+
string(5) "alpha"
56+
array(4) {
57+
[0]=>
58+
object(Test)#%d (2) {
59+
["val1"]=>
60+
string(1) "A"
61+
["val2"]=>
62+
string(5) "alpha"
63+
}
64+
[1]=>
65+
object(Test)#%d (2) {
66+
["val1"]=>
67+
string(1) "B"
68+
["val2"]=>
69+
string(4) "beta"
70+
}
71+
[2]=>
72+
object(Test)#%d (2) {
73+
["val1"]=>
74+
string(1) "C"
75+
["val2"]=>
76+
string(5) "gamma"
77+
}
78+
[3]=>
79+
object(Test)#%d (2) {
80+
["val1"]=>
81+
string(1) "D"
82+
["val2"]=>
83+
string(5) "delta"
84+
}
85+
}

‎ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch5.phpt

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
--TEST--
2+
PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetchAll() (via warning and error handler)
3+
--EXTENSIONS--
4+
pdo
5+
--SKIPIF--
6+
<?php
7+
$dir = getenv('REDIR_TEST_DIR');
8+
if (false == $dir) die('skip no driver');
9+
require_once $dir . 'pdo_test.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
15+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
16+
$db = PDOTest::factory();
17+
18+
// Warning to hook into error handler
19+
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING);
20+
21+
class B {
22+
public function __construct() {}
23+
}
24+
25+
$db->exec('CREATE TABLE pdo_fetch_class_change_ctor_five(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))');
26+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_five VALUES(1, 'A', 'alpha')");
27+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_five VALUES(2, 'B', 'beta')");
28+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_five VALUES(3, 'C', 'gamma')");
29+
$db->exec("INSERT INTO pdo_fetch_class_change_ctor_five VALUES(4, 'D', 'delta')");
30+
31+
$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_change_ctor_five');
32+
$stmt->execute();
33+
34+
function stuffingErrorHandler(int $errno, string $errstr, string $errfile, int $errline) {
35+
global $stmt;
36+
$stmt->setFetchMode(PDO::FETCH_CLASS, 'B', [$errstr]);
37+
echo $errstr, PHP_EOL;
38+
}
39+
set_error_handler(stuffingErrorHandler(...));
40+
41+
var_dump($stmt->fetchAll(PDO::FETCH_CLASS|PDO::FETCH_SERIALIZE, 'B', [$stmt]));
42+
43+
?>
44+
--CLEAN--
45+
<?php
46+
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
47+
$db = PDOTest::factory();
48+
PDOTest::dropTableIfExists($db, "pdo_fetch_class_change_ctor_five");
49+
?>
50+
--EXPECTF--
51+
PDOStatement::fetchAll(): The PDO::FETCH_SERIALIZE mode is deprecated
52+
PDOStatement::fetchAll(): SQLSTATE[HY000]: General error: cannot unserialize class
53+
PDOStatement::fetchAll(): SQLSTATE[HY000]: General error%S
54+
array(0) {
55+
}

0 commit comments

Comments
 (0)