We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 5079

Summary: Out-of-bounds read results in information leak from the heap, in the PCX Image loader
Product: SDL_image Reporter: Shubham Tandale <s_tandale>
Component: miscAssignee: Sam Lantinga <slouken>
Status: NEW --- QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2    
Version: 2.0.5   
Hardware: All   
OS: All   
Attachments: Example pcx file
potential patch

Description Shubham Tandale 2020-04-07 00:52:56 UTC
Created attachment 4295 [details]
Example pcx file

By controlling certain values in a PCX file's header, its possible to leak memory from the heap into SDL_Surface's pixels buffer. 

The bug is in the IMG_pcx.c file. "buf" is allocated with size "bpl" which is controllable from values inside the PCX header. At Ln 227, bytes are copied from the "buf" buffer into a surface's pixels buffer with a for loop depending on the WIDTH of the image which is also controllable from values in the PCX header. By setting "width" large and controlling "bpl" to be small, it is possible to leak memory past the buffer, into the surface's pixels buffer.
Comment 1 Shubham Tandale 2020-04-07 12:22:55 UTC
To add specifics with code in IMG_pcx.c

The width for the surface is calculated via:
135: width = (pcxh.Xmax - pcxh.Xmin) + 1;

Here the buf is allocated with bpl bytes
163: bpl = pcxh.NPlanes * pcxh.BytesPerLine;
164: buf = (Uint8 *)SDL_calloc(bpl, 1);

Later data is copied from buf into the pixels buffer.

Uint8 *innerSrc = buf;
int plane;
for ( plane = 0; plane < pcxh.NPlanes; plane++ ) {
        int x;
        Uint8 *dst = row + plane;
        for ( x = 0; x < width; x++ ) {
                if ( dst >= row+surface->pitch ) {
                        error = "decoding out of bounds (corrupt?)";
                        goto done;
                }
                *dst = *innerSrc++;
                dst += pcxh.NPlanes;
        }
}

There is no check to see whether "innerSrc" is past the bounds of the buffer.
Comment 2 Shubham Tandale 2020-04-07 12:32:17 UTC
Created attachment 4297 [details]
potential patch