From d8e06a46a86d694a0d01238b04b51735b59a7846 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Thu, 25 Jul 2019 13:29:44 +1000 Subject: [PATCH] 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 --- includes/libs/mime/MimeAnalyzer.php | 4 +- .../data/media/zip-comment-overflow.png | Bin 0 -> 189 bytes .../data/media/zip-kind-of-valid-2.png | Bin 0 -> 191 bytes .../phpunit/data/media/zip-kind-of-valid.png | Bin 0 -> 189 bytes tests/phpunit/data/media/zip-sig-near-end.png | Bin 0 -> 188 bytes .../includes/libs/mime/MimeAnalyzerTest.php | 36 ++++++++++++++++++ 6 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 tests/phpunit/data/media/zip-comment-overflow.png create mode 100644 tests/phpunit/data/media/zip-kind-of-valid-2.png create mode 100644 tests/phpunit/data/media/zip-kind-of-valid.png create mode 100644 tests/phpunit/data/media/zip-sig-near-end.png 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 0000000000000000000000000000000000000000..710831feb63bc864e6a68b38d2d57e437437beb9 GIT binary patch literal 189 zcmeAS@N?(olHy`uVBq!ia0vp^Mj*_{3?x-PN__%SjKx9jP7LeL$-D$|*aCb)T>t<7 z4`hZOx?BgO{5@S9LnJOICrD^5=<9F)%Q#`(^T!kaGc1|>yHbaxp(M$^pV7l^<92mM z7xnMoRGOZ=t~}T9(rVrumI+NP7P-j`OLY~t^!#DwI9r!+pq7*22!qJc1Aomy&S3C# T^>bP0l+YC5&B~^L1{k;i%&|Pm literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..c0e7ff6129f3e342635cf4f348bbb0978a133671 GIT binary patch literal 191 zcmeAS@N?(olHy`uVBq!ia0vp^Mj*_{3?x-PN__%SjKx9jP7LeL$-D$|*aCb)T>t<7 z4`hZOx?BgO{5@S9LnJOICrD^5=<9F)%Q#`(^T!kaGc1|>yHbaxp(M$^pV7l^<92mM z7xnMoRGOZ=t~}T9(rVrumI+NP7P-j`OLY~t^!#DwI9r!+pq7*22!qJc1Aomy&S3C# V^>bP0l+YC5&B~^L1{jzW6aYq?Jpup# literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..1121af41a987a6cc27127f62907bc4503a1d96ff GIT binary patch literal 189 zcmeAS@N?(olHy`uVBq!ia0vp^Mj*_{3?x-PN__%SjKx9jP7LeL$-D$|*aCb)T>t<7 z4`hZOx?BgO{5@S9LnJOICrD^5=<9F)%Q#`(^T!kaGc1|>yHbaxp(M$^pV7l^<92mM z7xnMoRGOZ=t~}T9(rVrumI+NP7P-j`OLY~t^!#DwI9r!+pq7*22!qJc1Aomy&S3C# T^>bP0l+YC5&B~^L1{fFs%%?oS literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..29f3684baff7445c46e0d339e831a6ef7dc853fd GIT binary patch literal 188 zcmeAS@N?(olHy`uVBq!ia0vp^Mj*_{3?x-PN__%SjKx9jP7LeL$-D$|*aCb)T>t<7 z4`hZOx?BgO{5@S9LnJOICrD^5=<9F)%Q#`(^T!kaGc1|>yHbaxp(M$^pV7l^<92mM z7xnMoRGOZ=t~}T9(rVrumI+NP7P-j`OLY~t^!#DwI9r!+pq7*22!qJc1Aomy&S3C# R^>bP0l+YC5&B~^L4gin`Jm3HT literal 0 HcmV?d00001 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 ); + } } -- 2.20.1