Bug #81705 [Ver]: type confusion/UAF on set_error_handler with concat operation

Edit report at https://bugs.php.net/bug.php?id=81705&edit=1 ID: 81705 Updated by: cmb@php.net Reported by: yukik at ricsec dot co dot jp Summary: type confusion/UAF on set_error_handler with concat operation Status: Verified Type: Bug Package: Scripting Engine problem Operating System: Linux PHP Version: 8.0.14 Block user comment: N Private report: N New Comment: The problem is that result gets released[1] if it is identical to op1_orig (which is always the case for the concat assign operator). For the script from comment 1641358352[2], that decreases the refcount to zero, but on shutdown, the literal stored in the op array will be released again. If that script is modified to use a dynamic value (range(1,4) instead of [1,2,3,4]), its is already freed, when that code in concat_function() tries to release it again. [1] <https://github.com/php/php-src/blob/php-8.1.1/Zend/zend_operators.c#L1928> [2] <https://bugs.php.net/bug.php?id=81705#1641358352> Previous Comments: ------------------------------------------------------------------------ [2022-01-06 09:42:54] yukik at ricsec dot co dot jp > Contrary to the type confusion, which my patch would solve, the UAF scenario is way more tricky. Oh, is there any case where where not only op1.value but also op1 itself will be freed? If BC matters, then it seems we have to carefully increment/decrement refcounts of them... ------------------------------------------------------------------------ [2022-01-05 15:13:52] cmb@php.net Contrary to the type confusion, which my patch would solve, the UAF scenario is way more tricky. I don't see a way to cleanly solve that, besides throwing an exception instead of raising a warning for attempted array to string conversion, but we cannot do that for BC reasons. We also cannot simply suppress the warning while doing concat_function(), because there are many legitimate cases where that might be a relevant warning, not causing any particular issues. ------------------------------------------------------------------------ [2022-01-05 04:52:32] yukik at ricsec dot co dot jp Hello, cmb and stas First of all, I appreciate your very fast replies. And cmb's patch looks great to me. I was afraid of causing another UAF by using op1 and op2 in the validation, but I realized op1->u1.v.type won't be freed as I watched your patch. I'm just wondering if this is actually not a security issue. Of course, I've read https://wiki.php.net/security before submitting this. As you pointed out, definitely this should not be considered as High severity. But at the same time, this issue doesn't meet the conditions written in the section "Not a security issue". The bug just requires some uncommon code pattern. So I think this bug has Medium or Low secerity. And, I might have caused some misleadings by attaching a too long PoC for arbitrary memory write. To cause a UAF, just the following 4 lines $my_var = [[1,2,3,4],[1,2,3,4]]; set_error_handler(function() use(&$my_var,&$buf){ $my_var=1; }); $my_var[1] .= 1234; are enough. If this kind of code pattern can be seen in a product, that immediately leads to UAF(there is no need for attackers to set his own error handler). There might be another simple way to abuse this. My PoC was just too long. I would appreciate if I can hear your opinion considering these. ​ I respect your decision no matter what it is. ------------------------------------------------------------------------ [2022-01-05 02:22:56] stas@php.net Definitely not a security issue, requires special complicated code to trigger. ------------------------------------------------------------------------ [2022-01-04 12:26:56] cmb@php.net Thanks for reporting! I can reproduce the segfault; it wouldn't happen with the given test script if the optimizer is enabled, but a slight variation can work around that. It seems to me the most straightforward fix is to check that op1 and op2 are actually IS_STRING before we assume they are, and bail out otherwise. Suggested patch for PHP-8.0: <https://gist.github.com/cmb69/05f18d11ac5cf4c70d3514f24787f087> Given that the exploit requires pathological code, I don't think that qualifies as security issue according to our classification[1]. I'll let that for others to decide. If it is a security issue, PHP-7.4 likely needs to be fixed as well. [1] <https://wiki.php.net/security> ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=81705 -- Edit this bug report at https://bugs.php.net/bug.php?id=81705&edit=1