php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #68819 Fileinfo on specific file causes spurious OOM and/or segfault
Submitted: 2015-01-12 22:53 UTC Modified: 2016-02-11 14:08 UTC
Votes:2
Avg. Score:5.0 ± 0.0
Reproduced:2 of 2 (100.0%)
Same Version:1 (50.0%)
Same OS:0 (0.0%)
From: dominic at varspool dot com Assigned: ab
Status: Closed Package: Reproducible crash
PHP Version: 5.6.4 OS: Linux/MacOS/any?
Private report: No CVE-ID: 2015-4604
 [2015-01-12 22:53 UTC] dominic at varspool dot com
Description:
------------
When calling finfo::file() or finfo::buffer() with a crafted string, PHP will crash by either segfaulting or trying to allocate an large amount of memory (4GiB).

This was found in the wild when a user uploaded a file (running finfo on arbitrary files uploaded by users is one of its main use cases.). I've since anonymised the file, and made it more minimal. At this stage, very small changes to the string make it produce different behaviour - removing the remaining 'a', 's', or 'y' characters, for instance, will allow finfo to process it fine.

I suspect a bad magic rule (but I can't tell which from gdb).




Test script:
---------------
The basic test script is:

<?php

$string = <magic value>

$finfo = new finfo();
$type = $finfo->buffer($string);
var_dump($type);

?>

Here are 3v4l links to versions of the magic value that cause unexpected behaviour:
  Spurious OOM error (tried to allocate 4GB): http://3v4l.org/gOeJ3
  Segfault: http://3v4l.org/HghCY

The difference in the strings in the two versions is an single '-' before the first CRLF linebreak.

The behaviour is the same when the string is written to a file and loaded with finfo::file().

Expected result:
----------------
string(60) "ASCII text, with very long lines, with CRLF line terminators"

Actual result:
--------------
With one string:
  Fatal error: Allowed memory size of 67108864 bytes exhausted (tried to allocate 4294967295 bytes) in <script> on line X

With a very slightly different string:
  Segmentation fault

Here's the backtrace:

#0  __memcpy_sse2_unaligned () at ../sysdeps/x86_64/multiarch/memcpy-sse2-unaligned.S:36
#1  0x00000000007ce518 in _estrndup (
    s=0x7f692d976071 "-------a", '-' <repeats 12 times>, "s-----''------a----s--------a", '-' <repeats 13 times>, "a-\r\n", length=4294967295, __zend_filename=0xbfd968 "/home/someone/php-5.6.4/ext/fileinfo/libmagic/softmagic.c",
    __zend_lineno=2108, __zend_orig_filename=0x0, __zend_orig_lineno=0)
    at /home/someone/php-5.6.4/Zend/zend_alloc.c:2655
#2  0x00000000005c3ebf in magiccheck (ms=0x7f692db41978, m=0xbbb460 <php_magic_database+2544480>)
    at /home/someone/php-5.6.4/ext/fileinfo/libmagic/softmagic.c:2108
#3  0x00000000005bf080 in match (ms=0x7f692db41978, magic=0x94e1f8 <php_magic_database+248>, nmagic=10428,
    s=0x7f692d974070 "----a-----'''---------a", '-' <repeats 15 times>, "a--------a-----a-----a---------a-----as-------a----a--a", '-' <repeats 13 times>, "a--as-----s", '-' <repeats 15 times>, "a---------a---a--s-a-----a", '-' <repeats 11 times>, "asy---------a-----a", '-' <repeats 11 times>, "a"..., nbytes=8259, offset=0, mode=64, text=1, flip=0,
    recursion_level=0, printed_something=0x7fff3a875c5c, need_separator=0x7fff3a875c60, returnval=0x7fff3a875bac)
    at /home/someone/php-5.6.4/ext/fileinfo/libmagic/softmagic.c:274
#4  0x00000000005beae0 in file_softmagic (ms=0x7f692db41978,
    buf=0x7f692d974070 "----a-----'''---------a", '-' <repeats 15 times>, "a--------a-----a-----a---------a-----as-------a----a--a", '-' <repeats 13 times>, "a--as-----s", '-' <repeats 15 times>, "a---------a---a--s-a-----a", '-' <repeats 11 times>, "asy---------a-----a", '-' <repeats 11 times>, "a"..., nbytes=8259, level=0, mode=64, text=1)
    at /home/someone/php-5.6.4/ext/fileinfo/libmagic/softmagic.c:93
#5  0x00000000005b6414 in file_ascmagic_with_encoding (ms=0x7f692db41978,
    buf=0x7f692da28dc0 "----a-----'''---------a", '-' <repeats 15 times>, "a--------a-----a-----a---------a-----as-------a----a--a", '-' <repeats 13 times>, "a--as-----s", '-' <repeats 15 times>, "a---------a---a--s-a-----a", '-' <repeats 11 times>, "asy---------a-----a", '-' <repeats 11 times>, "a"..., nbytes=8259, ubuf=0x2d37a60, ulen=8259,
    code=0xbfcdd3 "ASCII", type=0xbfcdbf "text", text=1)
    at /home/someone/php-5.6.4/ext/fileinfo/libmagic/ascmagic.c:149
#6  0x00000000005b6230 in file_ascmagic (ms=0x7f692db41978,
    buf=0x7f692da28dc0 "----a-----'''---------a", '-' <repeats 15 times>, "a--------a-----a-----a---------a-----as-------a----a--a", '-' <repeats 13 times>, "a--as-----s", '-' <repeats 15 times>, "a---------a---a--s-a-----a", '-' <repeats 11 times>, "asy---------a-----a", '-' <repeats 11 times>, "a"..., nbytes=8259, text=1)
    at /home/someone/php-5.6.4/ext/fileinfo/libmagic/ascmagic.c:92
#7  0x00000000005bc45e in file_buffer (ms=0x7f692db41978, stream=0x0, inname=0x0, buf=0x7f692da28dc0, nb=8259)
    at /home/someone/php-5.6.4/ext/fileinfo/libmagic/funcs.c:264
#8  0x00000000005bd62c in magic_buffer (ms=0x7f692db41978, buf=0x7f692da28dc0, nb=8259)
    at /home/someone/php-5.6.4/ext/fileinfo/libmagic/magic.c:435
#9  0x00000000005af439 in _php_finfo_get_type (ht=1, return_value=0x7f692db3f320, return_value_ptr=0x7f692db07210,
    this_ptr=0x7f692db3cc20, return_value_used=1, mode=0, mimetype_emu=0)
    at /home/someone/php-5.6.4/ext/fileinfo/fileinfo.c:476
#10 0x00000000005af9a2 in zif_finfo_buffer (ht=1, return_value=0x7f692db3f320, return_value_ptr=0x7f692db07210,
    this_ptr=0x7f692db3cc20, return_value_used=1) at /home/someone/php-5.6.4/ext/fileinfo/fileinfo.c:588
#11 0x000000000084c4a7 in zend_do_fcall_common_helper_SPEC (execute_data=0x7f692db072e8)
    at /home/someone/php-5.6.4/Zend/zend_vm_execute.h:558
#12 0x000000000084cc7e in ZEND_DO_FCALL_BY_NAME_SPEC_HANDLER (execute_data=0x7f692db072e8)
    at /home/someone/php-5.6.4/Zend/zend_vm_execute.h:693
#13 0x000000000084bb16 in execute_ex (execute_data=0x7f692db072e8)
    at /home/someone/php-5.6.4/Zend/zend_vm_execute.h:363
#14 0x000000000084bb9f in zend_execute (op_array=0x7f692db3dbb8) at /home/someone/php-5.6.4/Zend/zend_vm_execute.h:388
#15 0x00000000008079c8 in zend_execute_scripts (type=8, retval=0x0, file_count=3)
    at /home/someone/php-5.6.4/Zend/zend.c:1344
#16 0x000000000076e291 in php_execute_script (primary_file=0x7fff3a8786c0) at /home/someone/php-5.6.4/main/main.c:2584
#17 0x00000000008b9f9d in do_cli (argc=2, argv=0x2ba9bd0) at /home/someone/php-5.6.4/sapi/cli/php_cli.c:994
#18 0x00000000008bb2cb in main (argc=2, argv=0x2ba9bd0) at /home/someone/php-5.6.4/sapi/cli/php_cli.c:1378


Patches

bug68819 (last revision 2015-03-02 13:23 UTC) by ab@php.net)
bug68819_56.patch (last revision 2015-02-05 15:56 UTC) by ab@php.net)
bug68819_54.patch (last revision 2015-02-05 15:55 UTC) by ab@php.net)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-01-12 22:58 UTC] dominic at varspool dot com
This appears to have been introduced in August/September. The affected versions are:

5.4.32+
5.5.16+
all versions of 5.6
7@20140901


Unaffected is:

5.4.31
5.5.15
and earlier.
 [2015-02-05 06:55 UTC] ab@php.net
-Type: Bug +Type: Security -Assigned To: +Assigned To: ab -Private report: No +Private report: Yes
 [2015-02-05 12:15 UTC] dominic at varspool dot com
Here's a much reduced test case:

<?php

$string = '';

// These two in any order
$string .= "\r\n";
$string .= "''''";

// Total string length > 8192
$string .= str_repeat(chr(rand(32, 127)), 8184);

// Ending in this string
$string .= "say";

$finfo = new finfo();
$type = $finfo->buffer($string);
var_dump($type);

?>
 [2015-02-05 13:53 UTC] dominic at varspool dot com
I've learnt a fair bit more about this.

The bug is triggered by this rule
(https://github.com/php/php-src/blob/master@%7B2015-02-06%7D/ext/fileinfo/tests/magic#L16237):

0	search/4096	"""
>&0	regex	.*"""$	Python script text executable
!:mime text/x-python

Specifically, while considering the continuation rule, during the
second call to file_softmagic() - during
file_ascmagic_with_encoding().

Say the buffer you're performing finfo on has a length of 9000 bytes.
At about line 247 in softmagic.c on current master, the OFFADD flag is
set for this rule, so ms->c.li[cont_level - 1].off is added to the
magic_set's offset, bringing its value from 0 to 9000. I think this is
the root cause, because the rule that is being continued should have
matched at a very early offset (a few bytes into the later test case's
string). So perhaps the c.li array is being filled out incorrectly by
the main rule, or incorrectly flushed?

So, ms->offset ends up being 9000 instead of 0.

Then, a little later, we end up calling mget() (softmagic.c, L1161)
with:
 - "offset" pulled from the magic_set, value 9000
 - "nbytes" is 9000 (as it has been the whole way through)
 - "o" is the original offset of 0

Then when we then call mcopy(), we do so with an nbytes and offset
*both* set to 9000. That seems to be where things finally unravel: at
about line 1082 of softmagic.c, we locate 'buf' 9000 bytes into the
haystack string, and last only 8192 bytes into it.

This seems to violate the comment at those lines:
 /* mget() guarantees buf <= last */

So, (ms->search.s_len = last - buf) overflows, and writes a huge
value.

The crash doesn't happen until magiccheck() tries to estrndup() this
size, a few lines later.
 [2015-02-05 14:37 UTC] ab@php.net
@dominic thanks for the further investigations. Your observations are correct. This misbehavior is introduced after this commit: http://git.php.net/?p=php-src.git;a=commit;h=eeaec70 

As a quick fix you can add these lines after the for loop

if (last < buf) {
    last = b;
}

However I don't believe that this piece of code is quite correct in the long term. In the mentioned patch I also don't see any invalid magic introduced. What I suspect is that the patch might be not complete as it's merged from an upper libmagic version. I'm also going to check what you've pointed out with the python part.

Thanks.
 [2015-02-05 14:54 UTC] ab@php.net
Dominic,

I think you've found it ... see here https://github.com/file/file/commit/eced9dbd4aa438de22ff453c723136beac41a558.patch ....

That should have been included into the original patch, but it's quite subtle to have been needed. With these lines commented out in the magic the test passes for me.

Thanks.

Anatol
 [2015-02-05 15:55 UTC] ab@php.net
The following patch has been added/updated:

Patch Name: bug68819_54.patch
Revision:   1423151753
URL:        https://bugs.php.net/patch-display.php?bug=68819&patch=bug68819_54.patch&revision=1423151753
 [2015-02-05 15:56 UTC] ab@php.net
The following patch has been added/updated:

Patch Name: bug68819_56.patch
Revision:   1423151781
URL:        https://bugs.php.net/patch-display.php?bug=68819&patch=bug68819_56.patch&revision=1423151781
 [2015-02-05 15:57 UTC] ab@php.net
-Status: Assigned +Status: Feedback
 [2015-02-05 15:57 UTC] ab@php.net
@dominic,

please check one of the supplied patches. It's either 5.4/5 or 5.6/master.

Thanks.
 [2015-02-05 22:15 UTC] dominic at varspool dot com
-Status: Feedback +Status: Assigned
 [2015-02-05 22:15 UTC] dominic at varspool dot com
I'd give feedback, but the patches are private for me ("You have no access to bug #68819").

(I'm using the password from the original bug submission to do edits like this.)
 [2015-02-06 06:09 UTC] ab@php.net
-Status: Assigned +Status: Feedback
 [2015-02-06 22:19 UTC] dominic at varspool dot com
-Status: Feedback +Status: Assigned
 [2015-02-06 22:19 UTC] dominic at varspool dot com
On master, I applied the 5.6/master patch, and confirmed the test cases now pass. However, I can still target other rules. Here's another one:

<?php

$string = 'try: except:';
$string .= str_repeat(' ', 8178);
$string .= 'say';

var_dump(strlen($string));

$finfo = new finfo();
$type = $finfo->buffer($string);
var_dump($type);

?>

I crafted this one to target the rule:

0	search/4096	try:
>&0	regex	\^\\s*except.*:	Python script text executable

(This from just grepping for search/4096 followed by a regex continuation).
 [2015-02-09 17:10 UTC] ab@php.net
-Status: Assigned +Status: Feedback
 [2015-02-09 17:10 UTC] ab@php.net
Ok, i've two news ... :) For one - the same overflow behavior is to reproduce in file-5.22, that means - it's not a PHP issue, but the mainstream one. Decide which one is bad/good. 

This bug is present in at least the range of 5.19-5.22. To reproduce, use the data from your latest PHP snippet and set a breakpoint like this (in 5.22):

(gdb) b softmagic.c:1140 if buf > last

I've also checked the current libmagic master, and there the issue isn't present anymore. However the code base differs now far more (not even talking that it's not released yet), so i would not backport it. Instead, i'd probably a hotfix for our codebase. We might still try to backport something later after the current master (probably 5.23) is released, right now I'd rather ensure that the offset doesn't exceed bytecnt or vice versa. That is the key point. On 5.6, please try the following patch:

====== START =======
diff --git a/ext/fileinfo/libmagic/softmagic.c b/ext/fileinfo/libmagic/softmagic.c
index c8c72b2..0938ea6 100644
--- a/ext/fileinfo/libmagic/softmagic.c
+++ b/ext/fileinfo/libmagic/softmagic.c
@@ -1074,6 +1074,9 @@ mcopy(struct magic_set *ms, union VALUETYPE *p, int type, int indir,
                        if (bytecnt > nbytes) {
                                bytecnt = nbytes;
                        }
+                       if (offset > bytecnt) {
+                               offset = bytecnt;
+                       }
                        if (s == NULL) {
                                ms->search.s_len = 0;
                                ms->search.s = NULL;

====== END =======

With this, it should be working even without the data patch. While the data patch can be updated later.

Thanks.
 [2015-02-21 09:22 UTC] ab@php.net
@dominic, any update on this? Otherwise i'd rather go by commiting 3 lines from my last comment.

Thanks.
 [2015-02-22 00:44 UTC] dominic at varspool dot com
-Status: Feedback +Status: Assigned
 [2015-02-22 00:44 UTC] dominic at varspool dot com
@ab I've just tested that three line patch, and it looks good. Works well for me.

I haven't had time to chase the issue upstream. I guess that would mean adding an issue at http://bugs.gw.com/? If they haven't done a release since fixing this, that suggests they might not be aware of it.
 [2015-02-23 13:38 UTC] ab@php.net
@dominic, thanks for checkin. I'm going to prepare a patch for this later this week.

Cheers.
 [2015-03-02 13:23 UTC] ab@php.net
The following patch has been added/updated:

Patch Name: bug68819
Revision:   1425302627
URL:        https://bugs.php.net/patch-display.php?bug=68819&patch=bug68819&revision=1425302627
 [2015-03-23 01:46 UTC] stas@php.net
If the fix is already published upstream, should we still keep this as private or can we merge it now?
 [2015-03-23 06:57 UTC] ab@php.net
This issue is still fixed in libmagic master only. To my observations on the vanilla "file" command, the issue is present between 5.19 and 5.22 at least. But in PHP it's a mix of 5.14 and 5.17 with some backported patches. Also, this is somehow mixed with another issue about the extensive backtracking in PHP/PCRE, whereby libmagic uses regex.

In short - I'd still prefer to keep this one private. ATM it still looks not like only an upstream issue, or at least PHP is much more sensitive than upstream in this case.

Thanks.
 [2015-04-14 07:29 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=f938112c495b0d26572435c0be73ac0bfe642ecd
Log: Fix bug #68819 (Fileinfo on specific file causes spurious OOM and/or segfault)
 [2015-04-14 07:29 UTC] stas@php.net
-Status: Assigned +Status: Closed
 [2015-04-14 08:31 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=f938112c495b0d26572435c0be73ac0bfe642ecd
Log: Fix bug #68819 (Fileinfo on specific file causes spurious OOM and/or segfault)
 [2015-04-14 08:31 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=80e263277600b5e62acfc993a308b8174f70581e
Log: Fix bug #68819 (Fileinfo on specific file causes spurious OOM and/or segfault)
 [2015-04-15 08:44 UTC] jpauli@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=f678693e1a6950cae08a99cd145d5d0dc24f92bb
Log: Fix bug #68819 (Fileinfo on specific file causes spurious OOM and/or segfault)
 [2015-05-08 13:09 UTC] artistan at gmail dot com
I am having this issue on PHP 5.4.16 (CentOS yum install)
is there a way for me to build a magic file to use?
thank you.
 [2016-02-11 14:08 UTC] kaplan@php.net
-CVE-ID: +CVE-ID: 2015-4604
 [2016-02-11 14:08 UTC] kaplan@php.net
About CVE:

"Use CVE-2015-4604 for the violation of the "mget() guarantees buf <=
last" constraint suggested in the [2015-02-05 13:53 UTC] comment.

Use CVE-2015-4605 for the issue in which offset can exceed bytecnt,
suggested in the [2015-02-09 17:10 UTC] comment.

These might be conceptually overlapping discoveries, but we decided to
have the two CVE IDs."
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Aug 29 15:01:52 2017 UTC