Bug #73809 [PATCH]: Phar Zip parse crash - mmap fail

Edit report at https://bugs.php.net/bug.php?id=73809&edit=1 ID: 73809 Patch added by: cmb@php.net Reported by: eyal dot itkin at gmail dot com Summary: Phar Zip parse crash - mmap fail Status: Assigned Type: Bug Package: PHAR related PHP Version: 7.1.0 Assigned To: cmb Block user comment: N Private report: N New Comment: 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 Previous Comments: ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ [2017-01-16 08:44:06] 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. ------------------------------------------------------------------------ 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