From: Tim Starling <tstarling@wikimedia.org>
Date: Thu, 25 Jul 2019 03:29:44 +0000 (+1000)
Subject: MimeAnalyzer: fix ZIP parsing failure
X-Git-Tag: 1.34.0-rc.0~887^2
X-Git-Url: http://git.cyclocoop.org/ecrire?a=commitdiff_plain;h=d8e06a46a86d694a0d01238b04b51735b59a7846;p=lhc%2Fweb%2Fwiklou.git

MimeAnalyzer: fix ZIP parsing failure

unpack() actually returns an array with indexes starting from 1, not
zero, so unpack(...)[0] gives a notice and always returns null. It is
lucky that ZIPs normally have zero-length comments, so this would have
had little impact on file type detection aside from log spam.

Also, add a check to make sure the unpack() will not read beyond
the end of the file. Without this, unpack() could generate a warning.

The bug was introduced by me in f12db3804882272794b.

Add tests. The test files were generated by appending an EOCDR signature
and some extra bytes to 1bit-png.png.

Bug: T223728
Change-Id: I6fab63102d1d8eea92cdcce5ab6d1eb747a0a890
---

diff --git a/includes/libs/mime/MimeAnalyzer.php b/includes/libs/mime/MimeAnalyzer.php
index 24621748ed..bafe5e3098 100644
--- a/includes/libs/mime/MimeAnalyzer.php
+++ b/includes/libs/mime/MimeAnalyzer.php
@@ -806,10 +806,10 @@ EOT;
 
 		// Check for ZIP variants (before getimagesize)
 		$eocdrPos = strpos( $tail, "PK\x05\x06" );
-		if ( $eocdrPos !== false ) {
+		if ( $eocdrPos !== false && $eocdrPos <= strlen( $tail ) - 22 ) {
 			$this->logger->info( __METHOD__ . ": ZIP signature present in $file\n" );
 			// Check if it really is a ZIP file, make sure the EOCDR is at the end (T40432)
-			$commentLength = unpack( "n", substr( $tail, $eocdrPos + 20 ) )[0];
+			$commentLength = unpack( "n", substr( $tail, $eocdrPos + 20 ) )[1];
 			if ( $eocdrPos + 22 + $commentLength !== strlen( $tail ) ) {
 				$this->logger->info( __METHOD__ . ": ZIP EOCDR not at end. Not a ZIP file." );
 			} else {
diff --git a/tests/phpunit/data/media/zip-comment-overflow.png b/tests/phpunit/data/media/zip-comment-overflow.png
new file mode 100644
index 0000000000..710831feb6
Binary files /dev/null and b/tests/phpunit/data/media/zip-comment-overflow.png differ
diff --git a/tests/phpunit/data/media/zip-kind-of-valid-2.png b/tests/phpunit/data/media/zip-kind-of-valid-2.png
new file mode 100644
index 0000000000..c0e7ff6129
Binary files /dev/null and b/tests/phpunit/data/media/zip-kind-of-valid-2.png differ
diff --git a/tests/phpunit/data/media/zip-kind-of-valid.png b/tests/phpunit/data/media/zip-kind-of-valid.png
new file mode 100644
index 0000000000..1121af41a9
Binary files /dev/null and b/tests/phpunit/data/media/zip-kind-of-valid.png differ
diff --git a/tests/phpunit/data/media/zip-sig-near-end.png b/tests/phpunit/data/media/zip-sig-near-end.png
new file mode 100644
index 0000000000..29f3684baf
Binary files /dev/null and b/tests/phpunit/data/media/zip-sig-near-end.png differ
diff --git a/tests/phpunit/includes/libs/mime/MimeAnalyzerTest.php b/tests/phpunit/includes/libs/mime/MimeAnalyzerTest.php
index 0f23b8cdc1..51ad915d44 100644
--- a/tests/phpunit/includes/libs/mime/MimeAnalyzerTest.php
+++ b/tests/phpunit/includes/libs/mime/MimeAnalyzerTest.php
@@ -163,4 +163,40 @@ class MimeAnalyzerTest extends PHPUnit\Framework\TestCase {
 		];
 	}
 
+	function providePngZipConfusion() {
+		return [
+			[
+				'An invalid ZIP file due to the signature being too close to the ' .
+					'end to accomodate an EOCDR',
+				'zip-sig-near-end.png',
+				'image/png',
+			],
+			[
+				'An invalid ZIP file due to the comment length running beyond the ' .
+					'end of the file',
+				'zip-comment-overflow.png',
+				'image/png',
+			],
+			[
+				'A ZIP file similar to the above, but without either of those two ' .
+					'problems. Not a valid ZIP file, but it passes MimeAnalyzer\'s ' .
+					'definition of a ZIP file. This is mostly a sanity check of the ' .
+					'above two tests.',
+				'zip-kind-of-valid.png',
+				'application/zip',
+			],
+			[
+				'As above with non-zero comment length',
+				'zip-kind-of-valid-2.png',
+				'application/zip',
+			],
+		];
+	}
+
+	/** @dataProvider providePngZipConfusion */
+	function testPngZipConfusion( $description, $fileName, $expectedType ) {
+		$file = __DIR__ . '/../../../data/media/' . $fileName;
+		$actualType = $this->doGuessMimeType( [ $file, 'png' ] );
+		$this->assertEquals( $expectedType, $actualType, $description );
+	}
 }