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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: 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

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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]

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]

Attachment: 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

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: file that exposes bug

Comment 5

[Jesse Ruderman]

Attachment: valgrind output (mac)

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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]

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

Comment 8

[Nicholas Nethercote [inactive]]

(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 (not getting mail)]

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 10

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/ff3496b1f6c7

Comment 11

[Reed Loden [:reed]]

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.

Comment 12

[Julian Seward [:jseward]]

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

Attachment: Patch for the 191 branch

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 400713 [details] [diff] [review]
Patch for the 191 branch

looks ok to me, but joe should still take a look

Comment 15

[Alfred Kayser]

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 (not getting mail)]

Comment on attachment 400713 [details] [diff] [review]
Patch for the 191 branch

Same patch as before. r+

Comment 17

[Daniel Veditz [:dveditz]]

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!

Comment 18

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e4477e90c539

Comment 19

[Reed Loden [:reed]]

(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 [:dveditz]]

Comment on attachment 400713 [details] [diff] [review]
Patch for the 191 branch

Approved for 1.9.0.15, a=dveditz

Comment 21

[Reed Loden [:reed]]

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]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c946223c5bd3

Comment 23

[Al Billings [:abillings - ex-MoCo]]

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]

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]]

(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 - ex-MoCo]]

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 - ex-MoCo]]

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

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

It looks like this bug causes bug 519589

Comment 30

[Jeff Muizelaar [:jrmuizel]]

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]

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.