fix: prevent integer overflow and OOB write in PSDDecoder#9881
fix: prevent integer overflow and OOB write in PSDDecoder#9881YLChen-007 wants to merge 2 commits intogoogle:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
libs/image/src/LinearImage.cpp
Outdated
| const uint32_t nfloats = width * height * channels; | ||
| const uint64_t nfloats64 = (uint64_t)width * (uint64_t)height * (uint64_t)channels; | ||
| if (nfloats64 > UINT32_MAX || nfloats64 > (std::numeric_limits<size_t>::max() / sizeof(float))) { | ||
| throw std::bad_alloc(); |
There was a problem hiding this comment.
Use FILAMENT_CHECK_PRECONDITION instead, it will use exceptions when they are enabled (and log the error).
There was a problem hiding this comment.
Thank your response, I have submitted a new commit. Is there any problem in this new commit?
An integer overflow vulnerability exists in the calculation of heap allocation size when parsing maliciously crafted PSD files with unrealistically large dimensions. This leads to an undersized allocation followed by an out-of-bounds heap write during pixel data decoding. This patch applies two mitigations: 1. Uses 64-bit arithmetic in LinearImage::SharedReference to safely calculate total bytes and throws std::bad_alloc on overflow. 2. Adds explicit bounds validation in PSDDecoder::decode for the image dimensions, bounding it to the absolute maximum PSB size (300,000).
YLChen-007
force-pushed
the
fix/psddecoder-integer-overflow
branch
from
b9c12b7 to
017ac47
Compare
| SharedReference(uint32_t width, uint32_t height, uint32_t channels) { | ||
| const uint32_t nfloats = width * height * channels; | ||
| const uint64_t nfloats64 = (uint64_t)width * (uint64_t)height * (uint64_t)channels; | ||
| FILAMENT_CHECK_PRECONDITION(nfloats64 <= UINT32_MAX && nfloats64 <= (std::numeric_limits<size_t>::max() / sizeof(float))) |
There was a problem hiding this comment.
This check looks valid. But could you add comments what this checks mean?
libs/imageio/src/ImageDecoder.cpp
Outdated
| if (width == 0 || height == 0) { | ||
| throw std::runtime_error("invalid PSD dimensions: width and height must be non-zero"); | ||
| } | ||
| if (width > 300000 || height > 300000) { |
There was a problem hiding this comment.
Are these correct numbers for PSD? could you add comments regarding this?
There was a problem hiding this comment.
These are the correct numbers. For images larger than 30,000 pixels you need a different format called PSB (Photoshop Big). But yeah, it could use a short comment or use of a constant.
There was a problem hiding this comment.
It looks like ImageDecoder.cpp doesn't support PSB file. The signature check and the Head struct is hard-coded for PSD. So this needs to be 30,000 with proper comments.
There was a problem hiding this comment.
Thank your response, I have submitted a new commit. Is there any problem in this new commit?
There was a problem hiding this comment.
Could you check my other comment?
- Specify that PSD image sizes are limited to 30,000 pixels instead of 300,000 as per the PSD specification. - Add comments explaining PSB unsupported fallback and explicit limitation preventing Integer Overflows during scaling allocations.
4 participants
Summary
Fix an integer overflow leading to an out-of-bounds heap write in PSDDecoder (resolves #9880).
Problem
The
PSDDecoder::decode()function parseswidthandheightdimensions from a user-supplied.psdfile header without bounds validation. These values are directly passed intoLinearImage. WithinLinearImage::SharedReference, the footprint is determined using 32-bit math (width * height * channels).By crafting a PSD file with specifically tailored large dimensions (e.g.$2^{32}$ ) into a dangerously undersized remainder. The resulting heap allocation completes successfully but allocates a tiny buffer that cannot house the data.
Width = 35031, Height = 40866), the multiplication wraps past theuint32_tmaximum (Subsequent loop logic blindly trusts the massive width/height counters and uses stream input content to rewrite linear memory adjacent to the heap chunk. The execution spills over, writing attacker-controlled values resulting in Denial of Service (DoS) and yielding a direct primitive for Remote Code Execution (RCE) by corrupting pointers (e.g., vtables of adjacent objects).
Fix Details
The patch introduces critical bounding checks and mitigations preventing the malicious arithmetic wrap around:
LinearImage::SharedReferenceconstructor to utilizeuint64_tsize accounting limit to safely pre-check bounds prior to casting down. If calculation surpasses limits, the structure defensively triggers astd::bad_alloc.PSDDecoder. Values equal to0or exceeding architectural reasonability (300,000equivalent to max PSB capacity) explicitly fire a softstd::runtime_error("PSD dimensions exceed maximum allowed size").Testing
Verified using an isolated harness incorporating exact parsing conditions alongside memory allocation alignment:
exploit.psd)std::runtime_errordimension limitrce.psd)std::runtime_errorexitcontrol_valid.psd)Changes
libs/imageio/src/ImageDecoder.cpp(+7 lines, -0 lines)libs/image/src/LinearImage.cpp(+6 lines, -2 lines)