php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Bug #81513 PHP-FPM heap overflow under strange configuration
Submitted: 2021-10-07 15:33 UTC Modified: 2021-11-07 21:01 UTC
From: c dot fol at ambionics dot io Assigned: bukka (profile)
Status: Closed Package: FPM related
PHP Version: 8.1.0RC3 OS:
Private report: No CVE-ID: None
View Developer Edit
 [2021-10-07 15:33 UTC] c dot fol at ambionics dot io 
Description:
------------
Hello PHP team,

There's a heap overflow in PHP-FPM's log management system, which is enabled only if you enable "catch_workers_output" in the configuration, and change zlog_limit.

Basically, with catch_workers_output On, the root process will buffer anything its workers write to stderr until a newline is encountered, in which case it will send the line to the log file, and flush the buffer.

The code responsible for (re-)allocating the buffer is wrong.

Take a look at zlog_stream_buf_copy_cstr(): if there is not enough space remaining in the buffer to store the new string, zlog_stream_buf_alloc_ex() is called.

zlog_stream_buf_alloc_ex() can, in some cases, allocate "needed" (3rd params) bytes.

Now, when you go back to zlog_stream_buf_copy_cstr(), you can see the memcpy() copies str_len bytes, but starting at offset stream->len.

Therefore, there can be cases where you (re-allocate) a buffer of size str_len, but copy up to offset stream->len + str_len, leading to an overflow of size stream->len.

The configuration required for this is very unlikely, and exploit pretty tedious, so I don't think it is a serious security bug, but this is a very simple patch.

I guess the simplest way to patch is to change:

if (stream->buf.size - stream->len <= str_len && !zlog_stream_buf_alloc_ex(stream, str_len)) {

to

if (stream->buf.size - stream->len <= str_len && !zlog_stream_buf_alloc_ex(stream, stream->len + str_len)) {


Charles

Expected result:
----------------
No overflow

Actual result:
--------------
Potential overflow

Patches

Pull Requests

Pull requests:

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2021-10-11 11:59 UTC] c dot fol at ambionics dot io 
Related To: Bug #81026
 [2021-10-11 12:49 UTC] bukka@php.net
-Status: Open +Status: Assigned -Package: *General Issues +Package: FPM related -Assigned To: +Assigned To: bukka
 [2021-10-17 20:03 UTC] bukka@php.net 
Ok I see the issue and the report is correct. This is actually my fault as I introduced this logic and completely missed that possibility.

I agree that this is a security issue as it can potentially result in buffer overflow which is possible to in some sort of way control especially with

decorate_workers_output = no

And then just writing specific string to stderr. Although it's quite hard as you say and this setting is often just set for for Docker where the priv escalation might have lower impact as there are other protections as well.

Anyway I will think about a test case for this as this might be crashable potentially if I tweak the values correctly. At least I will try. The suggested changes look correct though so the actual fix is pretty small.
 [2021-10-18 14:32 UTC] bukka@php.net 
Just to clarify, the PHP 7.3 support ends in something over one month on 6th December so it's most likely just one extra final security release which wouldn't most likely contain any fixes if there are any problems with this.
 [2021-10-18 19:35 UTC] bukka@php.net 
The above comment about PHP 7.3 should be in different bug so pls ignore
 [2021-10-31 21:29 UTC] bukka@php.net 
So I looked a bit more into this one and did a little bit of debugging and not sure if this can even happen. The part that you might have missed is this:

https://github.com/php/php-src/blob/8a79668dbe2044bbcae8720114ffea0edc160436/sapi/fpm/fpm/zlog.c#L474-L482

Just to note zlog_stream_buf_append is the only function that can add longer string to zlog_stream_buf_copy_cstr (others are just short prefixes) and it seems to already handle wrapping before the call so it doesn't seem possible to get to the path where MAX(size * 2, needed) would use needed. It should always go through the size * 2 path which should mean that there will be always enough space for the copied string and it should never overflow though. At least I wasn't able to find any case where it would overflow for me.

But it's a bit late so I might have missed something. Please correct me if there's a path that I missed. If you could also give me some recreateable code (just basically few strings that would overflow if processed by zlog buffering - doesn't need to be a full exploit though...), that would be great.
 [2021-11-01 11:16 UTC] c dot fol at ambionics dot io 
Hello bukka,

You're right (I believe) ! There's maybe an edge case that we didn't think about... but I can't find it.

However, although it is safe, it is still "wrong": (re)allocation should be in function of the full length (cursor position + additional size). Just because we can't find a path doesn't mean it does not exist or won't in the future...

Just my 2 cents.
Charles
 [2021-11-07 21:01 UTC] bukka@php.net
-Type: Security +Type: Bug
 [2021-11-07 21:01 UTC] bukka@php.net 
Yes it's a code bug and should be fixed as a normal bug.
 [2021-11-07 21:02 UTC] bukka@php.net 
The following pull request has been associated:

Patch Name: Fix bug #81513 (Future possibility for heap overflow in FPM zlog)
On GitHub:  https://github.com/php/php-src/pull/7632
Patch:      https://github.com/php/php-src/pull/7632.patch
 [2021-11-14 20:18 UTC] git@php.net 
Automatic comment on behalf of bukka
Revision: https://github.com/php/php-src/commit/b2cf9b7ec796f8251da62168a9e837102e2fcc1f
Log: Fix bug #81513 (Future possibility for heap overflow in FPM zlog)
 [2021-11-14 20:18 UTC] git@php.net
-Status: Assigned +Status: Closed
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Thu Nov 21 09:01:32 2024 UTC