Bug 467740: Crash [@ nsZipWriter::ReadFile] calling nsIZipWriter.open without PR_TRUNCATE on an existing malformed zip file. r=dtownsend

modules/libjar/zipwriter/src/StreamFunctions.h

@@ -47,8 +47,8 @@
 /*
  * ZIP file data is stored little-endian. These are helper functions to read and
  * write little endian data to/from a char buffer.
- * The off argument is incremented according to the number of bytes consumed
- * from the buffer.
+ * The off argument, where present, is incremented according to the number of
+ * bytes consumed from the buffer.
  */
 inline NS_HIDDEN_(void) WRITE8(char* buf, PRUint32* off, PRUint8 val)
 {
@@ -90,6 +90,14 @@ inline NS_HIDDEN_(PRUint32) READ32(char* buf, PRUint32* off)
   return val;
 }
 
+inline NS_HIDDEN_(PRUint32) PEEK32(unsigned char *buf)
+{
+  return (PRUint32)( (buf [0]      ) |
+                     (buf [1] <<  8) |
+                     (buf [2] << 16) |
+                     (buf [3] << 24) );
+}
+
 NS_HIDDEN_(nsresult) ZW_ReadData(nsIInputStream *aStream, char *aBuffer, PRUint32 aCount);
 
 NS_HIDDEN_(nsresult) ZW_WriteData(nsIOutputStream *aStream, const char *aBuffer,

modules/libjar/zipwriter/src/nsZipWriter.cpp

@@ -139,6 +139,10 @@ nsresult nsZipWriter::ReadFile(nsIFile *aFile)
     nsresult rv = aFile->GetFileSize(&size);
     NS_ENSURE_SUCCESS(rv, rv);
 
+    // If the file is too short, it cannot be a valid archive, thus we fail
+    // without even attempting to open it
+    NS_ENSURE_TRUE(size > ZIP_EOCDR_HEADER_SIZE, NS_ERROR_FILE_CORRUPTED);
+
     nsCOMPtr<nsIInputStream> inputStream;
     rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), aFile);
     NS_ENSURE_SUCCESS(rv, rv);
@@ -147,16 +151,14 @@ nsresult nsZipWriter::ReadFile(nsIFile *aFile)
     PRInt64 seek = size - 1024;
     PRUint32 length = 1024;
 
-    if (seek < 0) {
-        length += seek;
-        seek = 0;
-    }
-
-    PRUint32 pos;
-    PRUint32 sig = 0;
     nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(inputStream);
 
     while (true) {
+        if (seek < 0) {
+            length += (PRInt32)seek;
+            seek = 0;
+        }
+
         rv = seekable->Seek(nsISeekableStream::NS_SEEK_SET, seek);
         if (NS_FAILED(rv)) {
             inputStream->Close();
@@ -173,10 +175,9 @@ nsresult nsZipWriter::ReadFile(nsIFile *aFile)
          * CDS signature
          */
         // We know it's at least this far from the end
-        pos = length - ZIP_EOCDR_HEADER_SIZE;
-        sig = READ32(buf, &pos);
-        pos -= 4;
-        while (pos >=0) {
+        for (PRUint32 pos = length - ZIP_EOCDR_HEADER_SIZE;
+             (PRInt32)pos >= 0; pos--) {
+            PRUint32 sig = PEEK32((unsigned char *)buf + pos);
             if (sig == ZIP_EOCDR_HEADER_SIGNATURE) {
                 // Skip down to entry count
                 pos += 10;
@@ -241,8 +242,6 @@ nsresult nsZipWriter::ReadFile(nsIFile *aFile)
 
                 return inputStream->Close();
             }
-            sig = sig << 8;
-            sig += buf[--pos];
         }
 
         if (seek == 0) {
@@ -253,10 +252,6 @@ nsresult nsZipWriter::ReadFile(nsIFile *aFile)
 
         // Overlap by the size of the end of cdr
         seek -= (1024 - ZIP_EOCDR_HEADER_SIZE);
-        if (seek < 0) {
-            length += seek;
-            seek = 0;
-        }
     }
     // Will never reach here in reality
     NS_NOTREACHED("Loop should never complete");

modules/libjar/zipwriter/test/unit/data/smallfile.txt

@@ -0,0 +1 @@
+Small (16 bytes)

modules/libjar/zipwriter/test/unit/test_bug467740.js

@@ -0,0 +1,62 @@
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Zip Writer Component.
+ *
+ * The Initial Developer of the Original Code is
+ * Dave Townsend <dtownsend@oxymoronical.com>.
+ *
+ * Portions created by the Initial Developer are Copyright (C) 2008
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK *****
+ */
+
+function run_test()
+{
+  // In this test we try to open some files that aren't archives:
+  //  - An empty file, that is certainly not an archive.
+  //  - A file that couldn't be mistaken for archive, since it is too small.
+  //  - A file that could be mistaken for archive, if we checked only the file
+  //     size, but is invalid since it contains no ZIP signature.
+  var invalidArchives = ["emptyfile.txt", "smallfile.txt", "test.png"];
+
+  invalidArchives.forEach(function(invalidArchive) {
+    // Get a reference to the invalid file
+    var invalidFile = do_get_file(DATA_DIR + invalidArchive);
+
+    // Opening the invalid file should fail (but not crash)
+    try {
+      zipW.open(invalidFile, PR_RDWR);
+      do_throw("Should have thrown NS_ERROR_FILE_CORRUPTED on " +
+               invalidArchive + " !");
+    } catch (e if (e instanceof Ci.nsIException &&
+                   e.result == Components.results.NS_ERROR_FILE_CORRUPTED)) {
+      // do nothing
+    }
+  });
+}