php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #73809 Phar Zip parse crash - mmap fail
Submitted: 2016-12-23 21:47 UTC Modified: 2020-12-01 13:25 UTC
From: eyal dot itkin at gmail dot com Assigned: cmb (profile)
Status: Closed Package: PHAR related
PHP Version: 7.1.0 OS:
Private report: No CVE-ID: None
 [2016-12-23 21:47 UTC] eyal dot itkin at gmail dot com
Description:
------------
zip.c, phar_parse_zipfile() lacks sanity check on the "uncompressed_filesize" of the zip's entry. This can lead to an extensive memory allocation, that will cause a fatal exception, resulting from an mmap fail.

Trace (from 1 to 3):
1. entry.uncompressed_filesize = PHAR_GET_32(zipentry.uncompsize);
2. if (entry.filename_len == sizeof(".phar/signature.bin")-1 ...
3. sig = (char *) emalloc(entry.uncompressed_filesize);

Proposed fix:
should sanitize the length field, to aavoid extensive allocations. 0xFFFFFFFF will cause a fatal exception (as tested on version 7.1.0).




Test script:
---------------
<?php
    $p = new Phar("example_hostile.phar", 0);
    echo "Loaded the phar archive\n";
?>

Expected result:
----------------
printing: "Loaded the phar archive\n"

Actual result:
--------------
mmap() failed: [22] invalid argument
...

PHP Fatal error.

Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-12-24 12:50 UTC] eyal dot itkin at gmail dot com
-Summary: Zip parse crash - mmap fail +Summary: Phar Zip parse crash - mmap fail -Package: Zip Related +Package: PHAR related
 [2016-12-24 12:51 UTC] eyal dot itkin at gmail dot com
The bug is relevant only to the zip option in the phar module.
 [2016-12-27 07:05 UTC] stas@php.net
Please provide example_hostile.phar
 [2016-12-27 07:05 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2016-12-27 08:47 UTC] eyal dot itkin at gmail dot com
-Status: Feedback +Status: Open
 [2016-12-27 08:47 UTC] eyal dot itkin at gmail dot com
Added the example_hostile.phar at this link: http://www.cs.tau.ac.il/~eyalitki/Upload/73809/
Added also the python script that generated it.
 [2016-12-31 00:10 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2016-12-31 00:10 UTC] stas@php.net
I'm not sure I understand this one - what you mean by "extensive allocations"? Which number would be considered "extensive"? Of course it is possible that the file is too big for PHP to process - this is a legitimate error condition, not a security issue. Could you explain the security issue you see here?
 [2016-12-31 09:07 UTC] eyal dot itkin at gmail dot com
-Status: Feedback +Status: Open
 [2016-12-31 09:07 UTC] eyal dot itkin at gmail dot com
There are several options to limit the ZIP's size in the PHAR module. In all of the cases we want to trade a FATAL exception (i.e. a crash) with a normal exception error that indicates the ZIP is too large.

Some background:
1) TAR archive in the PHAR module can't be bigger than 512 bytes.
2) PHAR archive is limited to 100MB.
3) The ZIP archive in the PHAR module has an overall limit of 64KB.

The only thing that is not limited is the size of the signature.bin inside the ZIP archive.

In addition, this signature is supposed to hold a supported signature: 
1) MD5     - 16 bytes
2) SHA1    - 20 bytes
3) SHA256  - 32 bytes
4) SHA512  - 64 bytes
5) OPENSSL - not exactly clear to me how many bytes the SSL library is expecting

I think it is safe to limit the uncompressed signature size to 64KB:
1) Cryptographic signatures can not be compressed efficiently. Meaning that the size of a compressed valid signature will almost be as large as the original uncompressed signature
2) The overall size for the ZIP archive is 64KB
3) As for today OPENSSL does not support any signature of size > 10KB.
 [2016-12-31 09:16 UTC] stas@php.net
-Type: Security +Type: Feature/Change Request
 [2016-12-31 09:16 UTC] stas@php.net
OK, if this is about changing fatal error to more friendly error handling, it's not a security issue.
 [2016-12-31 09:21 UTC] eyal dot itkin at gmail dot com
I don't think I agree.
The fix is to return an error (via regular expression) instead of crashing the PHP process with trace of:
mmap() failed: [22] invalid argument
...

PHP Fatal error.

The module lacks size checks that causes it to crash, this is not a feature request.
 [2017-01-03 07:26 UTC] eyal dot itkin at gmail dot com
-Type: Feature/Change Request +Type: Security -Private report: No +Private report: Yes
 [2017-01-03 07:26 UTC] eyal dot itkin at gmail dot com
Had a typo in my prev comment: "... Via regular exception, i.e. error".

Since the module is currently crashing with mmap2 failed() that kills the process, I really yhink it is a security issue, even though the sanity check does enforce new (but sane) limitation on the user.
 [2017-01-09 16:59 UTC] leigh@php.net
@stas I believe the point here is that an unsanitised user-supplied value is being passed to emalloc, which can be used to crash the process via huge memory allocation.

// zipentry.uncompsize is user-supplied input (a field in the phar structure)
entry.uncompressed_filesize = PHAR_GET_32(zipentry.uncompsize);

// No validation is performed, and value blindly passed to emalloc
sig = (char *) emalloc(entry.uncompressed_filesize);
 [2017-01-16 05:59 UTC] eyal dot itkin at gmail dot com
Bump. Anything new about this ticket?
 [2017-01-16 08:05 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2017-01-16 08:05 UTC] stas@php.net
> 1) TAR archive in the PHAR module can't be bigger than 512 bytes.
> 2) PHAR archive is limited to 100MB.
> 3) The ZIP archive in the PHAR module has an overall limit of 64KB.

I'm not sure I understand. Where these limitations come from? I know PHAR manifest can't be larger than 100MB but I'm not sure why the whole phar can't be? 

I also don't see why passing any value to emalloc is a problem - we have memory limits for a reason. I'm not sure why on your system it produces mmap error - I don't have any data on the OS/build here in the ticket. In fact, when I run the current code on the same file this is what I get:

Fatal error: Uncaught UnexpectedValueException: internal corruption of phar "/Users/smalyshev/Downloads/example_hostile.phar" (truncated manifest entry)
 [2017-01-16 08:44 UTC] eyal dot itkin at gmail dot com
-Status: Feedback +Status: Open
 [2017-01-16 08:44 UTC] eyal dot itkin at gmail dot com
I don't know who set the current phar limitations. The fact is that there are hardcoded limitations today, and the ZIP module lacks checks that could be done according to these limitations, as I wrote in details in my prev comments.

As @leigh mentioned, passing controlled sizes to emalloc() is a security risk, that on my 32 bit linux machine caused an mmap failure with the supplied .phar file. I really can't see the difference between this ticket and previous DoS tickets that were caused from unsanitized calls to emalloc() and were handled without such a questioning process.
 [2017-01-16 08:48 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2017-01-16 08:48 UTC] stas@php.net
I was not asking who set the limitations, I was asking how you arrived at the conclusion these limitations exist. 

> As @leigh mentioned, passing controlled sizes to emalloc() is a security risk

Repeating this statement does not make it more true. I do not see how it is a security risk, if you think it is please explain it, not repeat it. I have no idea what "previous tickets" you refer to, but in any case we're discussing this ticket, not any other ones.
 [2017-01-16 08:53 UTC] eyal dot itkin at gmail dot com
-Status: Feedback +Status: Open
 [2017-01-16 08:53 UTC] eyal dot itkin at gmail dot com
The limitations I mentioned before are hardcodes code checks in thr PHAR module. I don't know what was the original design, but I  did wrote all of the code limitations from the module, from the beginning of the handling to it's end.

In adsition, on my computer (standard config) it produces the attached crash (32 bit linux) and so I reported it as a "crash" security report.
 [2020-11-26 15:22 UTC] cmb@php.net
Firstly, this is not related to a corrupt ZIP archive; any archive
which contains a file with very large uncompressed files can
trigger the reported behavior, assuming the script is run on a
32bit architecture, and memory_limit is set to a value too high
for such systems (in the given case memory_limit > 2GB).

This does not look like a security issue.  And we should not put
an arbitrary restriction on the uncompressed size of files in
archives (actually, we may want to support Zip64 some day).

Anyhow, passing unsanitized input to emalloc() is fine in this
case.  What is probaly not reasonable, is to attempt to fully
mmap() such large files at once.  Not long ago we landed a related
fix[1] regarding copying of streams where mmapping the whole file
yielded suboptimal performance.  It may make sense to try to move
this deeper into the streams layer (might not be possible without
BC break), or at least to let very large mmapping attempts fail.

[1] <https://github.com/php/php-src/commit/19c844594e40d79cea016b54f9ab3a367440b4c9>
 [2020-11-27 00:18 UTC] stas@php.net
-Type: Security +Type: Bug
 [2020-12-01 13:25 UTC] cmb@php.net
-Assigned To: +Assigned To: cmb
 [2020-12-01 13:25 UTC] cmb@php.net
My assessment above wasn't correct, since this issue is only
about the .phar/signature.bin file, and restricting its size
appears to be reasonable.

However, this doesn't change the fact that this is not a security
issue, since PHP does not crash the *process*, but rather
terminates the *request* with a fatal error.
 [2020-12-01 13:26 UTC] cmb@php.net
The following pull request has been associated:

Patch Name: Fix #73809: Phar Zip parse crash - mmap fail
On GitHub:  https://github.com/php/php-src/pull/6474
Patch:      https://github.com/php/php-src/pull/6474.patch
 [2020-12-01 16:01 UTC] cmb@php.net
Automatic comment on behalf of cmbecker69@gmx.de
Revision: http://git.php.net/?p=php-src.git;a=commit;h=c283f53b24b84e0571ca2b29df05247a7344392c
Log: Fix #73809: Phar Zip parse crash - mmap fail
 [2020-12-01 16:01 UTC] cmb@php.net
-Status: Assigned +Status: Closed
 
PHP Copyright © 2001-2021 The PHP Group
All rights reserved.
Last updated: Wed Dec 01 23:03:34 2021 UTC