Bug #81705 [Com]: 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 Comment by: robertnldspj11 at gmail dot com 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: A commitment of appreciation is all together for the update and quick reply. Bet upon your string. A commitment of appreciation is all together for making it. Appreciative for sharing such mind blowing information. (https://www.myeclass.me/)github.com Previous Comments: ------------------------------------------------------------------------ [2022-12-22 11:08:27] janettabloomquist at gmail dot com concat_function, however, implicitly assumes the variables op1 and op2 are always strings, and thus type confusion occurs as a result. UAF due to php_filter_float() failing for ints (CVE-2021-21708) ... (Potential type confusion in unixtojd() parameter parsing). (https://www.myhealthonline.biz/)github.com ------------------------------------------------------------------------ [2022-01-06 22:45:18] cmb@php.net 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> ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ 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