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.

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.

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.

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).

valgrind-3.4.1-Debian on linux doesn't catch the oob read. nnethercote: any ideas on why this might be?

Can someone test if valgrind doesn't complain anymore when the patch is applied?

(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 on attachment 398852 [details] [diff] [review] V1: When depth < 8, mask pixel values within valid range Asking for sr because it's security-related.

Comment on attachment 398852 [details] [diff] [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.

(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 on attachment 400713 [details] [diff] [review] Patch for the 191 branch looks ok to me, but joe should still take a look

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 on attachment 400713 [details] [diff] [review] Patch for the 191 branch Same patch as before. r+

Comment on attachment 400713 [details] [diff] [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!

(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 on attachment 400713 [details] [diff] [review] Patch for the 191 branch Approved for 1.9.0.15, a=dveditz

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

No automated test for this change in mozilla-central or the branches? QA is looking to verify that this issue is fixed.

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.

(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?

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.

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).

(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...

It looks like this bug causes bug 519589

This bug changed a check for mImageFrame into a check for mImageData causing bug 525326. What was the reason for that change?

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.