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

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[:Paolo Amadini]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/20080829 Firefox/2.0.0.17 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4

Firefox crashes if an extension calls the open method of the nsIZipWriter interface of "@mozilla.org/zipwriter;1", specifying the name of an existing file which has no ZIP signature inside it, and without specifying the PR_TRUNCATE open flag (that is, trying to append to an existing archive).

The "@mozilla.org/zipwriter;1" component is never called by Firefox itself, only an extension can cause the crash.

Reproducible: Always

Steps to Reproduce:
var file = Components.classes["@mozilla.org/file/directory_service;1"]
                     .getService(Components.interfaces.nsIProperties)
                     .get("TmpD", Components.interfaces.nsIFile);
file.append("malformedTestFile.zip");
file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666);

var zipWriter = Components.classes["@mozilla.org/zipwriter;1"]
 .createInstance(Components.interfaces.nsIZipWriter);

zipWriter.open(file, 0x04); // (0x04 == PR_RDWR)
Actual Results:  
Firefox crashes near line 245 of /modules/libjar/zipwriter/src/nsZipWriter.cpp:

             sig += buf[--pos];



Expected Results:  
The open method should have thrown "NS_ERROR_FILE_CORRUPTED"

The mentioned line may not be the only spot where malformed ZIP files can cause
a memory access violation, although it is the most important because it is
reached every time an append is attempted on a non-ZIP file.

Unfortunately, right now, I don't have the time to test or report them all,
nor to write a proper patch or test case. I might do it in the future if
nobody else addresses the problem in the meantime.

I'm not sure about what is the appropriate severity level; I'm filing this bug
report as "critical", even though it cannot happen on a clean Firefox.

Comment 1

[timeless]

Attachment: check size

touch q.zip;unzip -t q.zip
Archive:  q.zip
  End-of-central-directory signature not found.  Either this file is not
  a zipfile, or it constitutes one disk of a multi-part archive.  In the
  latter case the central directory and zipfile comment will be found on
  the last disk(s) of this archive.
unzip:  cannot find zipfile directory in one of q.zip or
        q.zip.zip, and cannot find q.zip.ZIP, period.

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 351162 [details] [diff] [review]
check size

This will catch the case of a 0 length zip file but it looks like we're still doing bad things for small files. We should check that the size is at least ZIP_EOCDR_HEADER_SIZE, I think that leaves us safe then. A unit test would be helpful too.

Comment 3

[georgi - hopefully not receiving bugspam]

short zip files indeed cause problems.

to reproduce on linux:

1.in shell:
echo 1234 > /tmp/zip1

2. in jsconsole:
var file = Components.classes["@mozilla.org/file/directory_service;1"].getService(Components.interfaces.nsIProperties).get("TmpD", Components.interfaces.nsIFile); file.append("zip1"); file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666);var zipWriter = Components.classes["@mozilla.org/zipwriter;1"].createInstance(Components.interfaces.nsIZipWriter); zipWriter.open(file, 0x04);alert(zipWriter); 

crash:
(gdb) bt
#0  0x00007f8ac304bb81 in nanosleep () from /lib/libc.so.6
#1  0x00007f8ac304b9a4 in sleep () from /lib/libc.so.6
#2  0x00007f8ac6e77b7c in ah_crap_handler (signum=11)
    at /opt/pub/firefox-central/src/toolkit/xre/nsSigHandlers.cpp:149
#3  0x00007f8ac6e78d49 in nsProfileLock::FatalSignalHandler (signo=11)
    at nsProfileLock.cpp:216
#4  <signal handler called>
#5  0x00007f8aad7ee7de in READ32 (buf=0x7fffcf4d3da0 "`0h�\212\177", 
    off=0x7fffcf4d3d8c)
    at /opt/pub/firefox-central/src/modules/libjar/zipwriter/src/StreamFunctions.h:86
#6  0x00007f8aad7f371f in nsZipWriter::ReadFile (this=0x7f8aae6b3c00, 
    aFile=0x7f8aafb8ab40)
    at /opt/pub/firefox-central/src/modules/libjar/zipwriter/src/nsZipWriter.cpp:177
#7  0x00007f8aad7f3f0e in nsZipWriter::Open (this=0x7f8aae6b3c00, 
    aFile=0x7f8aae4fbd00, aIoFlags=4)

(gdb) frame 5
#5  0x00007f8aad7ee7de in READ32 (buf=0x7fffcf4d3da0 "`0h�\212\177", 
    off=0x7fffcf4d3d8c)
    at /opt/pub/firefox-central/src/modules/libjar/zipwriter/src/StreamFunctions.h:86
86        PRUint32 val = (PRUint32)buf[(*off)++] & 0xff;

(gdb) p buf[(*off)++]
Cannot access memory at address 0x8000cf4d3d8b

Comment 4

[:Paolo Amadini]

In addition to the case of short zip files, I noticed the crash every
time the ZIP signature is missing. From the latest code in MXR:

179         while (pos >=0) {
...
244             sig = sig << 8;
245             sig += buf[--pos];
246         }

It seems that "pos" is 0 when the ZIP backwards scanning is finished,
and the crash happens because of the pre-decrement.

A break just before the last instruction would fix this particular crash
instance, but I'm not sure this is the complete solution.

Hope this helps.

Comment 5

[:Paolo Amadini]

Attachment: Fix bug and two compiler warnings. Includes ordinary testcase.

Although this implementation is theoretically not as efficient as the
previous one, it is more readable in my opinion, and removes the
offending instruction "sig += buf[--pos]". Regarding this instruction,
also note that "buf" is a pointer to signed chars, so I'm not sure the
former implementation was correct in the first place.

Other notes:
* READ32 requires "pos" to be unsigned. I casted it to signed only
   while checking for negative values.
* The for statement is split on two lines for readability.
* This patch also fixes a compiler warning by explicitly casting
   "seek" to PRInt32 ("seek"'s absolute value is less than 1024
   when that happens, so the cast is safe).

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 358752 [details] [diff] [review]
Fix bug and two compiler warnings. Includes ordinary testcase.

Pretty much ok, just a couple of fix-ups:

>+/*
>+ * These functions read little-endian data without incrementing the read
>+ * buffer pointer. Borrowed from nsZipArchive.cpp.
>+ */
>+
>+inline NS_HIDDEN_(PRUint32) xtolong (unsigned char *ll)
>+{
>+  return (PRUint32)( (ll [0] <<  0) |
>+                     (ll [1] <<  8) |
>+                     (ll [2] << 16) |
>+                     (ll [3] << 24) );
>+}
>+

Move this above ZW_ReadData and name it and it's arguments something more in line with the other functions, PEEK32 or GET32 or something. The << 0 is redundant. There is only one function so s/these/this/

>         // 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) {
>+        PRUint32 pos = length - ZIP_EOCDR_HEADER_SIZE;
>+        for (; (PRInt32)pos >= 0; pos--) {
>+            PRUint32 sig = xtolong((unsigned char *)buf + pos);

Put the pos initialisation inside the for statement.

Please also just add a check that the file size is large enough since there is no point wasting time opening the file in that case.

>+function run_test()
>+{
>+  // Create an empty file
>+  tmpFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0666);
>+
>+  // Opening the empty file should fail (but not crash)
>+  try {
>+    zipW.open(tmpFile, PR_RDWR);
>+    do_throw("Should have thrown NS_ERROR_FILE_CORRUPTED!");
>+  } catch (e if (e instanceof Ci.nsIException &&
>+                 e.result == Components.results.NS_ERROR_FILE_CORRUPTED)) {
>+    // do nothing
>+  }
>+}

You need to catch the case where an incorrect exception is thrown. I'd just put a return in the catch block and move the do_throw to the end.
Can you also include a test for trying to open a file that is too small to be real.

Comment 7

[:Paolo Amadini]

Attachment: Optimized fix for the bug and two compiler warnings. Includes ordinary testcase.

(In reply to comment #6)
> (From update of attachment 358752 [details] [diff] [review])
> Pretty much ok, just a couple of fix-ups:
> 
> >+inline NS_HIDDEN_(PRUint32) xtolong (unsigned char *ll)
> > [...]
> 
> Move this above ZW_ReadData and name it and it's arguments something more in
> line with the other functions, PEEK32 or GET32 or something. The << 0 is
> redundant. There is only one function so s/these/this/

Done: I integrated the function with the others instead of keeping the
same name and body of the original one, used another approach for the
comment, and removed the << 0 while keeping the original alignment.

> >+        PRUint32 pos = length - ZIP_EOCDR_HEADER_SIZE;
> >+        for (; (PRInt32)pos >= 0; pos--) {
> 
> Put the pos initialisation inside the for statement.

Done. The for statement is now split on two lines because of the 80 character
limit.

> Please also just add a check that the file size is large enough since there is
> no point wasting time opening the file in that case.

Done: I test for ZIP_EOCDR_HEADER_SIZE as you suggested in a previous post.

> >+function run_test()
> >+{
> >+  // Create an empty file
> >+  tmpFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0666);
> >+
> >+  // Opening the empty file should fail (but not crash)
> >+  try {
> >+    zipW.open(tmpFile, PR_RDWR);
> >+    do_throw("Should have thrown NS_ERROR_FILE_CORRUPTED!");
> >+  } catch (e if (e instanceof Ci.nsIException &&
> >+                 e.result == Components.results.NS_ERROR_FILE_CORRUPTED)) {
> >+    // do nothing
> >+  }
> >+}
> 
> You need to catch the case where an incorrect exception is thrown. I'd just
> put a return in the catch block and move the do_throw to the end.

I think my code is correct: only the expected exception is caught, all the
other unexpected exceptions would cause the test to fail, exactly as if for
instance an exception was thrown by the tmpFile.create(...) instruction.
I've seen this approach to handling expected exceptions used consistently
in other portions of the codebase, for instance:

http://mxr.mozilla.org/mozilla-central/source/modules/libjar/test/unit/test_bug458158.js

> Can you also include a test for trying to open a file that is too small to be
> real.

Done: I try to open "test.png" which is big enough but not a ZIP archive.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 364159 [details] [diff] [review]
Optimized fix for the bug and two compiler warnings. Includes ordinary testcase.

>diff --git a/modules/libjar/zipwriter/src/StreamFunctions.h b/modules/libjar/zipwriter/src/StreamFunctions.h
> nsresult nsZipWriter::ReadFile(nsIFile *aFile)
> {
>     PRInt64 size;
>     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);

I guess this is not >= because there must always be a central directory record earlier too? I guess that is ok.

>+        for (PRUint32 pos = length - ZIP_EOCDR_HEADER_SIZE;
>+             (PRInt32)pos >= 0; pos--) {
>+            PRUint32 sig = PEEK32((unsigned char *)buf + pos);

Nit: you have a tab character in the line above.

>+  // Now try reading 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 invalidFile = do_get_file(DATA_DIR + "test.png");
>+
>+  // 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 invalid file!");
>+  } catch (e if (e instanceof Ci.nsIException &&
>+                 e.result == Components.results.NS_ERROR_FILE_CORRUPTED)) {
>+    // do nothing
>+  }
>+}

Actually I was more interested to see a test of a file that was non-zero but still too small to be a zip. Can you add that too, should be just the same just test against a text file less than 22 bytes. Keep the test of test.png too though, that is helpful.

Sorry for the long turnaround on the reviews, this all looks fine, make those small changes and upload a new patch and it is ready for checkin, say if you don't have privileges and I'll get it landed.

Comment 9

[:Paolo Amadini]

Attachment: Optimized fix for the bug and two compiler warnings. Includes ordinary testcase.

(In reply to comment #8)
> (From update of attachment 364159 [details] [diff] [review])
> >+    // 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);
> 
> I guess this is not >= because there must always be a central directory record
> earlier too? I guess that is ok.

Yes, I think the smallest possible ZIP archive would be a few more bytes
larger than ZIP_EOCDR_HEADER_SIZE. Using >= would have hinted towards a
different meaning for the test (which is only an optimization, not a
strict verification), so I used > instead.

> >+            PRUint32 sig = PEEK32((unsigned char *)buf + pos);
> 
> Nit: you have a tab character in the line above.

Fixed, thanks.

> Actually I was more interested to see a test of a file that was non-zero but
> still too small to be a zip. Can you add that too, should be just the same just
> test against a text file less than 22 bytes. Keep the test of test.png too
> though, that is helpful.

Done. To implement the third test I refactored the code a little, please
have a look and see if it is still OK.

> Sorry for the long turnaround on the reviews, this all looks fine, make those
> small changes and upload a new patch and it is ready for checkin, say if you
> don't have privileges and I'll get it landed.

No problem, I understand the need for accurate code review and quality check.
I don't have commit access, so you would do me a favor if you can see that the
patch is checked in, and while you're at it, if you can do the necessary steps
to backport it to the 1.9.1 branch (I don't know what the steps are, actually).

Thanks in advance!

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 366106 [details] [diff] [review]
Optimized fix for the bug and two compiler warnings. Includes ordinary testcase.

r=me

Comment 11

[Dave Townsend [:mossop]]

Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/092468a23f4b

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 366106 [details] [diff] [review]
Optimized fix for the bug and two compiler warnings. Includes ordinary testcase.

Looking for approval to land this simple well tested patch to solve a crasher that extension and application developers might hit.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 366106 [details] [diff] [review]
Optimized fix for the bug and two compiler warnings. Includes ordinary testcase.

a191=beltzner

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0fde46608ad8