Fix GH-22046: The unserialize function with Uri\WhatWg\Url leads to NULL pointer dereference when object serialized back#22058
Fix GH-22046: The unserialize function with Uri\WhatWg\Url leads to NULL pointer dereference when object serialized back#22058kocsismate wants to merge 2 commits into
Conversation
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
As mentioned in the issue, this should get a general fix. But the patch LGTM if we decide for some reason not to fix this generally.
kocsismate
requested review from
DanielEScherzer,
Girgias,
NattyNarwhal,
SakiTakamachi,
adoy,
alexdowad,
arnaud-lb,
bukka,
derickr,
devnexen,
dstogov,
kamil-tekiela,
youkidearitai and
zeriyoshi
as code owners
github-actions
Bot
added
Extension: ldap
Extension: libxml
Extension: mbstring
Extension: mysqli
Extension: mysqlnd
Extension: odbc
Extension: opcache
Extension: pcntl
Extension: pcre
Extension: pdo (core)
Extension: pdo_dblib
Extension: pdo_firebird
Extension: pdo_mysql
Extension: pdo_odbc
Extension: pdo_pgsql
Extension: pdo_sqlite
Extension: pgsql
Extension: phar
Extension: posix
Extension: readline
Extension: reflection
Extension: session
Extension: shmop
Extension: simplexml
labels
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
IMO, fuzzing-style issues (things that are effectively impossible to happen by accident) should be fixed on master, assuming they don't pose a security risk (which this does not).
Yes, I think you are right! So I changed the target branch to master. |
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
PR should be retitled to indicate that it's not a fix that is specific to ext/uri. UPGRADING and NEWS should be added. But the change itself LGTM.
| zend_error(E_WARNING, "Class %s has no unserializer", ZSTR_VAL(ce->name)); | ||
| object_init_ex(rval, ce); | ||
| zend_throw_exception_ex(NULL, 0, "Class %s has no unserializer", ZSTR_VAL(ce->name)); | ||
| return 0; |
There was a problem hiding this comment.
On a second thought: Just E_WARNING + return 0; should be sufficient here. The error handling in unserialize() is pretty broken, but the return 0; should keep unserialize(…) === false from working, whereas an exception might not.
See also: https://wiki.php.net/rfc/improve_unserialize_error_handling
3 participants
Unserializing from the "C" format is explicitly disabled.