510247 - Buffer under run on JAR files

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[Peter Naulls]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1) Gecko/20090707 Iceweasel/3.5.1 (Debian-3.5.1-1)
Build Identifier: Firefox 3.6 Mercurial

In nsZipArchive::BuildFileList, the loop steps backwards through the buffer looking for the central directory.  

If bufsize < ZIPEND_SIZE (e.g, the very start of the file, when it would otherwise file to find the central directory), endp will be decremented below buf, and xtolong will deference from before the buffer.

Of course, this assumes that the zip file is malformed, or there's some other reason the central directory is not being found, which is how I found this at all.



Reproducible: Always

Comment 1

[Alfred Kayser]

Now that jar's are mmapped, this is easy to fix.

Comment 2

[Alfred Kayser]

Attachment: V1: Simplify the BuildFileList scanner logic to prevent underruns

This patch simplifies the code of the BuildFileList removing the underrun issue, and making the code overall much more readable, and even a little bit faster.

Comment 3

[(dormant account)]

Comment on attachment 394781 [details] [diff] [review]
V1: Simplify the BuildFileList scanner logic to prevent underruns

>+  PRUint8* buf;
>+  PRUint8* endp = mFd->mFileData + mFd->mLen;
> 
>-  //-- get archive size using end pos
>-  PRInt32  pos = (PRInt32) mFd->mLen;
>+  for (buf = endp - ZIPEND_SIZE; xtolong(buf) != ENDSIG; buf--)
>+  {

Note that a file with no ENDSIG will cause you to read past begining of mmap'ed area. Should check for buf > mFd->mFileData. Either that or use an index and check > 0.

Comment 4

[Alfred Kayser]

This check is within the loop itself:
    if (buf == mFd->mFileData) {
      // We're at the beginning of the file, and still no sign
      // of the end signature.  File must be corrupted!
      return ZIP_ERR_CORRUPT;
    }

Comment 5

[Alfred Kayser]

Comment on attachment 394781 [details] [diff] [review]
V1: Simplify the BuildFileList scanner logic to prevent underruns

Taras, can you do the r of this one?
As said in the previous comment, the check for underrun is within the loop itself.

Comment 6

[(dormant account)]

Comment on attachment 394781 [details] [diff] [review]
V1: Simplify the BuildFileList scanner logic to prevent underruns

looks good to me

Comment 7

[(dormant account)]

Comment on attachment 394781 [details] [diff] [review]
V1: Simplify the BuildFileList scanner logic to prevent underruns

this is good to go in

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/4fa9f187e09c

Comment 9

[(dormant account)]

This is a low risk patch that avoids a potential crash.

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 394781 [details] [diff] [review]
V1: Simplify the BuildFileList scanner logic to prevent underruns

a=me per discussion w/ beltzner et al.

Comment 11

[(dormant account)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6a25259a0d0c