php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #71220 Null pointer deref (segfault) in compact via ob_start
Submitted: 2015-12-25 20:03 UTC Modified: 2016-01-12 01:20 UTC
From: hugh at allthethings dot co dot nz Assigned: laruence
Status: Closed Package: Reproducible crash
PHP Version: 7.0.1 OS: Linux
Private report: No CVE-ID:
 [2015-12-25 20:03 UTC] hugh at allthethings dot co dot nz
Description:
------------
Found this using afl-fuzz, see http://lcamtuf.coredump.cx/afl/

Affects 7.0.0, 7.0.1, 7.0.2, but none of the 5.6 series or before.

To reproduce, compile PHP normally, then run ./sapi/cli/php with the test script
<?php ob_start( compact ); ?>
You should get a segfault.

The test case is similar to a bug I filed for 5.6, bug #70290, where ob_start can call Zend functions instead of just PHP userland functions.

What is happening is the call to compact actually goes to the function zif_compact in ext/standard/array.c, where the symbol table created on line 1977 with zend_rebuild_symbol_table() returns null, and is then passed into php_compact_var on line 1989, which produces the stack trace below.

A patch to fix would be:

diff --git a/ext/standard/array.c b/ext/standard/array.c
index 79c9ab6..a2be69b 100644
--- a/ext/standard/array.c
+++ b/ext/standard/array.c
@@ -1976,6 +1976,10 @@ PHP_FUNCTION(compact)
 
        symbol_table = zend_rebuild_symbol_table();
 
+       if (symbol_table == NULL) {
+               return;
+       }
+
        /* compact() is probably most used with a single array of var_names
           or multiple string names, rather than a combination of both.
           So quickly guess a minimum result size based on that */




Test script:
---------------
<?php ob_start( compact ); ?>


Expected result:
----------------
No crash

Actual result:
--------------
When compiled with ASAN, get this backtrace:

 $ ./php-7.0.2-asan ~/php-crash-7.0-compact 
ASAN:SIGSEGV
=================================================================
==21914== ERROR: AddressSanitizer: SEGV on unknown address 0x000000000010 (pc 0x00000138e715 sp 0x7ffd5189a610 bp 0x8000000000001505 T0)
AddressSanitizer can not provide additional info.
    #0 0x138e714 in zend_hash_find_bucket /root/php-src/Zend/zend_hash.c:492
    #1 0x138e714 in zend_hash_find /root/php-src/Zend/zend_hash.c:1947
    #2 0xb52c0d in zend_hash_find_ind /root/php-src/Zend/zend_hash.h:278
    #3 0xb52c0d in php_compact_var /root/php-src/ext/standard/array.c:1942
    #4 0xb52c0d in zif_compact /root/php-src/ext/standard/array.c:1989
    #5 0x11e8793 in zend_call_function /root/php-src/Zend/zend_execute_API.c:881
    #6 0x12f28ba in zend_fcall_info_call /root/php-src/Zend/zend_API.c:3574
    #7 0xf5f746 in php_output_handler_op /root/php-src/main/output.c:960
    #8 0xf5f746 in php_output_stack_pop /root/php-src/main/output.c:1221
    #9 0xf5f746 in php_output_end_all /root/php-src/main/output.c:341
    #10 0xeb5014 in php_request_shutdown /root/php-src/main/main.c:1777
    #11 0x19bd1a2 in do_cli /root/php-src/sapi/cli/php_cli.c:1142
    #12 0x472e0a in main /root/php-src/sapi/cli/php_cli.c:1345
    #13 0x7f8960f2cec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
    #14 0x473ed1 in _start (/root/php-src/php-7.0.2-asan+0x473ed1)
SUMMARY: AddressSanitizer: SEGV /root/php-src/Zend/zend_hash.c:491 zend_hash_find_bucket
==21914== ABORTING
Aborted


When compiled without ASAN, it Segmentation Fault's, and running under gdb gives this:

 $ gdb -ex r -ex 'x/i $rip' -ex bt -ex c -ex quit --args ./php-7.0.2-noasan ~/php-crash-7.0-compact 
<snip>
Starting program: /root/php-src/php-7.0.2-noasan /root/php-crash-7.0-compact

Program received signal SIGSEGV, Segmentation fault.
0x0000000000e6e5f8 in zend_hash_find_bucket (ht=0x0, ht=0x0, key=0x7ffff7003580) at /root/php-src/Zend/zend_hash.c:492
492             nIndex = h | ht->nTableMask;
=> 0xe6e5f8 <zend_hash_find+216>:       mov    0xc(%r12),%eax
#0  0x0000000000e6e5f8 in zend_hash_find_bucket (ht=0x0, ht=0x0, key=0x7ffff7003580) at /root/php-src/Zend/zend_hash.c:492
#1  zend_hash_find (ht=ht@entry=0x0, key=0x7ffff7003580) at /root/php-src/Zend/zend_hash.c:1947
#2  0x00000000008d2c6b in zend_hash_find_ind (key=<optimized out>, ht=0x0) at /root/php-src/Zend/zend_hash.h:278
#3  php_compact_var (entry=0x7ffff7014090, return_value=<optimized out>, eg_active_symbol_table=<optimized out>) at /root/php-src/ext/standard/array.c:1942
#4  zif_compact (execute_data=0x7ffff7014030, return_value=0x7fffffffd230) at /root/php-src/ext/standard/array.c:1989
#5  0x0000000000d568b3 in zend_call_function (fci=fci@entry=0x7ffff7072000, fci_cache=fci_cache@entry=0x7ffff7072048) at /root/php-src/Zend/zend_execute_API.c:879
#6  0x0000000000e071bb in zend_fcall_info_call (fci=0x7ffff7072000, fcc=0x7ffff7072048, retval_ptr=retval_ptr@entry=0x7fffffffd230, args=args@entry=0x0)
    at /root/php-src/Zend/zend_API.c:3574
#7  0x0000000000bbda34 in php_output_handler_op (context=0x7fffffffd260, handler=0x7ffff707f050) at /root/php-src/main/output.c:960
#8  php_output_stack_pop (flags=1) at /root/php-src/main/output.c:1221
#9  php_output_end_all () at /root/php-src/main/output.c:341
#10 0x0000000000b47335 in php_request_shutdown (dummy=dummy@entry=0x0) at /root/php-src/main/main.c:1777
#11 0x00000000011bdb9b in do_cli (argc=2, argv=0x18e38b0) at /root/php-src/sapi/cli/php_cli.c:1142
#12 0x00000000004473b9 in main (argc=2, argv=0x18e38b0) at /root/php-src/sapi/cli/php_cli.c:1345
Continuing.

Program terminated with signal SIGSEGV, Segmentation fault.
The program no longer exists.



Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-12-26 05:12 UTC] laruence@php.net
I don't understand why this is a security problem?
 [2015-12-26 05:21 UTC] laruence@php.net
-Type: Security +Type: Bug
 [2015-12-26 05:21 UTC] laruence@php.net
I think this is not a security issue, public it.
 [2015-12-26 05:47 UTC] hugh at allthethings dot co dot nz
Hi,

This is a null pointer deference, which is described on the common weakness enumeration (CWE) list as CWE-476 [1]. It can cause a denial of service, by causing the PHP process to crash unexpectedly (segmentation fault on linux systems).

It is similar to bug #70290 which you fixed promptly, and to earlier bugs I filed such as bug #70183 where ab said that similar bugs (null pointer derefence causing crashes) would count as security after PHP 7 was released, which it has.

If you would like me to label null pointer derefences as non security issues in future, let me know.

Cheers,

Hugh



[1] - https://cwe.mitre.org/data/definitions/476.html
 [2015-12-26 08:47 UTC] laruence@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: laruence
 [2015-12-26 08:47 UTC] laruence@php.net
simple null pointer deref,and it require specific codes. I don't this this is a security issue.

and your patch has been committed, thus closed.

thanks
 [2015-12-26 09:01 UTC] hugh at allthethings dot co dot nz
Just to make sure you understand. This requires a different patch to the one you did in bug #71221.

I'm a bit confused why the stance from php devs have changed since the comment from an in bug #70183?

In my opinion, the big issue here is that you are allowed to call zend defined functions via ob_start instead of just userland defined functions. So far I've filed three independent reports about this and got two patches in and awaiting a third here. I'm positive if I start fuzzing this again I'll find more. If you would like I'm happy collaborating with php to get a patch in that will fix that root issue if I can get guarantee that a patch of that nature would be accepted by upstream.

Cheers,

Hugh
 [2015-12-27 23:18 UTC] stas@php.net
Hugh, what you mean by "zend defined functions"? compact() is a regular PHP function: http://php.net/manual/en/function.compact.php
 [2015-12-27 23:26 UTC] hugh at allthethings dot co dot nz
Hi stas,

I mean functions that are defined in the Zend c language space rather than php userland which I assume is what ob_start is intended for. Here the compact is. Actually calling zif_compact and in the other vug reports they are also zif_ functions. Sorry about confusion about what I meant with Zend defined. Hopefully this clears it uo
 [2016-01-11 00:01 UTC] hugh at allthethings dot co dot nz
Hi,

Just looking at the patch for this [1], I notice that the test case has the extract function, not the compact function. Just tested, the extract function doesn't have this issue, so as it is the test isn't useful.

stas, did my comment above help you understand what I'm seeing as the root cause of this bug?

Cheers,

Hugh

[1] http://git.php.net/?p=php-src.git;a=commitdiff;h=c56efb848b01fa3ecdb7f7253b541b020d154290;hp=6700be67f58611d08bbacc44f327ce98ed0473c9
 [2016-01-11 20:32 UTC] nikic@php.net
@hugh: The test file has been fixed in a commit shortly after that.

As to the root cause of this issue: There is nothing inherently wrong with using an internal function as the callback to ob_start(). Just think about something like ob_start('bin2hex') to create a hex output stream. (It will not actually work due to argument count mismatch, but you get the idea.)

The problem that I see here is that we allow functions those purpose is to expect userland stack frames to be called dynamically. This not only causes the segfaults in this and related bug reports, but the behavior of these functions as callbacks is generally ill-defined.

For example, consider this piece of code:

namespace Foo {
    function test($a, $b, $c) {
        var_dump(call_user_func('func_get_args'));
        var_dump(call_user_func('get_defined_vars'));
    }
    test(1, 2, 3);
}
namespace {
    function test($a, $b, $c) {
        var_dump(call_user_func('func_get_args'));
        var_dump(call_user_func('get_defined_vars'));
    }
    test(1, 2, 3);
}

The output under PHP 7 is:

array(1) {
  [0]=> string(13) "func_get_args"
}
array(3) {
  ["a"]=> int(1)
  ["b"]=> int(2)
  ["c"]=> int(3)
}
array(3) {
  [0]=> int(1)
  [1]=> int(2)
  [2]=> int(3)
}
array(3) {
  ["a"]=> int(1)
  ["b"]=> int(2)
  ["c"]=> int(3)
}

So, in the former case, func_get_args() will inspect the parent call frame (whether it's an internal function or not), so it returns the arguments passed to call_user_func (which is just "func_get_args"). On the other hand, get_defined_vars() will inspect the next higher userland stack frame, so it will instead act on function test().

In the latter case, func_get_args() now works on the user function again -- because here the call_user_func() has been optimized away, so the next higher frame is the test() one.

On HHVM the first (namespaced) get_defined_vars() call instead produces this output:

array(2) {
  ["callback"]=> string(16) "get_defined_vars"
  ["parameters"]=> array(0) { }
}

So HHVM chooses to always use the next-higher stack frame here, even if it's an internal one. In this case only the arguments of the function are used as variables.

Even odder, if you try do run

var_dump(array_map('extract', [['res' => 123]]));

under HHVM, you'll get an "Cannot use a scalar value as an array" warning, as this particular variant of the array_map() function has been implemented using HHAS. In PHP 7, instead a $res variable is created in the parent scope.

These stack-inspecting/manipulating functions really are more language items than functions, using them as callbacks makes no sense, is ill-defined and causes confusing behavior. We should ban it.
 [2016-01-11 20:53 UTC] hugh at allthethings dot co dot nz
Hi Nikic,

Thanks for that detailed reply!

You mention in your first paragraph that calling bin2hex via ob_start won't work due to mismatched argument counts, though in bug #70290 I found that the Zend engine gets arguments through its own way even if they don't exist. In that case it was calling spl_autoload, which expected 1 or 2 arguments, but ob_start passed it both the buffer and the phase, where the second argument ob_start is meant to pass is an int, but spl_autoload treated it as a string.

I think you are hitting the nail full on by noticing the mismatch in the lower level stack reading/changing functions when used as callbacks. And I agree with the need to ban them. Prehaps a blacklist of functions that make no sense, or a whitelist of core defined functions that do make sense (and of course any user defined functions).

I would be happy to work on a patch for that if it would be likely to be accepted, with direction of what core devs would like. If that is the case, should I create a new bug for that?

Cheers,

Hugh
 [2016-01-11 22:00 UTC] yohgaki@php.net
bin2hex() wouldn't work due to signature because it's not designed for ob_start().
However, the argument itself is valid.

ob_start() must accept internal functions as callback. Examples are ob_gzhandler(), etc. http://php.net/ob_gzhandler
 [2016-01-11 22:33 UTC] yohgaki@php.net
It's better not to crash by broken code.
However, one may crash PHP very easily by broken code

<?php
function foo() {
   foo();
}
foo();
?>

crashes. We should protect PHP from internal memory manipulation/leak, but I'm not sure if crash protections for broken code worth the effort/additional overhead.
 [2016-01-12 01:20 UTC] yohgaki@php.net
BTW, simple whitelist does work neither. 3rd party module may have native output handler for better performance. You'll need registration system for output handler whitelist.

There are modules that accept callbacks. For instance, session module is one of them and it has more complex issue with broken code.. If we are going to be strict for broken callbacks, we should check/improve all codes that accept callback.
 [2016-07-20 11:34 UTC] davey@php.net
Automatic comment on behalf of laruence@gmail.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=c56efb848b01fa3ecdb7f7253b541b020d154290
Log: Fixed bug #71220 (Null pointer deref (segfault) in compact via ob_start)
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Fri Jul 21 06:01:37 2017 UTC