Closed Bug 157646 Opened 19 years ago Closed 18 years ago

Possible heap corruption in libjar


(Core :: Security, defect)

Not set





(Reporter: security-bugs, Assigned: dveditz)



(Whiteboard: [sg:blocker])


(2 files, 1 obsolete file)

Submitter name:                zen-parse
   Submitter email address:
   Acknowledgement checkbox:      on
   Product:                       Netscape 6.x
   Operating system:              Unix: x86 Linux
   OS version:                    Redhat 7.0
   Issue summary:                 Exploitable heap corruption via jar: URI handler.

Issue details:
Details to verify existance of problem.

Create a file, called test.gif with the 
following 6 'int's in it.


$ zip orig.jar test.gif
  adding: test.gif (deflated 17%)
$ unzip -v orig.jar
Archive:  orig.jar
 Length   Method    Size  Ratio   Date   Time 
  CRC-32    Name
--------  ------  ------- -----   ----   ---- 
  ------    ----
      24  Defl:N       20  17%  07-08-02 
16:11  b74deafe  test.gif
--------          -------  ---                
      24               20  17%                
            1 file
$ sed s/`printf '\x18'`/`printf '\x01'`/g 
orig.jar >new.jar
$ unzip -v new.jar 
Archive:  new.jar
 Length   Method    Size  Ratio   Date   Time 
  CRC-32    Name
--------  ------  ------- -----   ----   ---- 
  ------    ----
       1  Defl:N       20 -1900%  07-08-02 
16:11  b74deafe  test.gif
--------          -------  ---                
       1               20 -1900%              
              1 file
$ cp new.jar ~/public_html

(This file only contains the 2 0x18s (24s) 
representing the realsize, so
it works ok on this file. Actual explot uses 
a hex editor.)

In Netscape open:


The jr file is retrieved, the requested file 
is found...
584   //-- Read the item into memory
585   //   Inflate if necessary and save in 
586   //   for sequential reading.
587   //   (nsJAR needs the whole file in 
memory before passing it on)
588   char* buf = 
589   if (!buf) return ZIP_ERR_MEMORY;
590   switch(item->compression)
591   {
592     case DEFLATED:
593       result = InflateItem(item, 0, buf);
594       break;...
A buffer is allocated for storing the data. 
The realsize value is used for
the length. (Size 1 actually allocates 8 
bytes, hence the padding.)
The buf is the passed to the inflater.
1268 PRInt32 nsZipArchive::InflateItem( const 
nsZipItem* aItem, PRFileDesc* fOut,
1269                                    char* 
bigBuf )
as bigBuf. Some temporary storage is made, 
and a chunk of decompression done.
1382       {
1383         //-- copy inflated buffer to our 
big buffer
1384         // Assertion makes sure we don't 
overflow bigBuf
1385         PR_ASSERT( outpos + ZIP_BUFLEN 
<= bigBufSize);
1386         char* copyStart = bigBuf + 
1387         memcpy(copyStart, outbuf, 
1388       } 
The assertion doesn't fire. It should 
probably be made into a normal check as

We now have a heap based buffer overflow.

At some point in the future, chunk_free() is 
called, and a SEGV will occur
with while referencing the values 0xdeadbeef 
and 0xfee1600d.

If these are replaced with (address of a 
function pointer)-12 and (address
of user supplied code), when the function 
pointer is called, the user
supplied code will execute.

I have successfully changed the flow of 
control in tests, by overwriting
the function pointer for PR_Free in the 
global offset table of

"Shellcode" can be supplied in a previously 
loaded image. (A large area
can be filled using compressed image files 
stored in a .jar as the source.)

This form was submitted from
with Mozilla/4.76 (Linux 2.2.19-7.0.16 i686; U) Opera 6.0  [en].
Simple patch for this overflow. 
Adds test that mirrors the assert.

--- mozilla/modules/libjar/nsZipArchive.cpp.orig        Thu Aug 15 03:45:31 2002
+++ mozilla/modules/libjar/nsZipArchive.cpp     Thu Aug 15 03:45:31 2002
@@ -1383,8 +1383,16 @@
         //-- copy inflated buffer to our big buffer
         // Assertion makes sure we don't overflow bigBuf
         PR_ASSERT( outpos + ZIP_BUFLEN <= bigBufSize);
-        char* copyStart = bigBuf + outpos;
-        memcpy(copyStart, outbuf, ZIP_BUFLEN);
+        if(outpos + ZIP_BUFLEN <= bigBufSize)
+        {
+         char* copyStart = bigBuf + outpos;
+         memcpy(copyStart, outbuf, ZIP_BUFLEN);
+        }
+        else
+        {
+         status = ZIP_ERR_CORRUPT;
+         break;
+        }
       outpos = zs.total_out;

Attached file Demonstration zipfile
This is as serious as it gets and we have a patch. Why hasn't it been fixed?
Definite 1.2 blocker.
Dan, could you try out the patch above and get it checked in?
Anyone?  This is a 1.2 bug.  We need traction here.
Attached patch protect against buffer overflow (obsolete) — Splinter Review
Comment on attachment 104422 [details] [diff] [review]
protect against buffer overflow

looks fine. r=mstoltz.
Attachment #104422 - Flags: review+
Comment on attachment 104422 [details] [diff] [review]
protect against buffer overflow

>Index: nsZipArchive.cpp

>         PR_ASSERT( outpos + ZIP_BUFLEN <= bigBufSize);
>+        if ( outpos + ZIP_BUFLEN <= bigBufSize )
>+        else
>+        {
>+          status = ZIP_ERR_CORRUPT;
>+          break;
>+        }

nit: how about moving the PR_ASSERT into the |else| block.  PR_NOT_REACHED
is probably more appropriate since it avoids duplicating the expression.

>       PR_ASSERT( (outpos + chunk) <= bigBufSize );
>+      if ( outpos + ZIP_BUFLEN <= bigBufSize )

these two lines don't match!  shouldn't |ZIP_BUFLEN| be |chunk| instead?
This fixes the bug Darin noticed and removes the now-unnecessary PR_ASSERTs.
Note it's a -w patch so the indents appear slightly off.
Attachment #104422 - Attachment is obsolete: true
Comment on attachment 104588 [details] [diff] [review]
Fix review comments

Carrying over mitch's r=
Attachment #104588 - Flags: review+
This should go on the branch, too.
Comment on attachment 104588 [details] [diff] [review]
Fix review comments

a=roc+moz for trunk
Attachment #104588 - Flags: approval+
Attachment #104588 - Flags: superreview+
Comment on attachment 104588 [details] [diff] [review]
Fix review comments

Discussed in bBird team meeting.  Plussing for adt only.
Keywords: adt1.0.2adt1.0.2+
Fix checked into trunk
Closed: 18 years ago
Resolution: --- → FIXED
a=chofmann for 1.0.2 as well
fixed on the branch
Mitch is still looking for a demonstrable exploit. I could crash, but not with
the supplied addresses, and it hasn't been shown that an attacker could predict
the address of any attack code that could be loaded as an image.
With regard to exploitablity:

 Same method for predicted return address as the PNG exploits works for NS6.2.3,
which was what this bug was originally reported in.

 Make a 12-16meg PNG image, with 1/2 of it a NOP sleep, and the other 1/2 the
pointers to the shellcode. In my example exploit for the other jar bug, I used
0x41ab005e as my return address, which was, under redhat 7.0, allocated for
storing the png image, so I did the same for this one.

7a 65 6e 2d 68 65 72 65 00 00 00 00 00 00 00 00 64 02 1c 40 5e 00 ab 41

That is the hex for contents of the test.gif I successfully used for explotation
on Redhat 7.0, with Netscape 6.2.3.

Under Netscape 7.0 release, the is more difficult, but not impossilbe to
exploit.(Could maybe also create a string of a size that would force it to be
allocated with javascript, of the same length/format the the PNG image
mentioned.) It might also be possible to overwrite some stack address with the
chunk and change the flow of execution with that method, or to change the value
of some address containing a flag that suggests something might be a safe
operation to perform when it is not.

It could still be exploitable by overwriting function pointers with the address
of a call *eax, or similar, where the register contains the code to execute, or
a pointer to such code. 

There are many different ways of exploiting an arbitrary length heap overrun.

Verified on 2002-11-06-branch on Win 2000.

All the test cases in comment #3, works fine. None of them crashes and all of
them gives the error as invalid attachment.

Please let me know if any other verification is needed.
Group: security
You need to log in before you can comment on or make changes to this bug.