ZipBombEngine doesn't check if the archive has been opened, and more.

[twisted1919]

Hello,
Thank you for this awesome library and the time you put into it 👍
I'm trying to use it and i get some issues along the way, hopefully you can take a look and maybe shed some light.

The part where the engine tries to open the archive:

$zip->open($realPath, ZIPARCHIVE::CHECKCONS);

just assumes the open process will always work, which is not the case.
It should be wrapped in something like:

if ($zip->open($realPath, ZIPARCHIVE::CHECKCONS) !== true) {
	        throw new RuntimeException(sprintf('Unable to open: %s', $realPath));
        }

Of course, that code doesn't mean anything since it does not show the actual error, so you can map it like:

if (($result = $zip->open($realPath, ZIPARCHIVE::CHECKCONS)) !== true) {
	        $errorMap = [
		        ZipArchive::ER_EXISTS   => 'File already exists',
		        ZipArchive::ER_INCONS   => 'Zip archive inconsistent.',
		        ZipArchive::ER_INVAL    => 'Invalid argument.',
		        ZipArchive::ER_MEMORY   => 'Malloc failure.',
		        ZipArchive::ER_NOENT    => 'No such file.',
		        ZipArchive::ER_NOZIP    => 'Not a zip archive.',
		        ZipArchive::ER_OPEN     => 'Can\'t open file.',
		        ZipArchive::ER_READ     => 'Read error.',
		        ZipArchive::ER_SEEK     => 'Seek error.',
	        ];
        	$errorReason = $errorMap[(int)$result] ?? 'Unknown error.';
	        throw new RuntimeException(sprintf('Unable to open: %s, reason: %s', $realPath, $errorReason));
        }

Which imho is a better workflow, because it also tells what's wrong, but it's up to you how you want to implement this.

Additionally, opening with ZIPARCHIVE::CHECKCONS flag will often complain with ZipArchive::ER_INCONS error for some valid archives, for example either one from https://www.mailwizz.com/free-email-templates/
Changing the flag to ZipArchive::CREATE seems to fix the issue, so the final code for me was:

if (($result = $zip->open($realPath, ZipArchive::CREATE)) !== true) {
        	$errorMap = [
		        ZipArchive::ER_EXISTS   => 'File already exists',
		        ZipArchive::ER_INCONS   => 'Zip archive inconsistent.',
		        ZipArchive::ER_INVAL    => 'Invalid argument.',
		        ZipArchive::ER_MEMORY   => 'Malloc failure.',
		        ZipArchive::ER_NOENT    => 'No such file.',
		        ZipArchive::ER_NOZIP    => 'Not a zip archive.',
		        ZipArchive::ER_OPEN     => 'Can\'t open file.',
		        ZipArchive::ER_READ     => 'Read error.',
		        ZipArchive::ER_SEEK     => 'Seek error.',
	        ];
        	$errorReason = $errorMap[(int)$result] ?? 'Unknown error.';
	        throw new RuntimeException(sprintf('Unable to open: %s, reason: %s', $realPath, $errorReason));
        }

Further down the line, i believe the final check

// Header uncompressed size must be the same as files uncompressed size
        $result = $size !== $size2;

should actually be:

$result = $size > 0 && $size2 > 0 && $size !== $size2;

But i am not sure about this entirely, so again, this is up to you since you know better how this works.

Best.

[odan]

I just added the additional check for the zip file.

But with this change most tests would fail. So I skip it.

$result = $size > 0 && $size2 > 0 && $size !== $size2;