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

go to bug id or search bugs for

Sec Bug #70755 fpm_log.c memory leak and buffer overflow
Submitted: 2015-10-21 08:54 UTC Modified: 2016-05-29 11:31 UTC
From: imre dot rad at search-lab dot hu Assigned: stas (profile)
Status: Closed Package: FPM related
PHP Version: 5.6.14 OS: All
Private report: No CVE-ID: 2016-5114
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: imre dot rad at search-lab dot hu
New email:
PHP Version: OS:

 

 [2015-10-21 08:54 UTC] imre dot rad at search-lab dot hu 
Description:
------------
After adding %{REQUEST_URI}e to the end of the access.format option of the PHP-FPM ini, I noticed binary output appearing in the access log file. (I find this feature quite useful because it helps understanding the current workload in case of rewritten URLs)

At line 237 of php-fpm.c:
len2 = snprintf(b, FPM_LOG_BUFFER - len, "%s", env ? env : "-");

From snprintf() manual:
The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). If the output was truncated due to this limit then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. 


The full length (len) is increased by len2 at line 449:
len += len2;

After exiting the loop, a \n byte is written outside of the compiled buffer and the log line along with some memory area lying after it is flushed into the access log:

	if (!test && strlen(buffer) > 0) {
 		buffer[len] = '\n';
		write(fpm_log_fd, buffer, len + 1);
	}


I classify the above vulnerability as memory leak and limited buffer overflow.
Due to the pre-requisites, I consider severity of this vulnerability as low.

Are you going to make a CVE identifier assigned to it or should I report to MITRE independently?



Test script:
---------------
The access.format looked like this (it is important to have the long stuff as the last token in the format string):

access.format = %{HTTP_HOST}e %R [%t] %m %r%Q%q %p %{user}C %{system}C %{total}C %{kilo}M %{mili}d %s %f %{REMOTE_ADDR}e %{REQUEST_URI}e

Send a HTTP request with long query string then.


Expected result:
----------------
Too long lines in the access log should be cutted in some way, for eg like this:

foobar.info 127.0.0.1 [17/Jul/2015:19:21:37 +0200] GET /wp-admin/load-scripts.php?c=1&load%5B%5D=hoverIntent,common,admin-bar,suggest,inline-edit-post,heartbeat,svg-painter,wp-auth-check,jquery-ui-core,jquery-ui-widget,jquery&load%5B%5D=-ui-tabs,jquery-ui-mouse,jquery-ui-draggable,jquery-ui-slider,jquery-touch-punch,iris,wp-color-picker,jquery-ui-accordion,jquery&load%5B%5D=-ui-position,jquery-ui-menu,jquery-ui-autocomplete,jquery-ui-sortable,backbone,wp-util,wp-backbone,media-models,wp-plupload,medi&load%5B%5D=aelement,wp-mediaelement,media-views,media-editor,media-audiovideo,wp-playli 17109 81.10 40.55 121.64 2816 24.662 200 /foobar.info/pages/wp-admin/load-scripts.php 5.38.155.205 /wp-admin/load-scripts.php?c=1&load%5B%5D=hoverIntent,common,admin-bar,suggest,inline-edit-post,heartbeat,svg-painter,wp-auth-check,jquery-ui-core,jquery-ui-widget,jquery&load%5B%5D=-ui-tabs,jquery-ui-mouse,jquery-ui-draggable,jquery-ui-slider,jquery-touch-punch,iris,wp-color-picker,jquery-ui-accordion,jquery&load%5B%5D=-ui-posi...



Actual result:
--------------
A sample line from the access log:


foobar.info 127.0.0.1 [17/Jul/2015:19:21:37 +0200] GET /wp-admin/load-scripts.php?c=1&load%5B%5D=hoverIntent,common,admin-bar,suggest,inline-edit-post,heartbeat,svg-painter,wp-auth-check,jquery-ui-core,jquery-ui-widget,jquery&load%5B%5D=-ui-tabs,jquery-ui-mouse,jquery-ui-draggable,jquery-ui-slider,jquery-touch-punch,iris,wp-color-picker,jquery-ui-accordion,jquery&load%5B%5D=-ui-position,jquery-ui-menu,jquery-ui-autocomplete,jquery-ui-sortable,backbone,wp-util,wp-backbone,media-models,wp-plupload,medi&load%5B%5D=aelement,wp-mediaelement,media-views,media-editor,media-audiovideo,wp-playli 17109 81.10 40.55 121.64 2816 24.662 200 /foobar.info/pages/wp-admin/load-scripts.php 5.38.155.205 /wp-admin/load-scripts.php?c=1&load%5B%5D=hoverIntent,common,admin-bar,suggest,inline-edit-post,heartbeat,svg-painter,wp-auth-check,jquery-ui-core,jquery-ui-widget,jquery&load%5B%5D=-ui-tabs,jquery-ui-mouse,jquery-ui-draggable,jquery-ui-slider,jquery-touch-punch,iris,wp-color-picker,jquery-ui-accordion,jquery&load%5B%5D=-ui-posi^@^@^@^@^@^@^@^@^@ è<97> ü^?^@^@^A^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^A^@^@^@^@^@^@^@¡9©U^@^@^@^@ÕB^@^@^@^@^@^@^B^@^@^@^@^@^@^@^F^@^@^@^@^@^@^@¯D<93>^@^@^@^@^@l^C^B^@^@^@^@^@^@^@^@^@^@^@^@^@V`^@^@^@^@^@^@¡9©U^@^@^@^@¯D<93>^@^@^@^@^@Âc^B^@^@^@^@^@/wp-admin/load-scripts.php^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@c=1&load%5B%5D=hoverIntent,com

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-12-08 08:09 UTC] stas@php.net
-Assigned To: +Assigned To: stas
 [2015-12-08 08:09 UTC] stas@php.net 
Please try the following patch: 

diff --git a/sapi/fpm/fpm/fpm_log.c b/sapi/fpm/fpm/fpm_log.c
index b0bf32a..187fe9b 100644
--- a/sapi/fpm/fpm/fpm_log.c
+++ b/sapi/fpm/fpm/fpm_log.c
@@ -448,6 +448,11 @@ int fpm_log_write(char *log_format TSRMLS_DC) /* {{{ */
                                b += len2;
                                len += len2;
                        }
+                       if (len >= FPM_LOG_BUFFER) {
+                               zlog(ZLOG_NOTICE, "the log buffer is full (%d). The access log request has been truncated.", FPM_LOG_BUFFER);
+                               len = FPM_LOG_BUFFER;
+                               break;
+                       }
                        continue;
                }
 [2015-12-10 07:37 UTC] imre dot rad at search-lab dot hu 
I've tested it using this simple Perl script: http://pastebin.com/XE9cSejA

It works as expected; thank you for the patch.
 [2015-12-28 20:32 UTC] stas@php.net 
In security repo as commit be19dbcb84fea0001e53cea2732c00de7ae6c371
 [2016-01-06 03:19 UTC] stas@php.net
-Status: Assigned +Status: Closed
 [2016-01-06 03:19 UTC] stas@php.net 
The fix for this bug has been committed.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.
 [2016-01-06 03:38 UTC] stas@php.net 
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=be19dbcb84fea0001e53cea2732c00de7ae6c371
Log: Fixed bug #70755: fpm_log.c memory leak and buffer overflow
 [2016-01-06 06:34 UTC] ab@php.net 
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=2eaa7556605ffc0d78c89965f5e905cf801207ef
Log: Fixed bug #70755: fpm_log.c memory leak and buffer overflow
 [2016-01-06 06:34 UTC] ab@php.net 
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=be19dbcb84fea0001e53cea2732c00de7ae6c371
Log: Fixed bug #70755: fpm_log.c memory leak and buffer overflow
 [2016-01-06 06:34 UTC] ab@php.net 
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=2eaa7556605ffc0d78c89965f5e905cf801207ef
Log: Fixed bug #70755: fpm_log.c memory leak and buffer overflow
 [2016-01-06 06:34 UTC] ab@php.net 
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=be19dbcb84fea0001e53cea2732c00de7ae6c371
Log: Fixed bug #70755: fpm_log.c memory leak and buffer overflow
 [2016-01-06 06:35 UTC] ab@php.net 
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=2721a0148649e07ed74468f097a28899741eb58f
Log: Fixed bug #70755: fpm_log.c memory leak and buffer overflow
 [2016-02-02 10:54 UTC] korvin1986 at gmail dot com 
Hello, 
is any CVE-ID exists for this vulnerability?
Or have I request it at http://www.cve.mitre.org ?
 [2016-05-27 05:07 UTC] henri at nerv dot fi 
2016-01-30: CVE request was made directly to MITRE.
2016-02-02: CVE request in oss-security mailing list: http://www.openwall.com/lists/oss-security/2016/02/02/4
2016-05-27: Discussing with MITRE again to get this assigned.
 [2016-05-29 10:01 UTC] henri at nerv dot fi 
MITRE responds http://www.openwall.com/lists/oss-security/2016/05/29/1 following:

As explained in the www.search-lab.hu post (in the section between "We
found the answer by reviewing the source code" and "And here we are"),
there was really only one underlying problem: the code misinterpreted
the semantics of the snprintf return value. Use CVE-2016-5114. The
other outcomes were consequences of this. The "memory leak" is the
same as the "out-of-boundaries read": extra bytes from process memory
were being written to a log file that might be readable by untrusted
users. The "buffer overflow" is the same as the "wrote a \n character
outside of the allocated memory."
 [2016-05-29 11:31 UTC] kaplan@php.net
-CVE-ID: +CVE-ID: 2016-5114
 [2016-07-20 11:34 UTC] davey@php.net 
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=2eaa7556605ffc0d78c89965f5e905cf801207ef
Log: Fixed bug #70755: fpm_log.c memory leak and buffer overflow
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Thu Nov 21 11:01:29 2024 UTC