diff --git a/NEWS b/NEWS index 7587ada8216..ac5d3d4c01a 100644 --- a/NEWS +++ b/NEWS @@ -74,6 +74,7 @@ PHP NEWS - PDO: . Fixed a memory leak when the GC is used to free a PDOStatment. (Girgias) . Fixed a crash in the PDO Firebird Statement destructor. (nielsdos) + . Fixed UAFs when changing default fetch class ctor args. (Girgias, nielsdos) - PgSql: . Fixed build failure when the constant PGRES_TUPLES_CHUNK is not present diff --git a/ext/pdo/pdo_stmt.c b/ext/pdo/pdo_stmt.c index 965311be7bc..94106b715ef 100644 --- a/ext/pdo/pdo_stmt.c +++ b/ext/pdo/pdo_stmt.c @@ -1247,6 +1247,7 @@ PHP_METHOD(PDOStatement, fetchAll) zval *arg2 = NULL; zend_class_entry *old_ce; zval old_ctor_args, *ctor_args = NULL; + HashTable *current_ctor = NULL; bool error = false; int flags, old_arg_count; @@ -1264,6 +1265,10 @@ PHP_METHOD(PDOStatement, fetchAll) old_ce = stmt->fetch.cls.ce; ZVAL_COPY_VALUE(&old_ctor_args, &stmt->fetch.cls.ctor_args); + if (Z_TYPE(old_ctor_args) == IS_ARRAY) { + /* Protect against destruction by marking this as immutable: we consider this non-owned temporarily */ + Z_TYPE_INFO(stmt->fetch.cls.ctor_args) = IS_ARRAY; + } old_arg_count = stmt->fetch.cls.fci.param_count; do_fetch_opt_finish(stmt, 0); @@ -1288,7 +1293,13 @@ PHP_METHOD(PDOStatement, fetchAll) } if (ctor_args && zend_hash_num_elements(Z_ARRVAL_P(ctor_args)) > 0) { - ZVAL_COPY_VALUE(&stmt->fetch.cls.ctor_args, ctor_args); /* we're not going to free these */ + /* We increase the refcount and store it in case usercode has been messing around with the ctor args. + * We need to store current_ctor separately as usercode may change the ctor_args which will cause a leak. */ + current_ctor = Z_ARRVAL_P(ctor_args); + ZVAL_COPY(&stmt->fetch.cls.ctor_args, ctor_args); + /* Protect against destruction by marking this as immutable: we consider this non-owned + * as destruction is handled via current_ctor. */ + Z_TYPE_INFO(stmt->fetch.cls.ctor_args) = IS_ARRAY; } else { ZVAL_UNDEF(&stmt->fetch.cls.ctor_args); } @@ -1360,6 +1371,7 @@ PHP_METHOD(PDOStatement, fetchAll) } PDO_STMT_CLEAR_ERR(); + if ((how & PDO_FETCH_GROUP) || how == PDO_FETCH_KEY_PAIR || (how == PDO_FETCH_USE_DEFAULT && stmt->default_fetch_type == PDO_FETCH_KEY_PAIR) ) { @@ -1384,9 +1396,15 @@ PHP_METHOD(PDOStatement, fetchAll) } do_fetch_opt_finish(stmt, 0); + if (current_ctor) { + zend_array_release(current_ctor); + } /* Restore defaults which were changed by PDO_FETCH_CLASS mode */ stmt->fetch.cls.ce = old_ce; + /* ctor_args may have been changed to an owned object in the meantime, so destroy it. + * If it was not, then the type flags update will have protected us against destruction. */ + zval_ptr_dtor(&stmt->fetch.cls.ctor_args); ZVAL_COPY_VALUE(&stmt->fetch.cls.ctor_args, &old_ctor_args); stmt->fetch.cls.fci.param_count = old_arg_count; diff --git a/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch1.phpt b/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch1.phpt new file mode 100644 index 00000000000..194610b1119 --- /dev/null +++ b/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch1.phpt @@ -0,0 +1,59 @@ +--TEST-- +PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetch() +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setFetchMode(PDO::FETCH_CLASS, 'Test', [$this->val2]); + } + } +} + +$db->exec('CREATE TABLE pdo_fetch_class_change_ctor_one(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))'); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_one VALUES(1, 'A', 'alpha')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_one VALUES(2, 'B', 'beta')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_one VALUES(3, 'C', 'gamma')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_one VALUES(4, 'D', 'delta')"); + +$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_change_ctor_one'); +$stmt->setFetchMode(PDO::FETCH_CLASS, 'Test', [$stmt]); + +$stmt->execute(); +var_dump($stmt->fetch()); + +?> +--CLEAN-- + +--EXPECTF-- +object(PDOStatement)#%d (1) { + ["queryString"]=> + string(54) "SELECT val1, val2 FROM pdo_fetch_class_change_ctor_one" +} +object(Test)#%d (2) { + ["val1"]=> + string(1) "A" + ["val2"]=> + string(5) "alpha" +} diff --git a/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch2.phpt b/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch2.phpt new file mode 100644 index 00000000000..f2cc4a29469 --- /dev/null +++ b/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch2.phpt @@ -0,0 +1,59 @@ +--TEST-- +PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetchObject() +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setFetchMode(PDO::FETCH_CLASS, 'Test', [$this->val2]); + } + } +} + +// TODO Rename pdo_fetch_class_change_ctor_two table to pdo_fetch_class_change_ctor_two in PHP-8.4 +$db->exec('CREATE TABLE pdo_fetch_class_change_ctor_two(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))'); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_two VALUES(1, 'A', 'alpha')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_two VALUES(2, 'B', 'beta')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_two VALUES(3, 'C', 'gamma')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_two VALUES(4, 'D', 'delta')"); + +$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_change_ctor_two'); + +$stmt->execute(); +var_dump($stmt->fetchObject('Test', [$stmt])); + +?> +--CLEAN-- + +--EXPECTF-- +object(PDOStatement)#%s (1) { + ["queryString"]=> + string(54) "SELECT val1, val2 FROM pdo_fetch_class_change_ctor_two" +} +object(Test)#%s (2) { + ["val1"]=> + string(1) "A" + ["val2"]=> + string(5) "alpha" +} diff --git a/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch3.phpt b/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch3.phpt new file mode 100644 index 00000000000..6b2fb788700 --- /dev/null +++ b/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch3.phpt @@ -0,0 +1,86 @@ +--TEST-- +PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetchAll() (no args variation) +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setFetchMode(PDO::FETCH_CLASS, 'Test', [$this->val2]); + } + } +} + +$db->exec('CREATE TABLE pdo_fetch_class_change_ctor_three(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))'); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_three VALUES(1, 'A', 'alpha')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_three VALUES(2, 'B', 'beta')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_three VALUES(3, 'C', 'gamma')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_three VALUES(4, 'D', 'delta')"); + +$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_change_ctor_three'); +$stmt->setFetchMode(PDO::FETCH_CLASS, 'Test', [$stmt]); + +$stmt->execute(); +var_dump($stmt->fetchAll()); + +?> +--CLEAN-- + +--EXPECTF-- +object(PDOStatement)#%d (1) { + ["queryString"]=> + string(56) "SELECT val1, val2 FROM pdo_fetch_class_change_ctor_three" +} +string(5) "alpha" +string(5) "alpha" +string(5) "alpha" +array(4) { + [0]=> + object(Test)#%d (2) { + ["val1"]=> + string(1) "A" + ["val2"]=> + string(5) "alpha" + } + [1]=> + object(Test)#%d (2) { + ["val1"]=> + string(1) "B" + ["val2"]=> + string(4) "beta" + } + [2]=> + object(Test)#%d (2) { + ["val1"]=> + string(1) "C" + ["val2"]=> + string(5) "gamma" + } + [3]=> + object(Test)#%d (2) { + ["val1"]=> + string(1) "D" + ["val2"]=> + string(5) "delta" + } +} diff --git a/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch4.phpt b/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch4.phpt new file mode 100644 index 00000000000..d4bad52a2b5 --- /dev/null +++ b/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch4.phpt @@ -0,0 +1,85 @@ +--TEST-- +PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetchAll() (args in fetchAll) +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setFetchMode(PDO::FETCH_CLASS, 'Test', [$this->val2]); + } + } +} + +$db->exec('CREATE TABLE pdo_fetch_class_change_ctor_four(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))'); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_four VALUES(1, 'A', 'alpha')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_four VALUES(2, 'B', 'beta')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_four VALUES(3, 'C', 'gamma')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_four VALUES(4, 'D', 'delta')"); + +$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_change_ctor_four'); + +$stmt->execute(); +var_dump($stmt->fetchAll(PDO::FETCH_CLASS, 'Test', [$stmt])); + +?> +--CLEAN-- + +--EXPECTF-- +object(PDOStatement)#%d (1) { + ["queryString"]=> + string(55) "SELECT val1, val2 FROM pdo_fetch_class_change_ctor_four" +} +string(5) "alpha" +string(5) "alpha" +string(5) "alpha" +array(4) { + [0]=> + object(Test)#%d (2) { + ["val1"]=> + string(1) "A" + ["val2"]=> + string(5) "alpha" + } + [1]=> + object(Test)#%d (2) { + ["val1"]=> + string(1) "B" + ["val2"]=> + string(4) "beta" + } + [2]=> + object(Test)#%d (2) { + ["val1"]=> + string(1) "C" + ["val2"]=> + string(5) "gamma" + } + [3]=> + object(Test)#%d (2) { + ["val1"]=> + string(1) "D" + ["val2"]=> + string(5) "delta" + } +} diff --git a/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch5.phpt b/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch5.phpt new file mode 100644 index 00000000000..533a61410c4 --- /dev/null +++ b/ext/pdo/tests/pdo_fetch_class_change_ctor_args_during_fetch5.phpt @@ -0,0 +1,55 @@ +--TEST-- +PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetchAll() (via warning and error handler) +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING); + +class B { + public function __construct() {} +} + +$db->exec('CREATE TABLE pdo_fetch_class_change_ctor_five(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))'); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_five VALUES(1, 'A', 'alpha')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_five VALUES(2, 'B', 'beta')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_five VALUES(3, 'C', 'gamma')"); +$db->exec("INSERT INTO pdo_fetch_class_change_ctor_five VALUES(4, 'D', 'delta')"); + +$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_change_ctor_five'); +$stmt->execute(); + +function stuffingErrorHandler(int $errno, string $errstr, string $errfile, int $errline) { + global $stmt; + $stmt->setFetchMode(PDO::FETCH_CLASS, 'B', [$errstr]); + echo $errstr, PHP_EOL; +} +set_error_handler(stuffingErrorHandler(...)); + +var_dump($stmt->fetchAll(PDO::FETCH_CLASS|PDO::FETCH_SERIALIZE, 'B', [$stmt])); + +?> +--CLEAN-- + +--EXPECTF-- +PDOStatement::fetchAll(): The PDO::FETCH_SERIALIZE mode is deprecated +PDOStatement::fetchAll(): SQLSTATE[HY000]: General error: cannot unserialize class +PDOStatement::fetchAll(): SQLSTATE[HY000]: General error%S +array(0) { +} diff --git a/ext/pdo/tests/pdo_fetch_class_cyclic_ctor.phpt b/ext/pdo/tests/pdo_fetch_class_cyclic_ctor.phpt new file mode 100644 index 00000000000..48c10c01f78 --- /dev/null +++ b/ext/pdo/tests/pdo_fetch_class_cyclic_ctor.phpt @@ -0,0 +1,59 @@ +--TEST-- +PDO Common: PDO::FETCH_CLASS with a constructor that changes the ctor args within PDO::fetch() +--EXTENSIONS-- +pdo +--SKIPIF-- + +--FILE-- +exec('CREATE TABLE pdo_fetch_class_cyclic_ctor(id int NOT NULL PRIMARY KEY, val1 VARCHAR(10), val2 VARCHAR(10))'); +$db->exec("INSERT INTO pdo_fetch_class_cyclic_ctor VALUES(1, 'A', 'alpha')"); +$db->exec("INSERT INTO pdo_fetch_class_cyclic_ctor VALUES(2, 'B', 'beta')"); +$db->exec("INSERT INTO pdo_fetch_class_cyclic_ctor VALUES(3, 'C', 'gamma')"); +$db->exec("INSERT INTO pdo_fetch_class_cyclic_ctor VALUES(4, 'D', 'delta')"); + +$args = []; +$args[] = &$args; + +$stmt = $db->prepare('SELECT val1, val2 FROM pdo_fetch_class_cyclic_ctor'); +$stmt->setFetchMode(PDO::FETCH_CLASS, 'Test', [$args]); + +$stmt->execute(); +var_dump($stmt->fetch()); + +?> +--CLEAN-- + +--EXPECTF-- +array(1) { + [0]=> + *RECURSION* +} +object(Test)#%d (2) { + ["val1"]=> + string(1) "A" + ["val2"]=> + string(5) "alpha" +}