Add check for ID3v2 frame data length (#1300)

Also fix some wrong frame sizes in the unit tests.

---------

Co-authored-by: Urs Fleisch <ufleisch@users.sourceforge.net>

Author: sbooth

taglib/mpeg/id3v2/id3v2frame.cpp

@@ -295,6 +295,13 @@ ByteVector Frame::fieldData(const ByteVector &frameData) const
     frameDataLength = SynchData::toUInt(frameData.mid(headerSize, 4));
     frameDataOffset += 4;
   }
+  if(frameData.size() >= headerSize &&
+     frameDataOffset + frameDataLength > frameData.size()) {
+    // The first check is needed because some "dual purpose" frame constructors
+    // call this method with only the frame ID, i.e. without a complete header.
+    debug("Invalid frame data length");
+    return ByteVector();
+  }
 
   if(zlib::isAvailable() && d->header->compression() && !d->header->encryption()) {
     if(frameData.size() <= frameDataOffset) {

tests/test_id3v2.cpp

@@ -234,13 +234,13 @@ class TestID3v2 : public CppUnit::TestFixture
     f.setData(multiBomBeData);
     CPPUNIT_ASSERT_EQUAL(String("Foo Bar"), f.toString());
 
-    ByteVector singleBomLeData("TPE1\x00\x00\x00\x13\x00\x00\x01\xff\xfe"
+    ByteVector singleBomLeData("TPE1\x00\x00\x00\x11\x00\x00\x01\xff\xfe"
                                "F\0o\0o\0\0\0"
                                "B\0a\0r\0", 27);
     f.setData(singleBomLeData);
     CPPUNIT_ASSERT_EQUAL(String("Foo Bar"), f.toString());
 
-    ByteVector singleBomBeData("TPE1\x00\x00\x00\x13\x00\x00\x01\xfe\xff"
+    ByteVector singleBomBeData("TPE1\x00\x00\x00\x11\x00\x00\x01\xfe\xff"
                                "\0F\0o\0o\0\0"
                                "\0B\0a\0r", 27);
     f.setData(singleBomBeData);
@@ -281,7 +281,7 @@ class TestID3v2 : public CppUnit::TestFixture
   void testParseAPIC_UTF16_BOM()
   {
     ID3v2::AttachedPictureFrame f(ByteVector(
-      "\x41\x50\x49\x43\x00\x02\x0c\x59\x00\x00\x01\x69\x6d\x61\x67\x65"
+      "\x41\x50\x49\x43\x00\x00\x00\x26\x00\x00\x01\x69\x6d\x61\x67\x65"
       "\x2f\x6a\x70\x65\x67\x00\x00\xfe\xff\x00\x63\x00\x6f\x00\x76\x00"
       "\x65\x00\x72\x00\x2e\x00\x6a\x00\x70\x00\x67\x00\x00\xff\xd8\xff",
       16 * 3));
@@ -668,7 +668,7 @@ class TestID3v2 : public CppUnit::TestFixture
   {
     ID3v2::SynchronizedLyricsFrame f(
       ByteVector("SYLT"                      // Frame ID
-                 "\x00\x00\x00\x21"          // Frame size
+                 "\x00\x00\x00\x1e"          // Frame size
                  "\x00\x00"                  // Frame flags
                  "\x00"                      // Text encoding
                  "eng"                       // Language
@@ -1424,7 +1424,7 @@ class TestID3v2 : public CppUnit::TestFixture
 
     auto chapterData =
       ByteVector("CHAP"                     // Frame ID
-                 "\x00\x00\x00\x20"         // Frame size
+                 "\x00\x00\x00\x12"         // Frame size
                  "\x00\x00"                 // Frame flags
                  "\x43\x00"                 // Element ID ("C")
                  "\x00\x00\x00\x03"         // Start time
@@ -1447,7 +1447,10 @@ class TestID3v2 : public CppUnit::TestFixture
     CPPUNIT_ASSERT(static_cast<unsigned int>(0x03) == f1.endOffset());
     CPPUNIT_ASSERT(static_cast<unsigned int>(0x00) == f1.embeddedFrameList().size());
 
-    ID3v2::ChapterFrame f2(&header, chapterData + embeddedFrameData);
+    auto chapWithTit2Data = chapterData + embeddedFrameData;
+    // Adapt the frame size for the added embedded frame data.
+    chapWithTit2Data[7] = static_cast<char>(chapterData[7] + embeddedFrameData.size());
+    ID3v2::ChapterFrame f2(&header, chapWithTit2Data);
 
     CPPUNIT_ASSERT_EQUAL(ByteVector("C"), f2.elementID());
     CPPUNIT_ASSERT(static_cast<unsigned int>(0x03) == f2.startTime());