Last Comment Bug 514776 - GIF with out-of-bounds colormap reference causes OOB read in nsGIFDecoder2::OutputRow
: GIF with out-of-bounds colormap reference causes OOB read in nsGIFDecoder2::O...
Status: RESOLVED FIXED
: [sg:low] OOB memory read
: verified1.9.0.15, verified1.9.1
Product: Core
Classification: Components
Component: ImageLib
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: Alfred Kayser
: imagelib
:
: 519589
:
  Show dependency treegraph
 
Reported: 2009-09-04 15:11 PDT by David Keeler
Modified: 2009-11-09 19:00 PST (History)
16 users (show)
vladimir: blocking1.9.2+
dveditz: blocking1.9.0.15+
dveditz: wanted1.9.0.x+
samuel.sidler+old: blocking1.8.1.next-
samuel.sidler+old: wanted1.8.1.x-
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta1-fixed
  .4+
  .4-fixed


Attachments
file that exposes bug (311.62 KB, image/gif)
2009-09-04 15:11 PDT, David Keeler
no flags Details
V1: When depth < 8, mask pixel values within valid range (4.83 KB, patch)
2009-09-05 08:42 PDT, Alfred Kayser
joe: review+
vladimir: superreview+
Details | Diff | Splinter Review
file that exposes bug (294.43 KB, image/gif)
2009-09-11 09:28 PDT, David Keeler
no flags Details
valgrind output (mac) (2.05 KB, text/plain)
2009-09-11 10:50 PDT, Jesse Ruderman
no flags Details
Patch for the 191 branch (4.02 KB, patch)
2009-09-15 01:58 PDT, Alfred Kayser
joe: review+
vladimir: superreview+
dveditz: approval1.9.1.4+
dveditz: approval1.9.0.15+
Details | Diff | Splinter Review

Summon comment box

Description David Keeler 2009-09-04 15:11:27 PDT
Created attachment 398772 [details]
file that exposes bug

Build Identifier: mozilla-central revision 91096486c92c

If a GIF image block header specifies a small local color map, the decoder can be made to read off the end of the allocated color map if the image has pixels with color indices greater than the size of the map.

The code in question is:
(line 493 is the important one)

nsGIFDecoder2.cpp:

nsGIFDecoder2::OutputRow():
484:    // Row to process
485:    const PRUint32 bpr = sizeof(PRUint32) * mGIFStruct.width; 
486:    PRUint8 *rowp = mImageData + (mGIFStruct.irow * bpr);
487:
488:    // Convert color indices to Cairo pixels
489:    PRUint8 *from = rowp + mGIFStruct.width;
490:    PRUint32 *to = ((PRUint32*)rowp) + mGIFStruct.width;
491:    PRUint32 *cmap = mColormap;
492:    for (PRUint32 c = mGIFStruct.width; c > 0; c--) {
493:      *--to = cmap[*--from];
494:    }

(where mColormap will be allocated in nsGIFDecoder2::GifWrite:
1097:      if (q[8] & 0x80) /* has a local colormap? */
1098:      {
1099:        mGIFStruct.local_colormap_size = 1 << depth;
1100:        if (!mGIFStruct.images_decoded) {
1101:          // First frame has local colormap, allocate space for it
1102:          // as the image frame doesn't have its own palette
1103:          mColormapSize = sizeof(PRUint32) << realDepth;
1104:          if (!mGIFStruct.local_colormap) {
1105:            mGIFStruct.local_colormap = 
                  (PRUint32*)PR_MALLOC(mColormapSize);
1106:            if (!mGIFStruct.local_colormap) {
1107:              mGIFStruct.state = gif_oom;
1108:              break;
1109:            }
1110:          }
1111:          mColormap = mGIFStruct.local_colormap;
1112:        }
             ...

As evidence that this actually happens:
(gdb) print mGIFStruct.local_colormap_size
$26 = 64
(gdb) print /d *from
$27 = 206
(the code is trying to look at element 206 of an array only 64 elements long)

I believe that this is a limited size read-only bug that has minimal security implications, and therefore I am not marking this as security-sensitive.
Comment 1 David Keeler 2009-09-04 15:33:15 PDT
Actually, I retract that last statement.  I believe up to 4 * 254 bytes of memory following the color map can be read, which could expose sensitive information.
Comment 2 Alfred Kayser 2009-09-05 02:10:56 PDT
Actually there are three places for a colormap
1. Global colormap (but this one is always 256 colors big)
2. Local colormap within the frame data
3. Local colormap allocated at line 1105 
(Only happens when the first frame has its own local colorpalette).

Two solutions here, both with performance impact:
1. Make the local colormaps always 256 colors large.
2. In the mapping function (line 493), clamp colorvaluess to the colormap size.
Comment 3 Alfred Kayser 2009-09-05 08:42:10 PDT
Created attachment 398852 [details] [review]
V1: When depth < 8, mask pixel values within valid range

This patch ensure that pixels values will never be larger than colormap, by masking them to 'mColorMask' so that any (illegal) values larger than specified depth are masked into a valid value.
This prevents 'reading' memory that one should be allowed to.
Increasing depth to accommodate invalid tpixel is just wrong, and also allows out of bound reads.
So, I also removed 'realDepth' which was needed for transparent pixels outside the colormap range, but resetting the transparent flag when this happens (as the image won't be transparent anyway, as the tpixel index is outside the valid colormap range).
Comment 4 David Keeler 2009-09-11 09:28:42 PDT
Created attachment 400066 [details]
file that exposes bug
Comment 5 Jesse Ruderman 2009-09-11 10:50:37 PDT
Created attachment 400093 [details]
valgrind output (mac)
Comment 6 David Keeler 2009-09-11 11:06:32 PDT
valgrind-3.4.1-Debian on linux doesn't catch the oob read.
nnethercote: any ideas on why this might be?
Comment 7 Alfred Kayser 2009-09-11 11:11:36 PDT
Can someone test if valgrind doesn't complain anymore when the patch is applied?
Comment 8 Nicholas Nethercote [:njn] 2009-09-11 15:53:03 PDT
(In reply to comment #6)
> valgrind-3.4.1-Debian on linux doesn't catch the oob read.
> nnethercote: any ideas on why this might be?

Does PR_MALLOC end up going through jemalloc on Linux?  I think jemalloc isn't used on Mac, right?

It's relevant because Valgrind is less accurate on jemalloc-enabled builds, partly because the annotations in jemalloc are a bit dodgy (see bug 503249), and partly because jemalloc doesn't pad both ends of malloc'd blocks with a redzone.
Comment 9 Joe Drew (:JOEDREW!) 2009-09-11 21:22:07 PDT
Comment on attachment 398852 [details] [review]
V1: When depth < 8, mask pixel values within valid range

Asking for sr because it's security-related.
Comment 10 Reed Loden [:reed] (very busy) 2009-09-11 22:28:43 PDT
http://hg.mozilla.org/mozilla-central/rev/ff3496b1f6c7
Comment 11 Reed Loden [:reed] (very busy) 2009-09-11 22:31:24 PDT
Comment on attachment 398852 [details] [review]
V1: When depth < 8, mask pixel values within valid range

Patch applies successfully on 1.9.2. Does not apply on 1.9.1/1.9.0, so need different patch for those branches.
Comment 12 Julian Seward 2009-09-14 14:42:45 PDT
(In reply to comment #8)
> It's relevant because Valgrind is less accurate on jemalloc-enabled
> builds,

Seconding that; I'd say "much less accurate".  I'd strongly
advise using a "ac_add_options --disable-jemalloc" build if you want
to reliably use Memcheck to debug and verify such problems.
Comment 13 Alfred Kayser 2009-09-15 01:58:36 PDT
Created attachment 400713 [details] [review]
Patch for the 191 branch
Comment 14 Vladimir Vukicevic (:vlad) 2009-09-15 03:02:30 PDT
Comment on attachment 400713 [details] [review]
Patch for the 191 branch

looks ok to me, but joe should still take a look
Comment 15 Alfred Kayser 2009-09-17 00:04:30 PDT
This patch is not needed for 1.8 nor 1.8.0, as there is an explicit check on colormap size before accessing it:
http://mxr.mozilla.org/mozilla1.8/source/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp#520
520             if (*rowBufIndex < cmapsize) {
and for 1.8.0:
http://mxr.mozilla.org/mozilla1.8.0/source/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp#520
Comment 16 Joe Drew (:JOEDREW!) 2009-09-17 11:19:36 PDT
Comment on attachment 400713 [details] [review]
Patch for the 191 branch

Same patch as before. r+
Comment 17 Daniel Veditz 2009-09-17 11:22:10 PDT
Comment on attachment 400713 [details] [review]
Patch for the 191 branch

Approved for 1.9.1.4, a=dveditz

This probably applies fine to 1.9.0.x -- if it does please request 1.9.0.15 approval as well. Thanks!
Comment 18 Reed Loden [:reed] (very busy) 2009-09-18 08:35:20 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e4477e90c539
Comment 19 Reed Loden [:reed] (very busy) 2009-09-18 08:39:26 PDT
(In reply to comment #17)
> This probably applies fine to 1.9.0.x -- if it does please request 1.9.0.15
> approval as well. Thanks!

It applies fine. approval requested.
Comment 20 Daniel Veditz 2009-09-18 19:06:55 PDT
Comment on attachment 400713 [details] [review]
Patch for the 191 branch

Approved for 1.9.0.15, a=dveditz
Comment 21 Reed Loden [:reed] (very busy) 2009-09-18 19:42:41 PDT
Checking in modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v  <--  nsGIFDecoder2.cpp
new revision: 1.104; previous revision: 1.103
done
Checking in modules/libpr0n/decoders/gif/nsGIFDecoder2.h;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.h,v  <--  nsGIFDecoder2.h
new revision: 1.39; previous revision: 1.38
done
Comment 22 Reed Loden [:reed] (very busy) 2009-09-18 19:59:18 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c946223c5bd3
Comment 23 Al Billings [:abillings] 2009-09-21 15:13:16 PDT
No automated test for this change in mozilla-central or the branches? QA is looking to verify that this issue is fixed.
Comment 24 Jesse Ruderman 2009-09-22 01:46:40 PDT
Do we want this GIF to become part of a reftest, checking for the exact behavior we chose, or just crashtest that can be run under Valgrind?  (I say "chose" because Safari shows it mostly white, Opera shows it mostly black, and Firefox shows it mostly striped!)

For now your best bet is to eyeball the image (it will look randomish and/or crash in bugs without the fix), or run Firefox under Valgrind/Purify.
Comment 25 Reed Loden [:reed] (very busy) 2009-09-22 01:52:30 PDT
(In reply to comment #23)
> No automated test for this change in mozilla-central or the branches? QA is
> looking to verify that this issue is fixed.

Note that even if we had a test, don't we usually hold off on landing such tests for security bugs until the release has shipped?
Comment 26 Al Billings [:abillings] 2009-09-22 09:15:17 PDT
We do but that doesn't mean it wouldn't be attached here for me to see or run (depending on the kind of test) since I have access privs.
Comment 27 Al Billings [:abillings] 2009-09-22 10:14:36 PDT
Verified for 1.9.1.4 using the attached gif and  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090922 Shiretoko/3.5.4pre (.NET CLR 3.5.30729).

Verified for 1.9.0.15 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.15pre) Gecko/2009092205 GranParadiso/3.0.15pre (.NET CLR 3.5.30729).
Comment 28 Samuel Sidler (old account; do not CC) 2009-10-05 09:05:44 PDT
(In reply to comment #15)
> This patch is not needed for 1.8 nor 1.8.0, as there is an explicit check on
> colormap size before accessing it:
> http://mxr.mozilla.org/mozilla1.8/source/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp#520
> 520             if (*rowBufIndex < cmapsize) {
> and for 1.8.0:
> http://mxr.mozilla.org/mozilla1.8.0/source/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp#520

Good to know. Changing those flags then...
Comment 29 Jeff Muizelaar [:jrmuizel] 2009-10-15 08:52:13 PDT
It looks like this bug causes bug 519589
Comment 30 Jeff Muizelaar [:jrmuizel] 2009-10-30 14:26:19 PDT
This bug changed a check for mImageFrame into a check for mImageData causing bug 525326. What was the reason for that change?
Comment 31 Alfred Kayser 2009-10-31 12:48:49 PDT
The problem that caused this change from mImageFrame to mImageData, was that with the decode-on-demand, the BeginImageFrame could fail to set mImageData.
The check should indeed to check both of them.

Note You need to log in before you can comment on or make changes to this bug.