Zend: Fix GH-22257 type confusion in Exception::getTraceAsString().#22263
Zend: Fix GH-22257 type confusion in Exception::getTraceAsString().#22263devnexen wants to merge 4 commits into
Conversation
A crafted, deliberately truncated unserialize() payload can leave Exception::$trace holding a non-array value, since the typed-property check is skipped on the parse failure path. getTraceAsString() then reinterpreted the object as a HashTable, causing an out-of-bounds read. Guard against a non-array trace and return an empty string instead. Fix php#22257
devnexen
changed the title
|
Shouldn't this throw instead? I don't think it makes sense trying to support/continue operating on maliciously-crafted code. |
|
yes, you re not wrong. |
|
Hello @ndossche @devnexen Thank you in advance! |
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Actually, $trace is defined with type array, so it should not be possible to break that constraint. This should be blocked instead of adding a check in getTraceAsString
|
To clarify, if I reset to the |
Yeah it should not be allowed to violate the property type so that seems better. |
| * declared type so we restore the default | ||
| */ | ||
| zval *tmp = &obj->ce->default_properties_table[OBJ_PROP_TO_NUM(info->offset)]; | ||
| zval_ptr_dtor(data); |
There was a problem hiding this comment.
I assume evil destructors can cause issues here, so a temporary copy may be necessary (i.e. the zval garbage; trick)
I wonder though if "restoring after violation" is the right approach, but that may be a much bigger change
There was a problem hiding this comment.
Went with var_push_dtor_value rather than the inline zval garbage, it moves the old value into the deferred-dtor list. So nothing is destroyed during the unwind or the delayed calls wdyt ?
3 participants
A crafted, deliberately truncated unserialize() payload can leave Exception::$trace holding a non-array value, since the typed-property check is skipped on the parse failure path. getTraceAsString() then reinterpreted the object as a HashTable, causing an out-of-bounds read. Guard against a non-array trace and return an empty string instead.
Fix #22257