From 1e4e15301a0ee677b5663ce61a33426f63c32d67 Mon Sep 17 00:00:00 2001 From: cinemaONE <35371193+cinema-ONE@users.noreply.github.com> Date: Sun, 30 Aug 2026 08:32:48 +0200 Subject: [PATCH 1/3] Do not let a malformed file terminate Kodi 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 --- src/MPOPicture.cpp | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/MPOPicture.cpp b/src/MPOPicture.cpp index 21dd72b..cdc4280 100644 --- a/src/MPOPicture.cpp +++ b/src/MPOPicture.cpp @@ -10,9 +10,30 @@ #include "../lib/TinyEXIF/TinyEXIF.h" +#include #include #include +namespace +{ + +// libjpeg's default error_exit calls exit(), and libmpo installs it via +// jpeg_std_error() without overriding it, so a malformed file terminates Kodi +// rather than failing the decode. mpo_decompress_error_exit() can only replace +// the function pointer, not attach state to it, so the jump target is +// thread-local; each decoder instance is driven from one thread at a time. +thread_local std::jmp_buf s_jpegEscape; + +void MpoFatalError(j_common_ptr cinfo) +{ + char message[JMSG_LENGTH_MAX] = {}; + (*cinfo->err->format_message)(cinfo, message); + kodi::Log(ADDON_LOG_ERROR, "libjpeg: %s", message); + std::longjmp(s_jpegEscape, 1); +} + +} // namespace + MPOPicture::MPOPicture(const kodi::addon::IInstanceInfo& instance) : CInstanceImageDecoder(instance) { @@ -36,8 +57,15 @@ bool MPOPicture::SupportsFile(const std::string& file) mpo_decompress_struct mpoinfo; mpo_create_decompress(&mpoinfo); + mpo_decompress_error_exit(&mpoinfo, MpoFatalError); + if (setjmp(s_jpegEscape)) + { + mpo_destroy_decompress(&mpoinfo); + return false; + } + mpo_mem_src(&mpoinfo, buffer.data(), buffer.size()); - bool ret = mpo_read_header(&mpoinfo); + const bool ret = mpo_read_header(&mpoinfo); mpo_destroy_decompress(&mpoinfo); return ret; } @@ -139,6 +167,13 @@ bool MPOPicture::LoadImageFromMemory(const std::string& mimetype, m_data.resize(bufSize); std::copy(buffer, buffer + bufSize, m_data.begin()); mpo_create_decompress(&m_mpoinfo); + mpo_decompress_error_exit(&m_mpoinfo, MpoFatalError); + if (setjmp(s_jpegEscape)) + { + mpo_destroy_decompress(&m_mpoinfo); + return false; + } + mpo_mem_src(&m_mpoinfo, m_data.data(), m_data.size()); if (!mpo_read_header(&m_mpoinfo)) { @@ -169,6 +204,9 @@ bool MPOPicture::Decode(uint8_t* pixels, return false; } + if (setjmp(s_jpegEscape)) + return false; + size_t image = 0; while (image < m_images) { From ab7df8e2993b6b086d3a9c948ef52ecd78a40780 Mon Sep 17 00:00:00 2001 From: cinemaONE <35371193+cinema-ONE@users.noreply.github.com> Date: Sun, 30 Aug 2026 08:48:13 +0200 Subject: [PATCH 2/3] Bounds-check the MP parser's cursor 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 --- lib/kodi-libmpo-note.txt | 7 ++++++- lib/libmpo/src/mpo.c | 18 +++++++++++++----- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/kodi-libmpo-note.txt b/lib/kodi-libmpo-note.txt index 2d36e77..80c7d1f 100644 --- a/lib/kodi-libmpo-note.txt +++ b/lib/kodi-libmpo-note.txt @@ -1,2 +1,7 @@ libmpo source from https://github.com/Lectem/libmpo -Sync to 63ada10 (14 Aug 2017) \ No newline at end of file +Sync to 63ada10 (14 Aug 2017) + +Locally modified, not present upstream (upstream's last code commit is 2017): + src/mpo.c - mpf_getbyte(), mpf_seek() and mpf_dc_rewindc() bounds-check the + cursor. The only check was an assert() in mpf_getbyte(), which NDEBUG removes + from release builds, leaving every read in the MP parser unchecked. diff --git a/lib/libmpo/src/mpo.c b/lib/libmpo/src/mpo.c index d007a2d..9382e0d 100644 --- a/lib/libmpo/src/mpo.c +++ b/lib/libmpo/src/mpo.c @@ -20,21 +20,29 @@ long mpf_tell(MPFbuffer_ptr b) void mpf_seek(MPFbuffer_ptr b,long offset, int from) { - if(from==SEEK_CUR)b->_cur+=offset; - else if(from==SEEK_SET)b->_cur=offset; - else if(from==SEEK_END)b->_cur=b->_size-1+offset; + /* Kodi downstream: offsets come from the file, so clamp rather than trust. */ + long target; + if(from==SEEK_CUR)target=b->_cur+offset; + else if(from==SEEK_SET)target=offset; + else if(from==SEEK_END)target=b->_size-1+offset; + else return; + if(target<0)target=0; + if(target>b->_size)target=b->_size; + b->_cur=target; } unsigned int mpf_getbyte (MPFbuffer_ptr b) /* Read next byte */ { - assert(b->_cur < b->_size); + /* Kodi downstream: this assert was the only bounds check on every read in + the MP parser, and NDEBUG removes it from the builds we ship. */ + if(b->_cur < 0 || b->_cur >= b->_size)return 0; return b->buffer[b->_cur++]; } void mpf_dc_rewindc(MPFbuffer_ptr b) { - b->_cur--; + if(b->_cur>0)b->_cur--; } uint16_t mpf_getint16 (MPFbuffer_ptr b, int swapEndian) From dc35e4e6abc48418648afff038f6f320a571211e Mon Sep 17 00:00:00 2001 From: cinemaONE <35371193+cinema-ONE@users.noreply.github.com> Date: Sun, 30 Aug 2026 09:24:51 +0200 Subject: [PATCH 3/3] Clear the MP entries realloc() adds before they are freed 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 --- lib/libmpo/include/libmpo/mpo.h | 3 +++ lib/libmpo/src/dmpo.c | 30 ++++++++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/libmpo/include/libmpo/mpo.h b/lib/libmpo/include/libmpo/mpo.h index 337bf81..519df9f 100644 --- a/lib/libmpo/include/libmpo/mpo.h +++ b/lib/libmpo/include/libmpo/mpo.h @@ -46,6 +46,9 @@ typedef struct {MPFLong numerator; MPFLong denominator;} MPFRational; typedef unsigned char MPFUndefined; /* 1 byte */ typedef struct {MPFSLong numerator; MPFSLong denominator;} MPFSRational; +/* Kodi downstream: a sanity ceiling on the file-supplied image count. */ +#define MPO_MAX_IMAGES 64 + typedef struct { MPFByte * buffer; diff --git a/lib/libmpo/src/dmpo.c b/lib/libmpo/src/dmpo.c index 4ef7f60..5ab407c 100644 --- a/lib/libmpo/src/dmpo.c +++ b/lib/libmpo/src/dmpo.c @@ -122,8 +122,34 @@ bool mpo_read_header(mpo_decompress_struct *mpoinfo) { res = jpeg_read_header((j_decompress_ptr) &mpoinfo->cinfo, TRUE) != 0; int nbImages = mpoinfo->APP02->numberOfImages; - if(nbImages > 1) - mpoinfo->APP02 = realloc(mpoinfo->APP02,nbImages * (sizeof *mpoinfo->APP02)); + /* Kodi downstream: numberOfImages comes from the file, and + mpo_destroy_decompress() walks every entry calling free() on its + MPentry. realloc() does not zero what it adds, and only entry 0 has + been parsed at this point, so the rest must be cleared or the + teardown frees uninitialised pointers. */ + 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 + { + /* Only one entry is allocated, and mpo_destroy_decompress() + walks numberOfImages of them, so it has to agree. */ + mpoinfo->APP02->numberOfImages = 1; + res = 0; + } + } + else if(nbImages > MPO_MAX_IMAGES) + { + mpoinfo->APP02->numberOfImages = 1; + res = 0; + } + else if(nbImages < 1) + mpoinfo->APP02->numberOfImages = 1; } return res;