Edit report at https://bugs.php.net/bug.php?id=73809&edit=1
ID: 73809
Updated by: cmb@php.net
Reported by: eyal dot itkin at gmail dot com
Summary: Phar Zip parse crash - mmap fail
-Status: Assigned
+Status: Closed
Type: Bug
Package: PHAR related
PHP Version: 7.1.0
Assigned To: cmb
Block user comment: N
Private report: N
New Comment:
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
Previous Comments:
------------------------------------------------------------------------
[2020-12-01 13:26:52] 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 13:25:36] 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-11-26 15:22:40] 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>
------------------------------------------------------------------------
[2017-01-16 08:53:46] 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.
------------------------------------------------------------------------
[2017-01-16 08:48:37] 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.
------------------------------------------------------------------------
The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at
https://bugs.php.net/bug.php?id=73809
--
Edit this bug report at https://bugs.php.net/bug.php?id=73809&edit=1