unserialize: Strictly check for :{ at object start
#10214
Conversation
|
@saundefined @adoy @ramsey Is this change acceptable as-is for PHP 8.2? This PR:
It does not introduce a behavioral change to valid serialization data, as emitted by Personally I would prefer to apply this as-is to PHP 8.2 which still is a fairly young branch. If including the full PR is undesired for compat purposes, then I would like to apply the first bullet point to PHP 8.2 at least and the stricter checking to master only. |
|
@TimWolla thanks! I see no reason not to include as is in PHP-8.2, perhaps Ben and Pierrick would disagree with me. |
|
@TimWolla Same as @saundefined it looks good to me. You can go ahead to merge this on 8.2 |
|
Thank you, I'll rebase this later onto PHP-8.2 to run CI again, add NEWS and UPGRADING notes and then merge. |
TimWolla
force-pushed
the
unserialize-strict
branch
from
6bae9ec to
6ddb683
Compare
TimWolla
force-pushed
the
unserialize-strict
branch
from
6ddb683 to
ed84f3a
Compare
It's unlikely that the object syntax error contributed to the actual CVE. The CVE is rather caused by the incorrect object serialization data of the `C` format. Add a second string without such a syntax error to ensure that path is still executed as well to ensure the CVE is absent.
No changes to the input required, because the test actually is intended to verify the behavior for a missing `}`, it's just that the report position changed.
TimWolla
force-pushed
the
unserialize-strict
branch
from
ed84f3a to
e41a8df
Compare
* PHP-8.2:
unserialize: Strictly check for `:{` at object start (#10214)
This change adds validation for the serialization format during object serialization: Checks where missing to verify that objects actually start with
:{, because the object format is not actually part of the grammar, likely because of the classname that is not necessary for arrays.It also fixes a case where
object_custommoved*ppast the string bounds. While the contents at the pointer are not read afterwards, even the creation of such a pointer is technically UB.It's not clear to me what the appropriate target branch would be. I think it should go into PHP 8.2 at least.