Bounds-check the MP parser and fix an invalid free on valid MPO files - #74
Conversation
|
| Filename | Overview |
|---|---|
| lib/libmpo/src/dmpo.c | Safely initializes successful APP02 growth, but rejection and allocation-failure paths preserve an inconsistent teardown count that permits out-of-bounds frees. |
| lib/libmpo/src/mpo.c | Replaces assertion-only cursor protection with release-build bounds handling; inspected parser loops remain counter-bounded. |
| src/MPOPicture.cpp | Adds libjpeg fatal-error recovery and contains the callers that expose the inconsistent APP02 state during failure cleanup. |
| lib/libmpo/include/libmpo/mpo.h | Introduces the downstream maximum image-count constant used by header parsing. |
| lib/kodi-libmpo-note.txt | Documents the downstream parser bounds-checking changes. |
Reviews (1): Last reviewed commit: "Clear the MP entries realloc() adds befo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if(nbImages > 1 && nbImages <= MPO_MAX_IMAGES) | ||
| { | ||
| MPExt_Data *grown = realloc(mpoinfo->APP02,nbImages * (sizeof *mpoinfo->APP02)); | ||
| if(grown) | ||
| { | ||
| memset(grown + 1, 0, (nbImages - 1) * (sizeof *grown)); | ||
| mpoinfo->APP02 = grown; | ||
| } | ||
| else res = 0; | ||
| } | ||
| else if(nbImages > MPO_MAX_IMAGES) res = 0; |
There was a problem hiding this comment.
When an MPO declares more than MPO_MAX_IMAGES, or APP02 growth fails, this path returns failure while retaining a one-entry allocation and the larger file-derived numberOfImages. The callers then destroy the decompressor, whose teardown iterates that count and reads beyond the allocation, potentially passing invalid MPentry values to free() and causing heap corruption. How this was verified: Teardown uses APP02->numberOfImages as its loop bound after both rejection paths leave that value unchanged.
| if(nbImages > 1 && nbImages <= MPO_MAX_IMAGES) | |
| { | |
| MPExt_Data *grown = realloc(mpoinfo->APP02,nbImages * (sizeof *mpoinfo->APP02)); | |
| if(grown) | |
| { | |
| memset(grown + 1, 0, (nbImages - 1) * (sizeof *grown)); | |
| mpoinfo->APP02 = grown; | |
| } | |
| else res = 0; | |
| } | |
| else if(nbImages > MPO_MAX_IMAGES) res = 0; | |
| if(nbImages > 1 && nbImages <= MPO_MAX_IMAGES) | |
| { | |
| MPExt_Data *grown = realloc(mpoinfo->APP02,nbImages * (sizeof *mpoinfo->APP02)); | |
| if(grown) | |
| { | |
| memset(grown + 1, 0, (nbImages - 1) * (sizeof *grown)); | |
| mpoinfo->APP02 = grown; | |
| } | |
| else | |
| { | |
| mpoinfo->APP02->numberOfImages = 1; | |
| res = 0; | |
| } | |
| } | |
| else if(nbImages > MPO_MAX_IMAGES) | |
| { | |
| mpoinfo->APP02->numberOfImages = 1; | |
| res = 0; | |
| } |
There was a problem hiding this comment.
Right, and it was mine — thanks.
The rejection paths left numberOfImages at the file's value while only one entry stayed allocated, and mpo_destroy_decompress() walks that count:
nbImages = mpoinfo->APP02->numberOfImages;
for(i=0;i<nbImages;++i) destroyMPF_Data(&mpoinfo->APP02[i]);So refusing to grow the array made teardown worse, not safer. Fixed in 7d971ea (was 13db5d6): every path that does not grow the array now brings numberOfImages back to 1, so the count and the allocation always agree. nbImages < 1 is clamped up to 1 as well, otherwise entry 0's MPentry is never freed.
Verified with an MPO edited to declare 5000 images:
| pristine | with this PR | |
|---|---|---|
| 5000-image declaration | heap-buffer-overflow | rejected, no crash |
| valid 2-image MPO (x2, confirmed by Pillow) | heap-buffer-overflow | no crash |
Worth noting the general shape, since I got it wrong twice in this PR series: a guard that refuses an operation has to leave the structure consistent for whatever runs next. Rejecting the growth while keeping the count was the same class of mistake as re-deriving a bounds expression instead of mirroring it.
13db5d6 to
7d971ea
Compare
libmpo installs libjpeg's error handler with jpeg_std_error() and never overrides error_exit, whose default implementation calls exit(). A malformed MPO therefore takes the whole application down rather than failing the decode, and the add-on never called mpo_decompress_error_exit() to replace it. Found by fuzzing libmpo as it actually ships. With NDEBUG - which Release builds define - a crafted file reaches exit() from mpo_read_header(). Without NDEBUG the same input trips one of libmpo's asserts instead; those asserts are the only validation of the attacker-controlled offsets in that parser, and they are compiled out of the builds we ship. libmpo's API can only replace the handler's function pointer, not attach state to it, so the jump target is thread-local. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mpf_getbyte() is the primitive every 16- and 32-bit reader in the MP extension parser is built on, and its only bounds check was an assert(), which NDEBUG removes from the builds we ship. In a release build the whole parser therefore read from a file-controlled offset with nothing checking it. Fuzzing found a heap-buffer-overflow read through that path within minutes, via mpf_getint16() -> MPExtReadTag() -> MPExtReadMPF() -> mpo_read_header(). It only became reachable once the previous commit stopped libjpeg calling exit() first. mpf_seek() took a file-supplied offset without clamping it, and mpf_dc_rewindc() could take the cursor negative; both are fixed here since mpf_getbyte() alone cannot recover from a cursor already out of range. This is a downstream change: upstream's last code commit is from 2017, so there is nothing to sync to. Recorded in lib/kodi-libmpo-note.txt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mpo_read_header() grows APP02 from one entry to numberOfImages once the count is known, but realloc() does not zero what it adds and only entry 0 has been parsed at that point. mpo_destroy_decompress() then walks every entry and calls free() on its MPentry pointer, so entries 1..n-1 are freed from uninitialised heap. This is reachable on a well-formed two-image MPO, which is every MPO: the count comes from the MP Index IFD of the first image, and the later entries are not populated until each image is decompressed - if it ever is. SupportsFile() creates, reads the header and destroys without decompressing at all. numberOfImages is file-supplied, so the allocation is capped. Whenever the array is not grown - over the cap, or realloc failing - numberOfImages is brought back to 1 to match what is actually allocated, since teardown walks that count and would otherwise read past the end and free from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7d971ea to
dc35e4e
Compare
|
Thanks! |
2 participants
Follows #73, which stops libjpeg's default
error_exitterminating Kodi. That fix is what made these reachable — theexit()was firing first.Three memory-safety problems in the vendored MP parser, all reachable from a well-formed MPO.
1. Every read was unchecked in release builds
mpf_getbyte()is the primitive every 16- and 32-bit reader is built on, and its only bounds check was anassert()— removed byNDEBUG:It is the single choke point: the only other
buffer[...]accesses in libmpo are two constant writes in the data source.mpf_seek()also took a file-supplied offset without clamping, andmpf_dc_rewindc()could take the cursor negative, so both are fixed here — bounds-checking the read alone cannot recover from a cursor already out of range.2.
realloc()leaves entries that are later freedmpo_read_header()growsAPP02from one entry tonumberOfImagesonce the count is known.realloc()does not zero what it adds, only entry 0 has been parsed, andmpo_destroy_decompress()then callsfree()on every entry'sMPentry. Entries 1..n-1 are freed from uninitialised heap.SupportsFile()reaches this with no decompression at all — create, read header, destroy.numberOfImagesis file-supplied, so the allocation is capped too.Evidence
Tested against two MPO files, both confirmed valid by Pillow (
format=MPO frames=2) — one built from the CIPA DC-007 spec independently of libmpo, one written by libmpo's own compressor:spec.mpo(640x480, 2 frames)valid.mpo(64x48, 2 frames)-DNDEBUGWhat this does not fix
mpo_read_header()still returns false for both valid files. This PR makes the failure safe, not correct — the add-on appears unable to decode MPOs that other readers handle. That looks like a separate defect in the marker/offset handling and I have not attempted it here.I would also flag the obvious: upstream libmpo's last code commit is
63ada10from August 2017, and the only commit since is a README edit. These are downstream patches because there is nowhere to send them; they are recorded inlib/kodi-libmpo-note.txt.Written by my AI co-author (Claude Code); posted from my account.