From fcb3cb5679539a3ca3def34bc367116d08c09a62 Mon Sep 17 00:00:00 2001 From: MarkLee131 Date: Fri, 31 Jul 2026 17:42:53 +0800 Subject: [PATCH] Validate string lengths and item counts in cached help books CacheReadString() took the length of a string straight from the .cached file and passed len - 1 to wxCharBuffer. A stored length of 0 underflowed to SIZE_MAX, so wxCharTypeBuffer allocated (SIZE_MAX + 1) bytes, i.e. none, and then wrote its terminator at str[SIZE_MAX], which wraps on 64-bit to a write one byte in front of the block. The buffer was also a byte shorter than the read that filled it, so the terminator was overwritten and the wxString constructor went looking for one past the end, and a large or negative length asked for a huge allocation or a read through a null pointer. Size the buffer as len so the terminator survives the read, as ReadString() in zipstrm.cpp already does, reject a length that cannot have come from CacheWriteString() or that exceeds the file, check the read actually delivered the bytes, and build the string from the known length rather than by scanning for a NUL. The contents and index counts are used to reserve memory before anything is read, so bound them against the file as well, and move the contents loop to std::make_unique so the new early returns cannot leak, as the index loop already does. See #26765, #26766. (cherry picked from commit 09cebab5cd6ddb5fef8a87a11753158864ff0044) --- docs/changes.txt | 1 + src/html/helpdata.cpp | 81 ++++++++++++++++++++++++++++++++++++------- 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/docs/changes.txt b/docs/changes.txt index 3689593988..b5ad08d5df 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -265,6 +265,7 @@ All (GUI): - Always recognize common file types in wxHTML (Richard Thomson, #20967). - Respect background colour when printing in wxHTML (Richard Thomson, #20652). - Fix handling TGA files with invalid size (MarkLee131, #26760). +- Fix buffer overflow with invalid cached help books (MarkLee131, #26765). wxMSW: diff --git a/src/html/helpdata.cpp b/src/html/helpdata.cpp index 74e075115d..f359fb7ad0 100644 --- a/src/html/helpdata.cpp +++ b/src/html/helpdata.cpp @@ -319,11 +319,28 @@ inline static void CacheWriteInt32(wxOutputStream *f, wxInt32 value) inline static wxInt32 CacheReadInt32(wxInputStream *f) { - wxInt32 x; + wxInt32 x = 0; f->Read(&x, sizeof(x)); return wxINT32_SWAP_ON_BE(x); } +// Check a count read from the file before it is used to reserve memory: every +// item takes at least one byte, so a count larger than the file itself can +// only come from a corrupted or hostile .cached file. +inline static bool CacheReadCount(wxInputStream *f, int& count) +{ + const wxInt32 stored = CacheReadInt32(f); + if ( stored < 0 ) + return false; + + const wxFileOffset available = f->GetLength(); + if ( available != wxInvalidOffset && (wxFileOffset)stored > available ) + return false; + + count = stored; + return true; +} + inline static void CacheWriteString(wxOutputStream *f, const wxString& str) { const wxWX2MBbuf mbstr = str.mb_str(wxConvUTF8); @@ -332,12 +349,41 @@ inline static void CacheWriteString(wxOutputStream *f, const wxString& str) f->Write((const char*)mbstr, len); } -inline static wxString CacheReadString(wxInputStream *f) +// Read a string written by CacheWriteString(), i.e. a byte count including the +// trailing NUL followed by that many bytes. Returns false for a length that +// can't have been produced by CacheWriteString(), leaving the file to be +// treated as unusable by the caller. +inline static bool CacheReadString(wxInputStream *f, wxString& str) { - size_t len = (size_t)CacheReadInt32(f); - wxCharBuffer str(len-1); - f->Read(str.data(), len); - return wxString(str, wxConvUTF8); + const wxInt32 stored = CacheReadInt32(f); + + // CacheWriteString() always counts the trailing NUL, so the shortest + // string it can write has a length of 1. Anything less would underflow + // when computing the size of the buffer below. + if ( stored < 1 ) + return false; + + const size_t len = (size_t)stored; + + // A single string can't be longer than the file containing it. + const wxFileOffset available = f->GetLength(); + if ( available != wxInvalidOffset && (wxFileOffset)len > available ) + return false; + + // wxCharBuffer allocates len + 1 bytes and puts a NUL in the last one, so + // reading len bytes into it leaves that terminator intact. + wxCharBuffer buf(len); + if ( f->Read(buf.data(), len).LastRead() != len ) + return false; + + // CacheWriteString() always writes the terminator, so a file lacking it is + // malformed. Checking for it also means the conversion below never has to + // go looking for one. + if ( buf.data()[len - 1] != '\0' ) + return false; + + str = wxString(buf.data(), wxConvUTF8, len - 1); + return true; } #define CURRENT_CACHED_BOOK_VERSION 5 @@ -368,29 +414,40 @@ bool wxHtmlHelpData::LoadCachedBook(wxHtmlBookRecord *book, wxInputStream *f) return false; /* load contents : */ + int count; + if (!CacheReadCount(f, count)) + return false; st = m_contents.size(); - newsize = st + CacheReadInt32(f); + newsize = st + count; m_contents.Alloc(newsize); for (i = st; i < newsize; i++) { wxHtmlHelpDataItem *item = new wxHtmlHelpDataItem; item->level = CacheReadInt32(f); item->id = CacheReadInt32(f); - item->name = CacheReadString(f); - item->page = CacheReadString(f); + if (!CacheReadString(f, item->name) || !CacheReadString(f, item->page)) + { + delete item; + return false; + } item->book = book; m_contents.Add(item); } /* load index : */ + if (!CacheReadCount(f, count)) + return false; st = m_index.size(); - newsize = st + CacheReadInt32(f); + newsize = st + count; m_index.Alloc(newsize); for (i = st; i < newsize; i++) { wxHtmlHelpDataItem *item = new wxHtmlHelpDataItem; - item->name = CacheReadString(f); - item->page = CacheReadString(f); + if (!CacheReadString(f, item->name) || !CacheReadString(f, item->page)) + { + delete item; + return false; + } item->level = CacheReadInt32(f); item->book = book; int parentShift = CacheReadInt32(f);