Use after free in zval_scan caused by spl_object_storage_get_gc
| Bug #69227 | Use after free in zval_scan caused by spl_object_storage_get_gc | ||||
|---|---|---|---|---|---|
| Submitted: | 2015-03-12 07:50 UTC | Modified: | 2015-03-13 07:46 UTC | ||
| From: | adam at vektah dot net | Assigned: | laruence (profile) | ||
| Status: | Closed | Package: | SPL related | ||
| PHP Version: | 5.5.22 | OS: | ubuntu 14.10 | ||
| Private report: | No | CVE-ID: | None | ||
[2015-03-12 07:50 UTC] adam at vektah dot net
Description:
------------
When an SplObjectStorage instance contains a reference to itself the garbage the garbage collector uses memory that has already been freed.
I have tested this on both
% php --version
PHP 5.5.12-2ubuntu4.2 (cli) (built: Feb 13 2015 18:56:49)
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2014 Zend Technologies
with Zend OPcache v7.0.4-dev, Copyright (c) 1999-2014, by Zend Technologies
% php --version
PHP 5.5.22-1+deb.sury.org~trusty+1 (cli) (built: Feb 20 2015 11:26:12)
Copyright (c) 1997-2015 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2015 Zend Technologies
with Zend OPcache v7.0.4-dev, Copyright (c) 1999-2015, by Zend Technologies
It appears that the gc scan phase keeps a reference to the properties of the object while recursing over the children. The spl_object_storage_get_gc method then cleans the underlying hash leaving the caller with an invalid pointer.
It is quite difficult to get a reliable segfault out in a short test, but valgrind can see the issue as long as USE_ZEND_ALLOC=0
Test script:
---------------
<?php
$s = new SplObjectStorage();
$s->attach($s);
gc_collect_cycles();
Expected result:
----------------
No use after free, no segfault.
Actual result:
--------------
5.5.22:
% USE_ZEND_ALLOC=0 valgrind --track-origins=yes php test.php
==7739== Memcheck, a memory error detector
==7739== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==7739== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright info
==7739== Command: php test.php
==7739==
==7739== Warning: set address range perms: large range [0x1614c000, 0x3614c000) (defined)
==7739== Invalid read of size 8
==7739== at 0x6F066D: zval_scan (in /usr/bin/php5)
==7739== by 0x6F0B9C: gc_collect_cycles (in /usr/bin/php5)
==7739== by 0x6E2628: zif_gc_collect_cycles (in /usr/bin/php5)
==7739== by 0x6C19DA: dtrace_execute_internal (in /usr/bin/php5)
==7739== by 0x781FC4: zend_do_fcall_common_helper_SPEC (in /usr/bin/php5)
==7739== by 0x6FBC97: execute_ex (in /usr/bin/php5)
==7739== by 0x6C18D8: dtrace_execute_ex (in /usr/bin/php5)
==7739== by 0x78260F: zend_do_fcall_common_helper_SPEC (in /usr/bin/php5)
==7739== by 0x6FBC97: execute_ex (in /usr/bin/php5)
==7739== by 0x6C18D8: dtrace_execute_ex (in /usr/bin/php5)
==7739== by 0x6D353F: zend_execute_scripts (in /usr/bin/php5)
==7739== by 0x673084: php_execute_script (in /usr/bin/php5)
==7739== Address 0x156fe700 is 32 bytes inside a block of size 72 free'd
==7739== at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7739== by 0x6DF8BD: zend_hash_clean (in /usr/bin/php5)
==7739== by 0x5DB66C: spl_object_storage_get_gc (in /usr/bin/php5)
==7739== by 0x6F0093: zval_scan_black (in /usr/bin/php5)
==7739== by 0x6F0574: zval_scan (in /usr/bin/php5)
==7739== by 0x6F066C: zval_scan (in /usr/bin/php5)
==7739== by 0x6F0B9C: gc_collect_cycles (in /usr/bin/php5)
==7739== by 0x6E2628: zif_gc_collect_cycles (in /usr/bin/php5)
==7739== by 0x6C19DA: dtrace_execute_internal (in /usr/bin/php5)
==7739== by 0x781FC4: zend_do_fcall_common_helper_SPEC (in /usr/bin/php5)
==7739== by 0x6FBC97: execute_ex (in /usr/bin/php5)
==7739== by 0x6C18D8: dtrace_execute_ex (in /usr/bin/php5)
5.5.12:
% USE_ZEND_ALLOC=0 valgrind --track-origins=yes php test2.php
==10230== Memcheck, a memory error detector
==10230== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==10230== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==10230== Command: php test2.php
==10230==
==10230== Invalid read of size 8
==10230== at 0x71CE25: zval_scan (zend_gc.c:570)
==10230== by 0x71D39C: zobj_scan (zend_gc.c:605)
==10230== by 0x71D39C: gc_scan_roots (zend_gc.c:625)
==10230== by 0x71D39C: gc_collect_cycles (zend_gc.c:796)
==10230== by 0x70E098: zif_gc_collect_cycles (zend_builtin_functions.c:361)
==10230== by 0x6EC6A9: dtrace_execute_internal (zend_dtrace.c:97)
==10230== by 0x7B67DD: zend_do_fcall_common_helper_SPEC (zend_vm_execute.h:552)
==10230== by 0x728D2F: execute_ex (zend_vm_execute.h:363)
==10230== by 0x6EC547: dtrace_execute_ex (zend_dtrace.c:73)
==10230== by 0x6FE27F: zend_execute_scripts (zend.c:1316)
==10230== by 0x69C08A: php_execute_script (main.c:2506)
==10230== by 0x7B887F: do_cli (php_cli.c:994)
==10230== by 0x461AAC: main (php_cli.c:1378)
==10230== Address 0xfdd00c0 is 32 bytes inside a block of size 72 free'd
==10230== at 0x4C2BE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==10230== by 0x70AFCD: zend_hash_clean (zend_hash.c:601)
==10230== by 0x5FE384: spl_object_storage_get_gc (spl_observer.c:382)
==10230== by 0x71C7BB: zval_scan_black (zend_gc.c:288)
==10230== by 0x71CD34: zval_scan (zend_gc.c:519)
==10230== by 0x71CE24: zval_scan (zend_gc.c:568)
==10230== by 0x71D39C: zobj_scan (zend_gc.c:605)
==10230== by 0x71D39C: gc_scan_roots (zend_gc.c:625)
==10230== by 0x71D39C: gc_collect_cycles (zend_gc.c:796)
==10230== by 0x70E098: zif_gc_collect_cycles (zend_builtin_functions.c:361)
==10230== by 0x6EC6A9: dtrace_execute_internal (zend_dtrace.c:97)
==10230== by 0x7B67DD: zend_do_fcall_common_helper_SPEC (zend_vm_execute.h:552)
==10230== by 0x728D2F: execute_ex (zend_vm_execute.h:363)
==10230== by 0x6EC547: dtrace_execute_ex (zend_dtrace.c:73)
Patches
Pull Requests
History
AllCommentsChangesGit/SVN commits
[2015-03-12 09:04 UTC] laruence@php.net
-Status: Open +Status: Verified -Assigned To: +Assigned To: laruence
[2015-03-12 09:04 UTC] laruence@php.net
[2015-03-12 10:18 UTC] laruence@php.net
A quick fix could be: diff --git a/ext/spl/spl_observer.c b/ext/spl/spl_observer.c index 5e21088..4fc0b6f 100644 --- a/ext/spl/spl_observer.c +++ b/ext/spl/spl_observer.c @@ -378,6 +378,10 @@ static HashTable *spl_object_storage_get_gc(zval *obj, zval ***table, int *n TSR /* clean \x00gcdata, as it may be out of date */ if (zend_hash_find(props, "\x00gcdata", sizeof("\x00gcdata"), (void**) &gcdata_arr_pp) == SUCCESS) { + if (Z_REFCOUNT_PP(gcdata_arr_pp) == 0) { + /* this is going to be freed by gc, don't make a new one */ + return props; + } gcdata_arr = *gcdata_arr_pp; zend_hash_clean(Z_ARRVAL_P(gcdata_arr)); } but let me think of side affects...