Skip to content

PharFileInfo refcount bug #17808

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

Closed
YuanchengJiang opened this issue Feb 15, 2025 · 5 comments
Closed

PharFileInfo refcount bug #17808

YuanchengJiang opened this issue Feb 15, 2025 · 5 comments

Comments

@YuanchengJiang
Copy link

Description

The following code:

<?php
$fname = str_replace('\\', '/', __DIR__ . '/files/Structures_Graph-1.0.3.tgz');
$tar = new PharData($fname);
foreach (new RecursiveIteratorIterator($tar) as $file) {
}
$fusion = $file;
$dests = array(
"$sub_dir/..///../copy_copy_variation8.tmp",
"$sub_dir///../*",
"$dirname_with_blank/copy_copy_variation8.tmp"
);
foreach($dests as $dest) {
unlink("$fusion");
}

Resulted in this output:

/home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_string.h:338:2: runtime error: member access within misaligned address 0x7461697261765f79 for type 'zend_string' (aka 'struct _zend_string'), which requires 8 byte alignment
0x7461697261765f79: note: pointer points here
<memory cannot be printed>
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_string.h:338:2 in 

To reproduce:

./php-src/sapi/cli/php  ./test.php

Commit:

commit 5acff0e61dd9a62ddff52bea25d552db45fb32e6
Author: Niels Dossche <[email protected]>
Date:   Tue Feb 11 21:57:50 2025 +0100

    Update NEWS and UPGRADING for zlib flock() support
    
    [ci skip]
    
    Closes GH-17752.

Configurations:

CC="clang-12" CXX="clang++-12" CFLAGS="-DZEND_VERIFY_TYPE_INFERENCE" CXXFLAGS="-DZEND_VERIFY_TYPE_INFERENCE" ./configure --enable-debug --enable-address-sanitizer --enable-undefined-sanitizer --enable-re2c-cgoto --enable-fpm --enable-litespeed --enable-phpdbg-debug --enable-zts --enable-bcmath --enable-calendar --enable-dba --enable-dl-test --enable-exif --enable-ftp --enable-gd --enable-gd-jis-conv --enable-mbstring --enable-pcntl --enable-shmop --enable-soap --enable-sockets --enable-sysvmsg --enable-zend-test --with-zlib --with-bz2 --with-curl --with-enchant --with-gettext --with-gmp --with-mhash --with-ldap --with-libedit --with-readline --with-snmp --with-sodium --with-xsl --with-zip

Operating System:

Ubuntu 20.04 Host, Docker 0599jiangyc/flowfusion:latest

This report is automatically generated by FlowFusion

PHP Version

5acff0e

Operating System

No response

@devnexen
Copy link
Member

Hi, can t reproduce this one. Would it be possible to automatically add to your flowfusion reports the following env var :

UBSAN_OPTIONS="print_stacktrace=1" ?

@YuanchengJiang
Copy link
Author

@devnexen sure! thank you for letting me know about this option.

/home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_string.h:338:2: runtime error: member access within misaligned address 0x7274532f73656c69 for type 'zend_string' (aka 'struct _zend_string'), which requires 8 byte alignment
0x7274532f73656c69: note: pointer points here
<memory cannot be printed>
    #0 0x2154c22 in zend_string_efree /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_string.h:338:2
    #1 0x2173b33 in zim_PharFileInfo___destruct /home/phpfuzz/WorkSpace/flowfusion/php-src/ext/phar/phar_object.c:4538:4
    #2 0x3f3494f in zend_call_function /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_execute_API.c:1021:4
    #3 0x3f3a3de in zend_call_known_function /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_execute_API.c:1102:23
    #4 0x4be3384 in zend_call_known_instance_method /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_API.h:860:2
    #5 0x4bde1db in zend_call_known_instance_method_with_0_params /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_API.h:866:2
    #6 0x4bdd4df in zend_objects_destroy_object /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_objects.c:194:3
    #7 0x2ab36e4 in spl_filesystem_object_destroy_object /home/phpfuzz/WorkSpace/flowfusion/php-src/ext/spl/spl_directory.c:109:2
    #8 0x4bd13e0 in zend_objects_store_call_destructors /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_objects_API.c:57:7
    #9 0x3f0f609 in shutdown_destructors /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_execute_API.c:266:3
    #10 0x4d30dbb in zend_call_destructors /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend.c:1335:3
    #11 0x35298d8 in php_request_shutdown /home/phpfuzz/WorkSpace/flowfusion/php-src/main/main.c:1921:3
    #12 0x4d5c3ae in do_cli /home/phpfuzz/WorkSpace/flowfusion/php-src/sapi/cli/php_cli.c:1144:3
    #13 0x4d5147f in main /home/phpfuzz/WorkSpace/flowfusion/php-src/sapi/cli/php_cli.c:1348:18
    #14 0x7fc96dc99d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #15 0x7fc96dc99e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #16 0x605954 in _start (/home/phpfuzz/WorkSpace/flowfusion/php-src/sapi/cli/php+0x605954)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_string.h:338:2 in

@nielsdos
Copy link
Member

nielsdos commented Feb 15, 2025

Simplified:

<?php
$fname = __DIR__.'/ext/phar/tests/tar/files/Structures_Graph-1.0.3.tgz';
$tar = new PharData($fname);
foreach (new RecursiveIteratorIterator($tar) as $file) {
}
unlink("$file");

EDIT: ah if you do this instead, the bug disappears, so the unlink removes some data still in use by file:

$str = "$file";
unset($file);
unlink($str);

@YuanchengJiang
Copy link
Author

BTW, the address looks dangerous to me. Is it controllable?

@nielsdos
Copy link
Member

It's probably controllable by a subsequent allocation. Question remains how an attacker would do this though but it's not outside the realm of possibility.

@nielsdos nielsdos changed the title SEGV Zend/zend_string.h:338 PharFileInfo refcount bug Feb 15, 2025
nielsdos added a commit to nielsdos/php-src that referenced this issue Feb 15, 2025
PharFileInfo just takes a pointer from the manifest without refcounting
anything. If the entry is then removed from the manifest while the
PharFileInfo object still exists, we get a UAF.
We fix this by using the fp_refcount field. This is technically a
behaviour change as the unlinking is now blocked, and potentially file
modifications can be blocked as well. The alternative would be to have a
field that indicates whether deletion is blocked, but similar corruption
bugs may occur as well with file overwrites, so we increment fp_refcount
instead.
This also fixes an issue where a destructor called multiple times
resulted in a UAF as well, by moving the NULL'ing of the entry field out
of the if.
@nielsdos nielsdos linked a pull request Feb 15, 2025 that will close this issue
nielsdos added a commit that referenced this issue Feb 15, 2025
* PHP-8.3:
  Fix GH-17808: PharFileInfo refcount bug
nielsdos added a commit that referenced this issue Feb 15, 2025
* PHP-8.4:
  Fix GH-17808: PharFileInfo refcount bug
nielsdos added a commit that referenced this issue Feb 15, 2025
nielsdos added a commit that referenced this issue Feb 15, 2025
* PHP-8.3:
  [ci skip] Fix GH-17808 dependencies
nielsdos added a commit that referenced this issue Feb 15, 2025
* PHP-8.4:
  [ci skip] Fix GH-17808 dependencies
charmitro pushed a commit to wasix-org/php that referenced this issue Mar 13, 2025
PharFileInfo just takes a pointer from the manifest without refcounting
anything. If the entry is then removed from the manifest while the
PharFileInfo object still exists, we get a UAF.
We fix this by using the fp_refcount field. This is technically a
behaviour change as the unlinking is now blocked, and potentially file
modifications can be blocked as well. The alternative would be to have a
field that indicates whether deletion is blocked, but similar corruption
bugs may occur as well with file overwrites, so we increment fp_refcount
instead.
This also fixes an issue where a destructor called multiple times
resulted in a UAF as well, by moving the NULL'ing of the entry field out
of the if.

Closes phpGH-17811.
charmitro pushed a commit to wasix-org/php that referenced this issue Mar 13, 2025
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.

3 participants