Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory management is broken for bad dictionaries #16326

Closed
YuanchengJiang opened this issue Oct 9, 2024 · 3 comments
Closed

Memory management is broken for bad dictionaries #16326

YuanchengJiang opened this issue Oct 9, 2024 · 3 comments

Comments

@YuanchengJiang
Copy link

Description

The following code:

<?php
require_once __DIR__ . DIRECTORY_SEPARATOR . 'test_offset_helpers.inc';
$fusion = $offsets;
$r = deflate_init(ZLIB_ENCODING_DEFLATE, ["dictionary" => $fusion]);

Resulted in this output:

=================================================================
==2278709==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000000264 at pc 0x55876106cf42 bp 0x7ffd9ea46bb0 sp 0x7ffd9ea46ba8
READ of size 4 at 0x603000000264 thread T0
    #0 0x55876106cf41 in zend_mm_free_heap /php-src/Zend/zend_alloc.c:1528:28
    #1 0x558761072825 in _efree /php-src/Zend/zend_alloc.c:2751:2
    #2 0x55875e2bcdb2 in zlib_create_dictionary_string /php-src/ext/zlib/zlib.c:816:9
    #3 0x55875e2c41af in zif_deflate_init /php-src/ext/zlib/zlib.c:1134:7
    #4 0x55876190eda3 in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /php-src/Zend/zend_vm_execute.h:1363:2
    #5 0x558761443713 in execute_ex /php-src/Zend/zend_vm_execute.h:58565:7
    #6 0x558761445872 in zend_execute /php-src/Zend/zend_vm_execute.h:64217:2
    #7 0x558762145b01 in zend_execute_script /php-src/Zend/zend.c:1928:3
    #8 0x558760a56798 in php_execute_script_ex /php-src/main/main.c:2574:13
    #9 0x558760a57858 in php_execute_script /php-src/main/main.c:2614:9
    #10 0x558762159276 in do_cli /php-src/sapi/cli/php_cli.c:935:5
    #11 0x558762153944 in main /php-src/sapi/cli/php_cli.c:1310:18
    #12 0x7f44ab6a7d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 490fef8403240c91833978d494d39e537409b92e)
    #13 0x7f44ab6a7e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 490fef8403240c91833978d494d39e537409b92e)
    #14 0x55875da06db4 in _start (/php-src/sapi/cli/php+0x2606db4) (BuildId: 10f0f1f073ebe7bcaf9944c9da559b10d59463b0)

0x603000000264 is located 2 bytes to the right of 18-byte region [0x603000000250,0x603000000262)
allocated by thread T0 here:
    #0 0x55875da8c7de in malloc (/php-src/sapi/cli/php+0x268c7de) (BuildId: 10f0f1f073ebe7bcaf9944c9da559b10d59463b0)
    #1 0x7f44aaa9fee7  (/lib/x86_64-linux-gnu/libtasn1.so.6+0x3ee7) (BuildId: 2fde6ecb43c586fe4077118f771077aa1298e7ea)

SUMMARY: AddressSanitizer: heap-buffer-overflow /php-src/Zend/zend_alloc.c:1528:28 in zend_mm_free_heap
Shadow bytes around the buggy address:
  0x0c067fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c067fff8000: fa fa fd fd fd fa fa fa 00 00 00 00 fa fa 00 00
  0x0c067fff8010: 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00 fa fa
  0x0c067fff8020: 00 00 00 fa fa fa 00 00 00 fa fa fa 00 00 00 fa
  0x0c067fff8030: fa fa 00 00 00 fa fa fa 00 00 00 fa fa fa 00 00
=>0x0c067fff8040: 00 fa fa fa 00 00 00 02 fa fa 00 00[02]fa fa fa
  0x0c067fff8050: 00 00 06 fa fa fa 00 00 00 02 fa fa 00 00 02 fa
  0x0c067fff8060: fa fa 00 00 00 02 fa fa 00 00 00 02 fa fa 00 00
  0x0c067fff8070: 06 fa fa fa 00 00 04 fa fa fa 00 00 00 fa fa fa
  0x0c067fff8080: 00 00 04 fa fa fa 00 00 05 fa fa fa 00 00 01 fa
  0x0c067fff8090: fa fa 00 00 01 fa fa fa 00 00 04 fa fa fa 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==2278709==ABORTING

PHP Version

PHP 8.4.0-dev

Operating System

ubuntu 22.04

@cmb69
Copy link
Member

cmb69 commented Oct 9, 2024

We might overflow GH issue IDs by tomorrow. :)

@cmb69
Copy link
Member

cmb69 commented Oct 9, 2024

Simpler reproducer:

deflate_init(ZLIB_ENCODING_DEFLATE, ["dictionary" => [""]]);

In

efree(*ptr);

we're trying to efree()[sic] zend_empty_string.

@cmb69
Copy link
Member

cmb69 commented Oct 9, 2024

Incomplete patch (would solve the reported issue, but there are more; for instance *strings should be allocated using safe_emalloc(); might also use zval_try_get_string() instead of zval_get_string(), and more):

 ext/zlib/zlib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/zlib/zlib.c b/ext/zlib/zlib.c
index 3c9f72e82b..e964ef25df 100644
--- a/ext/zlib/zlib.c
+++ b/ext/zlib/zlib.c
@@ -813,7 +813,7 @@ static bool zlib_create_dictionary_string(HashTable *options, char **dict, size_
 						*++ptr = zval_get_string(cur);
 						if (!*ptr || ZSTR_LEN(*ptr) == 0 || EG(exception)) {
 							if (*ptr) {
-								efree(*ptr);
+								zend_string_release(*ptr);
 							}
 							while (--ptr >= strings) {
 								efree(ptr);

@cmb69 cmb69 self-assigned this Oct 9, 2024
cmb69 added a commit to cmb69/php-src that referenced this issue Oct 9, 2024
We must not `efree()` `zend_string`s, since they may have a refcount
greater than one, and may even be interned.

We also must not confuse `zend_string *` with `zend_string **`.

And we should play it safe by using `safe_emalloc()` to avoid
theoretical integer overflows.
cmb69 added a commit to cmb69/php-src that referenced this issue Oct 9, 2024
We must not `efree()` `zend_string`s, since they may have a refcount
greater than one, and may even be interned.

We also must not confuse `zend_string *` with `zend_string **`.

And we should play it safe by using `safe_emalloc()` to avoid
theoretical integer overflows.
@cmb69 cmb69 linked a pull request Oct 9, 2024 that will close this issue
@cmb69 cmb69 changed the title Segmentation fault (heap-buffer-overflow) in Zend/zend_alloc.c:1528:28 in zend_mm_free_heap Memory management is broken for bad dictionaries Oct 9, 2024
@cmb69 cmb69 closed this as completed in d94be24 Oct 13, 2024
cmb69 added a commit that referenced this issue Oct 13, 2024
* PHP-8.2:
  Fix GH-16326: Memory management is broken for bad dictionaries
cmb69 added a commit that referenced this issue Oct 13, 2024
* PHP-8.3:
  Fix GH-16326: Memory management is broken for bad dictionaries
cmb69 added a commit that referenced this issue Oct 13, 2024
* PHP-8.4:
  Fix GH-16326: Memory management is broken for bad dictionaries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants