GIF with out-of-bounds colormap reference causes OOB read in nsGIFDecoder2::OutputRow

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: keeler, Assigned: alfredkayser)

Tracking

({verified1.9.0.15, verified1.9.1})

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
blocking1.9.0.15 +
wanted1.9.0.x +
blocking1.8.1.next -
wanted1.8.1.x -

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .4+, status1.9.1 .4-fixed)

Details

(Whiteboard: [sg:low] OOB memory read)

Attachments

(4 attachments, 1 obsolete attachment)

Posted image file that exposes bug (obsolete) —
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.
Group: core-security
Whiteboard: [sg:low] OOB memory read
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).
Assignee: nobody → alfredkayser
Status: NEW → ASSIGNED
Attachment #398852 - Flags: review?(joe)
Attachment #398772 - Attachment is obsolete: true
Summary: crafted GIF image data can cause an out of bounds read at nsGIFDecoder2.cpp:493 (nsGIFDecoder2::OutputRow) → GIF with out-of-bounds colormap reference causes OOB read in nsGIFDecoder2::OutputRow
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.
Attachment #398852 - Flags: superreview?(vladimir)
Attachment #398852 - Flags: review?(joe)
Attachment #398852 - Flags: review+
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.
Attachment #398852 - Flags: superreview?(vladimir) → superreview+
http://hg.mozilla.org/mozilla-central/rev/ff3496b1f6c7
Status: ASSIGNED → RESOLVED
blocking1.9.1: --- → ?
Closed: 10 years ago
Flags: blocking1.9.2?
Flags: blocking1.9.0.15?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
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.
Attachment #398852 - Flags: approval1.9.2?
Whiteboard: [sg:low] OOB memory read → [sg:low] OOB memory read [need new patch for 1.9.1/1.9.0]
blocking1.9.1: ? → .4+
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.15?
Flags: blocking1.9.0.15+
Flags: blocking1.8.1.next?
(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.
Attachment #400713 - Flags: superreview?(vladimir)
Attachment #400713 - Flags: review?(joe)
Attachment #400713 - Flags: approval1.9.1.4?
Attachment #400713 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 400713 [details] [diff] [review]
Patch for the 191 branch

looks ok to me, but joe should still take a look
Whiteboard: [sg:low] OOB memory read [need new patch for 1.9.1/1.9.0] → [sg:low][needs r=joe for 1.9.1 patch][needs 1.9.0 patch?] OOB memory read
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+
Attachment #400713 - Flags: review?(joe) → review+
Attachment #400713 - Flags: approval1.9.1.4? → approval1.9.1.4+
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!
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Whiteboard: [sg:low][needs r=joe for 1.9.1 patch][needs 1.9.0 patch?] OOB memory read → [sg:low][needs 1.9.0 patch?] OOB memory read
Flags: blocking1.8.1.next+ → blocking1.8.1.next?
Attachment #400713 - Flags: approval1.9.0.15?
(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.
Whiteboard: [sg:low][needs 1.9.0 patch?] OOB memory read → [sg:low] OOB memory read
Flags: blocking1.9.2? → blocking1.9.2+
Attachment #398852 - Flags: approval1.9.2?
Attachment #400713 - Flags: approval1.9.0.15? → approval1.9.0.15+
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
Keywords: fixed1.9.0.15
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...
Flags: wanted1.8.1.x-
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next?
Flags: blocking1.8.1.next-
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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.