From 051e9dacbe230a8e8cb8307cf47fe3f0ea159239 Mon Sep 17 00:00:00 2001 From: Michael Bommarito Date: Fri, 29 May 2026 09:36:10 -0400 Subject: [PATCH] Fix heap-buffer-overflow write in PICT UnpackBits The PackBits decompression loop in UnpackBits advances dst by the decoded run length without verifying that the destination remains within the allocated bitmap raster. A crafted PICT file with run lengths that exceed the image causes expandBuf/expandBuf8 and the repeat memcpy to write past the end of the bitmap. This is the sibling of the UnpackPictRow fix (PR #41). UnpackBits is the decoder used for 1/2/4/16-bpp PICT data (the 8/32-bpp paths go through the now-bounded UnpackPictRow), and it has the same unbounded run problem; it is reachable from both the version-1 bitmap opcode and the version-2 DirectBitsRect/PackBitsRect opcodes. Clamp each write to the allocated raster: compute the raster end once (FreeImage_GetBits(dib) + pitch * height) and break out of the row loop if a run would write past it, in both the packed-data and unpacked-data code paths. CWE-787 (Out-of-bounds Write) Assisted-by: Claude:claude-opus-4-8 --- Source/FreeImage/PluginPICT.cpp | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/Source/FreeImage/PluginPICT.cpp b/Source/FreeImage/PluginPICT.cpp index e359ef6..ec8d734 100644 --- a/Source/FreeImage/PluginPICT.cpp +++ b/Source/FreeImage/PluginPICT.cpp @@ -756,7 +756,13 @@ UnpackBits( FreeImageIO *io, fi_handle handle, FIBITMAP* dib, MacRect* bounds, W } } else { - for ( int i = 0; i < height; i++ ) { + // CWE-787: bound every PackBits write to the allocated raster. + // The RLE run length is attacker-controlled and the loop below + // advances dst without checking it against the bitmap bounds. + BYTE* const rasterStart = FreeImage_GetBits( dib ); + BYTE* const rasterEnd = rasterStart + + (size_t)FreeImage_GetPitch( dib ) * FreeImage_GetHeight( dib ); + for ( int i = 0; i < height; i++ ) { // For each line do... int linelen; // length of source line in bytes. if (rowBytes > 250) { @@ -784,16 +790,22 @@ UnpackBits( FreeImageIO *io, fi_handle handle, FIBITMAP* dib, MacRect* bounds, W // This is slow for some formats... if (pixelSize == 16) { + if (dst < rasterStart || + (size_t)(rasterEnd - dst) < (size_t)len*4*PixelPerRLEUnit) + break; expandBuf( io, handle, 1, pixelSize, dst ); - for ( int k = 1; k < len; k++ ) { + for ( int k = 1; k < len; k++ ) { // Repeat the pixel len times. memcpy( dst+(k*4*PixelPerRLEUnit), dst, 4*PixelPerRLEUnit); } dst += len*4*PixelPerRLEUnit; } else { + if (dst < rasterStart || + (size_t)(rasterEnd - dst) < (size_t)len*PixelPerRLEUnit) + break; expandBuf8( io, handle, 1, pixelSize, dst ); - for ( int k = 1; k < len; k++ ) { + for ( int k = 1; k < len; k++ ) { // Repeat the expanded byte len times. memcpy( dst+(k*PixelPerRLEUnit), dst, PixelPerRLEUnit); } @@ -806,10 +818,16 @@ UnpackBits( FreeImageIO *io, fi_handle handle, FIBITMAP* dib, MacRect* bounds, W // Unpacked data int len = (FlagCounter & 255) + 1; if (pixelSize == 16) { + if (dst < rasterStart || + (size_t)(rasterEnd - dst) < (size_t)len*4*PixelPerRLEUnit) + break; expandBuf( io, handle, len, pixelSize, dst ); dst += len*4*PixelPerRLEUnit; } else { + if (dst < rasterStart || + (size_t)(rasterEnd - dst) < (size_t)len*PixelPerRLEUnit) + break; expandBuf8( io, handle, len, pixelSize, dst ); dst += len*PixelPerRLEUnit; }