Fix #81992: SplFixedArray::setSize() causes use-after-free #11959
Conversation
Upon resizing, the elements are destroyed from lower index to higher index. When an element refers to an object with a destructor, it can refer to a lower (i.e. already destroyed) element, causing a uaf. Set refcounted zvals to NULL after destroying them to avoid a uaf.
| if (size == 0) { | ||
| spl_fixedarray_dtor(array); | ||
| array->elements = NULL; | ||
| array->size = 0; |
There was a problem hiding this comment.
Shouldn't this be set before spl_fixedarray_dtor to avoid the same issue on deallocation of the whole array? Edit: Ah, we're using size in there. Thinking about it, we might even do other shenanigans like modify the array in the destructor of released objects...
There was a problem hiding this comment.
I'll have a closer look, I'm currently fixing the case of calling resize within resize, still causing uaf :/ I think I have a solution for that though.
I'll have a deeper look at your comment after that.
|
I've pushed a working fix for the resize UAF. My solution is the following: |
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
LGTM, I tried to abuse it in various ways but was unable to do so.
|
@iluuu1994 as usual, thanks a lot for the sanity check ^^ ! |
Upon resizing, the elements are destroyed from lower index to higher index. When an element refers to an object with a destructor, it can refer to a lower (i.e. already destroyed) element, causing a uaf. Set refcounted zvals to NULL after destroying them to avoid a uaf. Closes phpGH-11959.
2 participants
Upon resizing, the elements are destroyed from lower index to higher index. When an element contains an object with a destructor, that destructor can access a lower (i.e. already destroyed) element, causing a uaf. Set refcounted zvals to NULL after destroying them to avoid a uaf.
Alternative solution I thought about: Maybe it's possible to do it in two passes. First run all destructors and then destroy everything. But I don't know off the top of my head if the engine supports that (in an easy way). It seems complicated, and would also be technically a BC break because the destructors see still alive data that they might've expected to be destroyed.
The solution in this PR is BC-preserving because it previously just crashed instead of doing anything useful.