From 46d377866e7d264adcd2168738a170b46bd05eaa Mon Sep 17 00:00:00 2001 From: avwarez Date: Sun, 30 Aug 2026 22:30:15 +0200 Subject: [PATCH 1/3] Do not index past the single ID3v2.4 extended header flag byte --- src/header_tag.cpp | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/header_tag.cpp b/src/header_tag.cpp index 6e4214f..bdb7cdf 100644 --- a/src/header_tag.cpp +++ b/src/header_tag.cpp @@ -179,14 +179,21 @@ void ID3_TagHeader::ParseExtended(ID3_Reader& reader) io::readUInt28(reader); const int extflagbytes = reader.readChar(); //Number of flag bytes if (extflagbytes > 0) { - ID3_Flags* extflags[1]; // ID3V2_4_0 has 1 flag byte, extflagbytes should be equal to 1 - for (uint16 i = 0; i < extflagbytes; ++i) + // ID3V2_4_0 defines exactly one flag byte, and only that one is ever + // examined below. extflagbytes, however, is a single byte read straight + // from the file, so a malformed tag can declare up to 255 of them: the + // loop that used to fill an ID3_Flags*[1] here wrote one pointer per + // declared byte into a one-element array, and never freed any of the + // ID3_Flags it allocated. Keep the first flag byte, consume and discard + // the rest, and never index past the one flag byte the spec defines. + ID3_Flags extflags; + extflags.set(static_cast(reader.readChar())); //flags + for (int i = 1; i < extflagbytes; ++i) { - extflags[i] = LEAKTESTNEW(ID3_Flags); - extflags[i]->set(reader.readChar()); //flags + reader.readChar(); //flag bytes beyond the one ID3V2_4_0 defines } - //extflags[0]->test(EXT_HEADER_FLAG_BIT1); // ID3V2_4_0 ext header flag bit 1 *should* be 0 - if (extflags[0]->test(EXT_HEADER_FLAG_BIT2)) + //extflags.test(EXT_HEADER_FLAG_BIT1); // ID3V2_4_0 ext header flag bit 1 *should* be 0 + if (extflags.test(EXT_HEADER_FLAG_BIT2)) { // ID3V2_4_0 ext header flag bit 2 = Tag is an update // read size @@ -197,7 +204,7 @@ void ID3_TagHeader::ParseExtended(ID3_Reader& reader) reader.setCur(reader.getCur() + extheaderflagdatasize); //reader.readChars(buf, extheaderflagdatasize); //buf should be at least 127 bytes = max extended header flagdata size } - if (extflags[0]->test(EXT_HEADER_FLAG_BIT3)) + if (extflags.test(EXT_HEADER_FLAG_BIT3)) { // ID3V2_4_0 ext header flag bit 3 = CRC data present // read size @@ -208,7 +215,7 @@ void ID3_TagHeader::ParseExtended(ID3_Reader& reader) reader.setCur(reader.getCur() + extheaderflagdatasize); //reader.readChars(buf, extheaderflagdatasize); //buf should be at least 127 bytes = max extended header flagdata size } - if (extflags[0]->test(EXT_HEADER_FLAG_BIT4)) + if (extflags.test(EXT_HEADER_FLAG_BIT4)) { // ID3V2_4_0 ext header flag bit 4 = Tag restrictions // read size From 3bd16b6bea4239acbe68ab15fd0100ca2afb73e6 Mon Sep 17 00:00:00 2001 From: avwarez Date: Sun, 30 Aug 2026 22:30:15 +0200 Subject: [PATCH 2/3] Limit nesting depth of ID3v2.2.1 compressed meta frames --- src/tag_parse.cpp | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/src/tag_parse.cpp b/src/tag_parse.cpp index d32ead7..32fe7f2 100644 --- a/src/tag_parse.cpp +++ b/src/tag_parse.cpp @@ -41,7 +41,15 @@ using namespace dami; namespace { - bool parseFrames(ID3_TagImpl& tag, ID3_Reader& rdr) + // An ID3v2.2.1 compressed meta frame (CDM) holds frames of its own, which + // parseFrames() below unpacks by calling itself. Nothing in the format stops + // those inner frames from being CDM frames too, so the nesting depth is + // whatever the file says it is, and each level costs a decompression buffer + // and a stack frame. No real tag nests these at all; the ceiling below is + // generous and only ever stops the pathological case. + const size_t MAX_COMPRESSION_DEPTH = 16; + + bool parseFrames(ID3_TagImpl& tag, ID3_Reader& rdr, size_t depth = 0) { ID3_Reader::pos_type beg = rdr.getCur(); io::ExitTrigger et(rdr, beg); @@ -99,15 +107,26 @@ namespace } else { - uint32 newSize = io::readBENumber(mr, sizeof(uint32)); - io::CompressedReader cr(mr, (uLong)newSize); - parseFrames(tag, cr); - if (!cr.atEnd()) + if (depth + 1 >= MAX_COMPRESSION_DEPTH) { - // hmm. it didn't parse the entire uncompressed data. wonder - // why. - ID3D_WARNING( "id3::v2::parseFrames(): didn't parse entire " << - "id3v2.2.1 compressed memory stream"); + // Don't descend, and don't uncompress either - the buffer this + // level would allocate is as much a waste as the recursion. + ID3D_WARNING( "id3::v2::parseFrames(): compressed frames nested " << + "deeper than " << MAX_COMPRESSION_DEPTH << + " levels, not descending" ); + } + else + { + uint32 newSize = io::readBENumber(mr, sizeof(uint32)); + io::CompressedReader cr(mr, (uLong)newSize); + parseFrames(tag, cr, depth + 1); + if (!cr.atEnd()) + { + // hmm. it didn't parse the entire uncompressed data. wonder + // why. + ID3D_WARNING( "id3::v2::parseFrames(): didn't parse entire " << + "id3v2.2.1 compressed memory stream"); + } } } } From 590edb90b644dee9a4ea7311cf98be8d4a7fdf7b Mon Sep 17 00:00:00 2001 From: avwarez Date: Sun, 30 Aug 2026 22:30:15 +0200 Subject: [PATCH 3/3] Bound the decompression buffer by what the compressed data can yield --- src/io_decorators.cpp | 42 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/src/io_decorators.cpp b/src/io_decorators.cpp index 5e25b3d..045c94f 100644 --- a/src/io_decorators.cpp +++ b/src/io_decorators.cpp @@ -198,16 +198,48 @@ ID3_Reader::int_type io::UnsyncedReader::readChar() } io::CompressedReader::CompressedReader(ID3_Reader& reader, uLong newSize) - : _uncompressed(new char_type[newSize]) + : _uncompressed(NULL) { size_type oldSize = reader.remainingBytes(); BString binary = readBinary(reader, oldSize); - ::uncompress(_uncompressed, - &newSize, - static_cast(binary.data()), - static_cast(oldSize)); + // newSize is the uncompressed length the tag declares, read verbatim from + // four bytes of the file, and nothing so far has compared it against the + // compressed data actually at hand. Deflate cannot expand by more than + // 1032:1, so anything past that ceiling is unattainable no matter what the + // stream turns out to hold, and clamping to it costs a well-formed tag + // nothing. The slack keeps the arithmetic honest for tiny inputs. Computed + // wider than uLong (32 bits on Windows) and only then narrowed, so a large + // frame cannot wrap the ceiling round to a small one. + const uLong maxUL = static_cast(-1); + const uLong maxSize = (oldSize > (maxUL - 1024) / 1032) + ? maxUL + : static_cast(oldSize) * 1032 + 1024; + if (newSize > maxSize) + { + ID3D_WARNING( "io::CompressedReader: claimed uncompressed size " << newSize << + " exceeds what " << oldSize << " compressed bytes can yield, " + "clamping to " << maxSize ); + newSize = maxSize; + } + + _uncompressed = new char_type[newSize]; + + // uncompress() leaves newSize at however much it actually wrote, so a short + // or corrupt stream yields a correspondingly short buffer rather than one + // with an uninitialised tail. A failure is worth noting but not worth + // discarding what did come out - the frames that decompressed are still + // parseable. + int err = ::uncompress(_uncompressed, + &newSize, + static_cast(binary.data()), + static_cast(oldSize)); + if (err != Z_OK) + { + ID3D_WARNING( "io::CompressedReader: uncompress failed with " << err << + ", using the " << newSize << " bytes it produced" ); + } this->setBuffer(_uncompressed, newSize); }