Robustness fixes for malformed ID3v2 tags

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<ID3_Flags::TYPE>(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

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<const uchar*>(binary.data()),
-               static_cast<uLong>(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<uLong>(-1);
+  const uLong maxSize = (oldSize > (maxUL - 1024) / 1032)
+                            ? maxUL
+                            : static_cast<uLong>(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<const uchar*>(binary.data()),
+                         static_cast<uLong>(oldSize));
+  if (err != Z_OK)
+  {
+    ID3D_WARNING( "io::CompressedReader: uncompress failed with " << err <<
+                  ", using the " << newSize << " bytes it produced" );
+  }
   this->setBuffer(_uncompressed, newSize);
 }
 

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");
+              }
             }
           }
         }