Repository navigation
zend_array_try_init() with dtor can cause engine UAF #17162
Description
Description
The following code:
<?php
$date_string = '00.00.0000 - 00:00:00';
$fusion = $date_string;
class Box {
}
class Test {
function __destruct() {
global $box;
$box->value = null;
}
}
function test($box) {
var_dump($box->value = new Test);
}
$box = new Box();
test($box);
try {mb_eregi($fusion,$date_string,$box);} catch (Exception $e) { echo($e); }Resulted in this output:
/home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_hash.c(2672) : ht=0x7ff28984b360 is being destroyed
php: /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_hash.c:74: void _zend_is_inconsistent(const HashTable *, const char *, int): Assertion `0' failed.
Aborted (core dumped)
PHP Version
nightly
Operating System
No response
Activity
ndossche commentedon Dec 15, 2024
I'm not a huge fan of adding these engine guards because they complicate things and make slowdowns. This one may be okay because it's never really done on a hot code path AFAIK.
Basically we make the object inaccessible prior to destruction such that it is protected against the overwrite. This is also consistent with the behaviour if we were to comment out the call to
mb_eregi.cc @iluuu1994 Do you have opinions?
iluuu1994 commentedon Dec 15, 2024
I don't mind this fix, but I agree that it's not a great approach long-term. Maybe static analysis could help detecting these cases, and then we can decide whether it's worth applying fixes, ignoring the issue or looking for an alternative solution, e.g. by delaying all destructor calls to some specified point-in-time. We may try to delay them until the zend_interrupt_helper, but obviously this is a behavioral change and may break some use-cases.
ndossche commentedon Dec 15, 2024
right-click > save
Yeah, okay, I'll open a PR.
Would fix the ones in the VM too.
I don't know if you remember the implicit move optimization PR I did, but there was some critique from someone from the Wikimedia team because they actually rely in many places on destructor calls happening at certain points in time. So it may be that the delay solution is also a no-go.
iluuu1994 commentedon Dec 15, 2024
I do remember. That said, it's extremely easy for this to break. E.g. accidentally store a non-static closure as a property, and you're relying on the cycle collector. It would be helpful to consider a specific case and see if there's an alternative solution we could offer.
ndossche commentedon Dec 15, 2024
I agree. It seems they're doing it for RAII/defer like purposes. I don't expect first-class "defer" support to be a hard thing to implement (famous last words).
[-]Assertion failure Zend/zend_hash.c:74[/-][+]zend_array_try_init() with dtor can cause engine UAF[/+]Fix phpGH-17162: zend_array_try_init() with dtor can cause engine UAF
Fix GH-17162: zend_array_try_init() with dtor can cause engine UAF
Merge branch 'PHP-8.3' into PHP-8.4
Merge branch 'PHP-8.4'