fix(exif): harden the shared Exif decoder against malformed input - #5399
Open
lgritz wants to merge 1 commit into
Open
fix(exif): harden the shared Exif decoder against malformed input#5399lgritz wants to merge 1 commit into
lgritz wants to merge 1 commit into
Conversation
Audit of the shared Exif decoder, which every container that embeds Exif reaches with file-controlled bytes: jpeg APP1, png eXIf and the "Raw profile type exif" text chunk, heif, psd, webp, and the raw plugin's LibRaw callback. The short-blob TIFFHeader overread from the external advisory draft was already closed; the following were not, two of them crashes on an ordinary Release build with no sanitizer. Recursive IFD directory-count overread: the recursive ExifIFD/GPSIFD and Interoperability-IFD branches in read_exif_tag only checked that the target offset was < buffer size before reading a 2-byte directory count there, so an offset of buffer_size - 1 passed but the 2-byte read then ran one byte past the end of the blob (heap out-of-bounds read). Require room for the full 2-byte count before reading, matching decode_ifd, and read the count via memcpy to avoid an unaligned load (UBSan). Unknown-type size sentinel: tiff_data_size() reports an unrecognized TIFF data type as size_t(-1). dataspan(), dataptr() and tiff_dir_data() multiplied that sentinel by the entry's count and fed the result to their bounds checks, where size_t(offset) + size_t(-1) wraps to offset - 1 and passes any "> buffer size" test, yielding a span of length size_t(-1) into the blob. The trigger is not exotic: EXIF_UTF8_TYPE = 129 is the Exif 3.0 UTF-8 string type added in AcademySoftwareFoundation#4961, which add_exif_item_to_spec explicitly accepts but tiff_data_size was never taught about, so every Exif 3.0 UTF-8 tag took this path. A 26-byte blob -- one tag, type 129, count 1, offset 1 -- is a heap out-of-bounds read under ASan and an uncaught std::length_error that aborts the process in Release. tiff_data_size now knows the type (which also makes Exif 3.0 UTF-8 tags decode correctly rather than being dropped or crashing), and a new pvt::dirdata_size() rejects the sentinel so no other unknown type can wrap the arithmetic. The offset math in those helpers is promoted to 64-bit while here. IFD depth: ifd_offsets_seen stops an IFD chain from looping, but nothing stopped it from being deep -- a nested IFD only has to sit at an offset not yet seen, and 18 bytes buys one level, so a blob can chain as many levels as its own length allows and a 180 KB blob exhausts the stack. A jpeg APP1 marker caps a blob at 64 KB, but a png eXIf chunk's length field is 31 bits and heif and psd are unbounded. Now capped at 32 levels, with the depth passed down the call stack as an ordinary parameter of read_exif_tag and decode_ifd. The one wrinkle is the MakerNote, the only tag whose decoding re-enters the walk: TagInfo::HandlerFunc is public and fixed, so it cannot carry the depth. Its body moves to decode_makernote(), which takes the depth, and read_exif_tag calls that directly instead of going through the handler pointer; makernote_handler stays in the tag table as a thin wrapper so the table and tag_table() are unchanged. alloca'd rational arrays: the RATIONAL and SRATIONAL branches of add_exif_item_to_spec sized an OIIO_ALLOCA from the file's element count. OIIO_ALLOCA's own bound is a plain assert, compiled out under NDEBUG, so a Release build has no check: a 32 MB blob holding one RATIONAL tag of count 4,000,000 alloca's 16 MB and segfaults. Now OIIO_ALLOCATE_STACK_OR_HEAP, which is what the rest of the codebase uses for exactly this. Also: the directory-entry bounds test used >= where it wanted >, quietly discarding a legitimate final entry that ended exactly at the blob end, and the TIFFHeader was read through a cast that assumed alignment. Adds jpeg-corrupt and png-damaged fixtures across two container routes, built by committed generators that embed their own 1x1 carrier so they depend on no writer and carry no version-dependent metadata. Assisted-by: Claude Code / Claude Opus 4.8 Signed-off-by: Larry Gritz <lg@larrygritz.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit of the shared Exif decoder, used by JPEG, PNG, Raw, HEIF, PSD, and WebP. Turned up several issues that could be trouble for corrupt, malicious, or unusual Exif blocks:
Recursive IFD directory-count overread: the recursive ExifIFD/GPSIFD and Interoperability-IFD branches in read_exif_tag only checked that the target offset was < buffer size before reading a 2-byte directory count there, so an offset of
buffer_size - 1would run one byte past the end of the blob.Unknown-type size sentinel: tiff_data_size() reports an unrecognized TIFF data type as size_t(-1). Then dataspan(), dataptr() and tiff_dir_data() multiplied that by the entry's count and fed the result to their bounds checks. Now tiff_data_size knows the type (which also makes Exif 3.0 UTF-8 tags decode correctly rather than being dropped or crashing), and a new pvt::dirdata_size() rejects the sentinel so that unknown types don't wrap the arithmetic. Also switch to 64 bit math for computing those offsets.
IFD depth: ifd_offsets_seen stops an IFD chain from looping, but nothing stopped it from being arbitrarily deep and overflowing the stack. Cap the depth at at 32 levels, more than enough for any real file.
Change some OIIO_ALLOCA to OIIO_ALLOCATE_STACK_OR_HEAP, to guarantee not allocating things too big to fit on the stack.
The directory-entry bounds test used >= where it wanted >, quietly discarding a legitimate final entry that ended exactly at the blob end, and the TIFFHeader was read through a cast that assumed alignment.
Add jpeg-corrupt and png-damaged tests (and scripts that generate those odd test cases).
Assisted-by: Claude Code / Claude Opus 4.8