Bug #79820 [Asn->Csd]: Use after free when type duplicated into ReflectionProperty gets resolved

Edit report at https://bugs.php.net/bug.php?id=79820&edit=1 ID: 79820 Updated by: nikic@php.net Reported by: christopher dot broadbent at zencontrol dot com Summary: Use after free when type duplicated into ReflectionProperty gets resolved -Status: Assigned +Status: Closed Type: Bug Package: Reflection related Operating System: linux debian buster PHP Version: 7.4.7 Assigned To: nikic Block user comment: N Private report: N New Comment: Automatic comment on behalf of chris-broadbent@zencontrol.com Revision: http://git.php.net/?p=php-src.git;a=commit;h=ee7c7a8e26b99e3b25a7d41abfe1a2c37b3f6968 Log: Fixed bug #79820 Previous Comments: ------------------------------------------------------------------------ [2020-07-15 08:39:46] nikic@php.net Right you are! Minimal reproducer: <?php class Test { public stdClass $prop; } $rp = new ReflectionProperty(Test::class, 'prop'); $test = new Test; $test->prop = new stdClass; var_dump($rp->getType()->getName()); For PHP 8 this has already been fixed by https://github.com/php/php-src/commit/0e3045ae69d1b80c21b2779df721a4ad8bcda401. ------------------------------------------------------------------------ [2020-07-15 05:44:51] chris-broadbent at zencontrol dot com The following patch has been added/updated: Patch Name: 0001-Add-refs-to-prop-names-to-avoid-use-after-free Revision: 1594791891 URL: https://bugs.php.net/patch-display.php?bug=79820&patch=0001-Add-refs-to-prop-names-to-avoid-use-after-free&revision=1594791891 ------------------------------------------------------------------------ [2020-07-14 23:53:50] christopher dot broadbent at zencontrol dot com I've made a change to php_reflecation.c lines 5281 to 5302, on 7.4.8 release, inside reflection_property____construct reference = (property_reference*) emalloc(sizeof(property_reference)); if (dynam_prop) { reference->prop.flags = ZEND_ACC_PUBLIC; reference->prop.name = name; reference->prop.doc_comment = NULL; reference->prop.ce = ce; reference->dynamic = 1; } else { reference->prop = *property_info; reference->dynamic = 0; + if (ZEND_TYPE_IS_NAME(reference->prop.type)) { + zend_string_addref(ZEND_TYPE_NAME(reference->prop.type)); + } } reference->unmangled_name = zend_string_copy(name); intern->ptr = reference; intern->ref_type = REF_TYPE_PROPERTY; intern->ce = ce; intern->ignore_visibility = 0; } This fixes the segfault in all the reproduction cases we've got, but I'm not familiar enough with the rest of the code to know if this is correct or causes leaks. ------------------------------------------------------------------------ [2020-07-14 23:27:12] christopher dot broadbent at zencontrol dot com Sorry, I meant to say bump the ref counters on reference->prop.type ------------------------------------------------------------------------ [2020-07-14 23:22:18] christopher dot broadbent at zencontrol dot com This is probably my inexperience with the code-base, but shouldn't the implementation for ZEND_METHOD(reflection_property, __construct) by increment the ref-count for the value copied by reference->prop = *property_info; when dynam_prop is false? It seems to be the thing keeping a reference around to the deallocated string, causing the use-after-free, and I can't see anywhere in the code path where it bumps the reference count. ------------------------------------------------------------------------ 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=79820 -- Edit this bug report at https://bugs.php.net/bug.php?id=79820&edit=1