php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #61964 finfo_open with directory causes invalid free
Submitted: 2012-05-06 13:39 UTC Modified: 2012-07-15 01:54 UTC
From: nikic@php.net Assigned: stas
Status: Closed Package: Filesystem function related
PHP Version: master-Git-2012-05-06 (Git) OS:
Private report: No CVE-ID:
 [2012-05-06 13:39 UTC] nikic@php.net
Description:
------------
The simple script

<?php
finfo_open(FILEINFO_NONE, '.');

causes an invalid free to be reported by glibc, with the following gdb bt:

#0  0x00130416 in __kernel_vsyscall ()
#1  0x009edc8f in __GI_raise (sig=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0x009f12b5 in __GI_abort () at abort.c:92
#3  0x00a2515c in __libc_message (do_abort=2, 
    fmt=0xafe4c0 "*** glibc detected *** %s: %s: 0x%s ***\n")
    at ../sysdeps/unix/sysv/linux/libc_fatal.c:189
#4  0x00a2ff22 in malloc_printerr (action=<optimized out>, 
    str=<optimized out>, ptr=0xbfff996c) at malloc.c:6283
#5  0x00a30168 in munmap_chunk (p=<optimized out>) at malloc.c:3540
#6  0x081ffde1 in apprentice_load (ms=0xb717d8f8, magicp=0xbfffa9bc, 
    nmagicp=0xbfffa9c0, fn=0xb717d150 "/home/nikic/dev/Phuzzy/results", 
    action=0) at /home/nikic/dev/php-src/ext/fileinfo/libmagic/apprentice.c:814
#7  0x081fecc9 in apprentice_1 (ms=0xb717d8f8, 
    fn=0xb717d150 "/home/nikic/dev/Phuzzy/results", action=0, mlist=0xb717d1a0)
    at /home/nikic/dev/php-src/ext/fileinfo/libmagic/apprentice.c:275
#8  0x081fef8c in file_apprentice (ms=0xb717d8f8, 
    fn=0xb717d150 "/home/nikic/dev/Phuzzy/results", action=0)
    at /home/nikic/dev/php-src/ext/fileinfo/libmagic/apprentice.c:369
#9  0x0820975e in magic_load (ms=0xb717d8f8, 
    magicfile=0xbfffaaec "/home/nikic/dev/Phuzzy/results")
    at /home/nikic/dev/php-src/ext/fileinfo/libmagic/magic.c:308
#10 0x081fdc23 in zif_finfo_open (ht=2, return_value=0xb717c2cc, 
    return_value_ptr=0x0, this_ptr=0x0, return_value_used=0, tsrm_ls=0x8c0f070)
    at /home/nikic/dev/php-src/ext/fileinfo/fileinfo.c:345
#11 0x085cf628 in zend_do_fcall_common_helper_SPEC (execute_data=0xb716007c, 
    tsrm_ls=0x8c0f070) at /home/nikic/dev/php-src/Zend/zend_vm_execute.h:642
#12 0x085d6ddb in ZEND_DO_FCALL_SPEC_CONST_HANDLER (execute_data=0xb716007c, 
    tsrm_ls=0x8c0f070) at /home/nikic/dev/php-src/Zend/zend_vm_execute.h:2219
#13 0x085cd8d5 in execute (op_array=0xb717cc14, tsrm_ls=0x8c0f070)
    at /home/nikic/dev/php-src/Zend/zend_vm_execute.h:410
#14 0x0859202e in zend_execute_scripts (type=8, tsrm_ls=0x8c0f070, retval=0x0, 
    file_count=3) at /home/nikic/dev/php-src/Zend/zend.c:1272
#15 0x084f4e91 in php_execute_script (primary_file=0xbfffe110, 
    tsrm_ls=0x8c0f070) at /home/nikic/dev/php-src/main/main.c:2473
#16 0x086dcbc9 in do_cli (argc=2, argv=0xbffff3b4, tsrm_ls=0x8c0f070)
    at /home/nikic/dev/php-src/sapi/cli/php_cli.c:988
#17 0x086de0ed in main (argc=2, argv=0xbffff3b4)
    at /home/nikic/dev/php-src/sapi/cli/php_cli.c:1361

The invalid free occurs in https://github.com/php/php-src/blob/master/ext/fileinfo/libmagic/apprentice.c#L814.

The code for loading from a directory seems completely broken: The filenames are snprintf'd into the mfn variable, which is a char[MAXPATHLEN].

For every file that variable is then written into the array, without further copying: filearr[files++] = mfn;

So basically at the end filearr just contains the last scanned path multiple times.

In the second loop the individual filearr elements are then freed, which is wrong in two senses: a) it's always the same array element, so it would be a multi-free b) mfn is an array, it was never allocated, so it shouldn't be freed.

The fix should be to copy mfn into a separate pointer when doing filearr[files++] = mfn;

:)


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2012-05-06 14:06 UTC] laruence@php.net
diff --git a/ext/fileinfo/fileinfo.c b/ext/fileinfo/fileinfo.c
index 2c0e39a..9241e5b 100644
--- a/ext/fileinfo/fileinfo.c
+++ b/ext/fileinfo/fileinfo.c
@@ -315,16 +315,24 @@ PHP_FUNCTION(finfo_open)
 	if (file_len == 0) {
 		file = NULL;
 	} else if (file && *file) { /* user specified file, perform open_basedir 
checks */
+		struct stat sb;
+
 		if (strlen(file) != file_len) {
 			FILEINFO_DESTROY_OBJECT(object);
 			RETURN_FALSE;
 		}
+
+		if (VCWD_STAT(file, &sb) || !S_ISREG(sb.st_mode)) {
+			FILEINFO_DESTROY_OBJECT(object);
+			RETURN_FALSE;
+		}
+
 		if (!VCWD_REALPATH(file, resolved_path)) {
 			FILEINFO_DESTROY_OBJECT(object);
 			RETURN_FALSE;
 		}
-		file = resolved_path;
 
+		file = resolved_path;
 #if PHP_API_VERSION < 20100412
 		if ((PG(safe_mode) && (!php_checkuid(file, NULL, 
CHECKUID_CHECK_FILE_AND_DIR))) || php_check_open_basedir(file TSRMLS_CC)) {
 #else
 [2012-05-09 23:37 UTC] felipe@php.net
In fact the libmagic code seems not prepared to work with directory, even alloc'ing the data properly and freeing, it causes memleaks in other parts.
 [2012-05-13 10:56 UTC] laruence@php.net
then I think we can simply prevent directory parameter
 [2012-05-13 11:03 UTC] nikic@php.net
Reeze already has a patch which fixes this issue and several related memory leaks. Though I can't find it anywhere now :/
 [2012-05-24 18:00 UTC] reeze dot xia at gmail dot com
Hi, 
I've sent the file lib author, he replied it was intend to support
dir open. and I sent a Pull Request: https://github.com/php/php-src/pull/91

@felipe will you take look?

Thanks.
 [2012-05-24 18:25 UTC] felipe@php.net
Hi, I got a crash when running the following code in the php-src root dir using your patch (pull-request):

<?php
finfo_open(FILEINFO_NONE, ".");

(gdb) r ../bug.php 
Starting program: /home/felipe/dev/php5_3/sapi/cli/php ../bug.php
[Thread debugging using libthread_db enabled]

Program received signal SIGSEGV, Segmentation fault.
memset () at ../sysdeps/i386/i686/memset.S:85
85	../sysdeps/i386/i686/memset.S: No such file or directory.
	in ../sysdeps/i386/i686/memset.S
Current language:  auto
The current source language is "auto; currently asm".
(gdb) bt
#0  memset () at ../sysdeps/i386/i686/memset.S:85
#1  0x081b29b9 in parse (ms=0x89baeac, mentryp=0xbfffae6c, nmentryp=0xbfffae68, line=0xbfff7dcb "> Makefile.fragments", lineno=143, action=0)
    at /home/felipe/dev/php5_3/ext/fileinfo/libmagic/apprentice.c:1178
#2  0x081b1a55 in load_1 (ms=0x89baeac, action=0, fn=0x89d4e48 "/home/felipe/dev/php5_3/acinclude.m4", errs=0xbfffae70, marray=0xbfffae6c, marraycount=0xbfffae68)
    at /home/felipe/dev/php5_3/ext/fileinfo/libmagic/apprentice.c:733
#3  0x081b1df8 in apprentice_load (ms=0x89baeac, magicp=0xbfffaef0, nmagicp=0xbfffaeec, fn=0x89baf58 "/home/felipe/dev/php5_3", action=0)
    at /home/felipe/dev/php5_3/ext/fileinfo/libmagic/apprentice.c:812
#4  0x081b0ebd in apprentice_1 (ms=0x89baeac, fn=0x89baf58 "/home/felipe/dev/php5_3", action=0, mlist=0x89bb33c) at /home/felipe/dev/php5_3/ext/fileinfo/libmagic/apprentice.c:275
#5  0x081b1180 in file_apprentice (ms=0x89baeac, fn=0x89baf58 "/home/felipe/dev/php5_3", action=0) at /home/felipe/dev/php5_3/ext/fileinfo/libmagic/apprentice.c:369
#6  0x081bb662 in magic_load (ms=0x89baeac, magicfile=0xbfffafb4 "/home/felipe/dev/php5_3") at /home/felipe/dev/php5_3/ext/fileinfo/libmagic/magic.c:308
#7  0x081b003d in zif_finfo_open (ht=2, return_value=0x89ba6b4, return_value_ptr=0x0, this_ptr=0x0, return_value_used=0, tsrm_ls=0x8874050)
    at /home/felipe/dev/php5_3/ext/fileinfo/fileinfo.c:350
#8  0x084673bd in zend_do_fcall_common_helper_SPEC (execute_data=0x89e924c, tsrm_ls=0x8874050) at /home/felipe/dev/php5_3/Zend/zend_vm_execute.h:320
#9  0x0846bac4 in ZEND_DO_FCALL_SPEC_CONST_HANDLER (execute_data=0x89e924c, tsrm_ls=0x8874050) at /home/felipe/dev/php5_3/Zend/zend_vm_execute.h:1640
---Type <return> to continue, or q <return> to quit---
#10 0x08466656 in execute (op_array=0x89badec, tsrm_ls=0x8874050) at /home/felipe/dev/php5_3/Zend/zend_vm_execute.h:107
#11 0x08433ee9 in zend_execute_scripts (type=8, tsrm_ls=0x8874050, retval=0x0, file_count=3) at /home/felipe/dev/php5_3/Zend/zend.c:1236
#12 0x083ae512 in php_execute_script (primary_file=0xbffff434, tsrm_ls=0x8874050) at /home/felipe/dev/php5_3/main/main.c:2308
#13 0x08510211 in main (argc=2, argv=0xbffff5b4) at /home/felipe/dev/php5_3/sapi/cli/php_cli.c:1189
(gdb) f 1
#1  0x081b29b9 in parse (ms=0x89baeac, mentryp=0xbfffae6c, nmentryp=0xbfffae68, line=0xbfff7dcb "> Makefile.fragments", lineno=143, action=0)
    at /home/felipe/dev/php5_3/ext/fileinfo/libmagic/apprentice.c:1178
1178			(void)memset(m, 0, sizeof(*m));
Current language:  auto
The current source language is "auto; currently c".
(gdb) p m
$1 = (struct magic *) 0x1d0
 [2012-05-25 02:03 UTC] laruence@php.net
libmagic is obviously support dir,  but PHP is not, I think you asked the wrong 
guy and wrong question.

what I mean is,  libmagic need a big operation, not just one and one little 
suture, before this, we can just be consistent with the doc said : 'no directory 
supproted" 

thanks
 [2012-05-25 11:07 UTC] reeze dot xia at gmail dot com
Hi, 
  @Felipe I've updated the patch, I made a simple reproducable magic file:
string
> A
> B

and I add it to the test file.(file command itself will leak when use this magic 
file).

@Laruence
the reason why I ask Christos Zoulas is you mentioned load multiple magic file 
may 
lead problem, then I asked him.

Anyway, the crash is a problem of finfo and the fprintf thing, maybe more need 
to be done.
 [2012-07-15 01:53 UTC] stas@php.net
Automatic comment on behalf of reeze.xia@gmail.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=1d2f61904987133d542c68cd349cf313d0bef1c8
Log: Fixed bug #61964 (finfo_open with directory cause invalid free)
 [2012-07-15 01:54 UTC] stas@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: stas
 [2012-07-15 01:54 UTC] stas@php.net
This bug has been fixed in SVN.

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.


 [2012-10-18 15:31 UTC] laruence@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=0d7965f0a32dc967a1665d274cdc0e39d44c87bb
Log: Merge the fix for #61964 to 5.3, which will fix #63304
 [2012-10-18 15:32 UTC] laruence@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=0d7965f0a32dc967a1665d274cdc0e39d44c87bb
Log: Merge the fix for #61964 to 5.3, which will fix #63304
 [2012-10-18 15:34 UTC] laruence@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=0d7965f0a32dc967a1665d274cdc0e39d44c87bb
Log: Merge the fix for #61964 to 5.3, which will fix #63304
 
PHP Copyright © 2001-2014 The PHP Group
All rights reserved.
Last updated: Thu Apr 17 03:01:55 2014 UTC