fix(psd): harden reader against oversized/bomb headers and ZIP-layer overflow - #5348
fix(psd): harden reader against oversized/bomb headers and ZIP-layer overflow#5348lgritz wants to merge 3 commits into
Conversation
…overflow Add open-time hardening to the PSD reader: - check_open() on the composite spec enforces the global limits:* (channels/resolution/imagesize) policy in addition to the existing per-dimension header validation. - check_compression_ratio() rejects a tiny file that declares a huge uncompressed composite before any pixel read (decompression bomb). - Size the ZIP / ZIP-prediction layer-channel decompression buffer with 64-bit arithmetic. width*height*(depth/8) was computed in uint32 and could wrap for large PSB layer dimensions, undersizing the buffer while read_channel_row() computes the row offset in 64-bit -> out-of-bounds read. Both now agree. Add a decompression-bomb regression fixture (testsuite/psd); it is picked up as a fuzz seed automatically via FORMAT_SOURCES. Valid files read byte-identically. Assisted-by: Claude Code / Claude Opus 4.8 Signed-off-by: Larry Gritz <lg@larrygritz.com>
| PSDInput::decompress_zip_prediction(span<char> src, span<char> dest, | ||
| const uint32_t width, const uint32_t height) | ||
| { | ||
| OIIO_ASSERT(width * height * (m_header.depth / 8) == dest.size()); |
There was a problem hiding this comment.
Wouldn't this assert fail for PSB since it still uses uint32_t to compute, but dest.size() is now in 64-bit? Should we cast these to size_t as well?
There was a problem hiding this comment.
I think you are correct. Will fix.
There was a problem hiding this comment.
Actually, it was easier (and maybe caught another couple problems) to change the width and height parameters themselves to uint64_t. Pushed an update.
…packbits Signed-off-by: Larry Gritz <lg@larrygritz.com>
| if (ok && !m_specs.empty()) { | ||
| if (!check_open(m_specs[0], { 0, 300000, 0, 300000, 0, 1, 0, 56 }) | ||
| || !check_compression_ratio(m_specs[0], ioproxy()->size())) { | ||
| close(); | ||
| return false; |
There was a problem hiding this comment.
Hmm, one question: do we want this guard here, or before load_layers()? My read is that we decompress the file in load_layer_channel(), which runs during load_layers(), and we want to avoid that. Correct me if I'm wrong!
There was a problem hiding this comment.
That's a good point. I'm not sure if we can do it before load_layers, but I'm really eyeing load_image_data as something we certainly want to do only after all the cheap early-outs. Let me take a look at all the various load_* functions and see how early I can push the check_open/check_compression_ratio, versus which of these read functions are needed for the information that's crucial to the check functionality.
There was a problem hiding this comment.
After some investigation, I think this can be arranged to happen right after load_header() and set_type_desc(), and before any of the other load_* functions get a chance to act on absurd header values. Update pushed, thanks for noticing this and making the suggestion!
Code review FTW!
There was a problem hiding this comment.
Glad it helped. I had to deal with a bunch of PSD/PSB decompression bombs in the past, so it caught my eye haha
… load_header and set_type_spec, but before the rest of the load_*() functions and their allocations. Signed-off-by: Larry Gritz <lg@larrygritz.com>
2 participants
Add open-time hardening to the PSD reader:
limits:*(channels/resolution/imagesize) policy in addition to the existing per-dimension header validation.width*height*(depth/8)was computed in uint32 and could wrap for large PSB layer dimensions, undersizing the buffer while read_channel_row() computes the row offset in 64-bit -> out-of-bounds read. Both now agree.Add a decompression-bomb regression fixture (testsuite/psd); it is picked up as a fuzz seed automatically via FORMAT_SOURCES. Valid files read byte-identically.
Assisted-by: Claude Code / Claude Opus 4.8