404898 - Purify reports IPW in the gif decoder.

Status: RESOLVED WORKSFORME

(Core :: Graphics: ImageLib, defect, P4)

Description

[Johnny Stenback (:jst)]

I recently ran firefox (trunk) under purify and I saw this (starting Firefox, loading the default start page http://www.mozilla.org/projects/minefield/):

[E] IPW: Invalid pointer write in nsGIFDecoder2::OutputRow(void) {1 occurrence}
        Writing 4 bytes to 0x0d692ffc (1 byte at 0x0d692fff illegal)
        Address 0x0d692ffc points into a committed VirtualAlloc'd block 
        Thread ID: 0xecc
        Error location
            nsGIFDecoder2::OutputRow(void) [e:\tip\mozilla\modules\libpr0n\decoders\gif\nsgifdecoder2.cpp:458]
                    PRUint32 *to = ((PRUint32*)rowp) + mGIFStruct.width;
                    PRUint32 *cmap = mColormap;
                    for (PRUint32 c = mGIFStruct.width; c > 0; c--) {
             =>       *--to = cmap[*--from];
                    }
                
                    // Duplicate rows
            nsGIFDecoder2::DoLzw(BYTE const*) [e:\tip\mozilla\modules\libpr0n\decoders\gif\nsgifdecoder2.cpp:617]
            nsGIFDecoder2::GifWrite(BYTE const*,UINT) [e:\tip\mozilla\modules\libpr0n\decoders\gif\nsgifdecoder2.cpp:715]
            nsGIFDecoder2::ProcessData(BYTE *,UINT,UINT *) [e:\tip\mozilla\modules\libpr0n\decoders\gif\nsgifdecoder2.cpp:242]
            ReadDataOut    [e:\tip\mozilla\modules\libpr0n\decoders\gif\nsgifdecoder2.cpp:187]
            nsGIFDecoder2::WriteFrom(nsIInputStream *,UINT,UINT *) [e:\tip\mozilla\modules\libpr0n\decoders\gif\nsgifdecoder2.cpp:261]
            imgRequest::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,UINT,UINT) [e:\tip\mozilla\modules\libpr0n\src\imgrequest.cpp:872]
            ProxyListener::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,UINT,UINT) [e:\tip\mozilla\modules\libpr0n\src\imgloader.cpp:877]
            nsHttpChannel::OnDataAvailable(nsIRequest *,nsISupports *,nsIInputStream *,UINT,UINT) [e:\tip\mozilla\netwerk\protocol\http\src\nshttpchannel.cpp:4497]
            nsInputStreamPump::OnStateTransfer(void) [e:\tip\mozilla\netwerk\base\src\nsinputstreampump.cpp:508]


No idea if this is a real problem or not, or if it's potentially exploitable etc. Marking security sensitive just in case.

Comment 1

[Jesse Ruderman]

This code in nsGIFDecoder2::OutputRow was added in bug 143046.

Comment 2

[Alfred Kayser]

The interesting aspect is that the problem seems to be in the last byte of the mImageData buffer:
 Writing 4 bytes to 0x0d692ffc (1 byte at 0x0d692fff illegal)
        Address 0x0d692ffc points into a committed VirtualAlloc'd block 
0x0f692ffc points to an even/dword align position, but 0x0f692fff (the 1 byte that Purify is complaining about) is the last byte of a dword. So, how come that this block doesn't end 'dword' aligned?
As this memory block is allocated through gfxImageFrame/nsIIMage/gfxImageSurface
(http://lxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxImageSurface.cpp#44), it is really strange how this block could end up not DWORD padded...

And for the 'security-sensitive' flag, there is no way that one could misuse this one byte issue.

Comment 3

[Mike Schroepfer]

Marking blocking just so we can investigate and confirm this isn't a serious issue...

Comment 4

[Martijn Wargers (dead)]

There are also crashes in imgContainer::DrawFrameTo.
Currently nr 15 on the list of topcrashers:
http://crash-stats.mozilla.com/topcrasher/bybranch/1.9
http://crash-stats.mozilla.com/report/list?range_unit=weeks&branch=1.9&range_value=2&signature=imgContainer%3A%3ADrawFrameTo%28gfxIImageFrame%2A%2C+gfxIImageFrame%2A%2C+nsRect%26%29

Is that related to this bug?

Comment 5

[Martijn Wargers (dead)]

Sorry, never mind, that's bug 403363.

Comment 6

[Alfred Kayser]

The report from Purify is really strange, and I don't know what is happening there.
In short Purify is reporting that the last byte is 'illegal', but the first 3 bytes at 0xd592ffc are legal. Considering that all allocations are DWORD aligned, this can never happen... 

Comment 7

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

Purify still keeps track of the exact allocation size -- so even though the app might not crash nor scribble over memory, this could be indicating that something is allocating a 1-byte too small buffer.

Comment 8

[Stuart Parmenter]

Not sure what is going on here.

Comment 9

[Alfred Kayser]

There is something 'odd' here. The image data allocated is always in units of PRUint32, allocations are always on 4-byte boundary. So, this is really strange...

Comment 10

[Alfred Kayser]

So, in short, as long there is no more information on this, this will be very difficult to analyse/solve.

Comment 11

[Brandon Sterne (:bsterne)]

This bug doesn't appear to be going anywhere.  Does it need to remain hidden or can we remove that flag per comment 2.

Comment 12

[Lucas Adamski [:ladamski]]

I suppose it could be a Purify bug/false positive.  Interesting memory address.  Looks like its writing to the very end of a page boundary so its also possible we're hitting some sort of corner case with Purify array guards?  

Marking sg:investigate for now since I suspect this will take significant research to figure out one way or another.

Though I wouldn't assume that one byte overwrites are inherently immune from abuse.  There have been many off-by-one exploits.

Comment 13

[Reed Loden [:reed]]

Does this have any relation to bug 514776? If one runs purify with a current trunk build, is this same IPW still showing up?

Comment 14

[Daniel Veditz [:dveditz]]

We should test this under Valgrind and ASan to see if this is real or not. Does Purify even still run on our codebase?

Comment 15

[Daniel Veditz [:dveditz]]

Josh retested GIFs with Valgrind and decoder with ASan and neither as seeing this problem.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

It may be WORKSFORME now, but that doesn't mean it isn't still there.

I should note that there was no indication that this happened on *all* GIFs, but that it happened on one particular GIF - which is not an unusual form for such errors in image decoders; they're often data-dependent.

Also note that http://www.mozilla.org/projects/minefield/ has certainly changed in the last 5 years, so going to that URL isn't a good test (if you can get the state of that URL on the day/month or even year it was reported, that'd be a better test.

There's no indication in any of the comments if anyone was able to reproduce it or not; perhaps no one tried since Purify isn't something many people ran (I assume).

Comment 17

[Christian Holler (:decoder)]

(In reply to Randell Jesup [:jesup] from comment #16)
> It may be WORKSFORME now, but that doesn't mean it isn't still there.
> 
> I should note that there was no indication that this happened on *all* GIFs,
> but that it happened on one particular GIF - which is not an unusual form
> for such errors in image decoders; they're often data-dependent.

We're well aware of that, that's why I ran the whole GIF collection we have in our fuzzing repository.

> Also note that http://www.mozilla.org/projects/minefield/ has certainly
> changed in the last 5 years, so going to that URL isn't a good test (if you
> can get the state of that URL on the day/month or even year it was reported,
> that'd be a better test.

For the sake of completeness, I also ran the URL as it was at the time of the bug report through ASan, no errors (used Wayback Machine).

I think it's likely that this is a false positive or bug in Purity.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Christian: excellent!  It wasn't clear from the closing of this bug that we (you) were that thorough.  Thanks.