From 2193d6dd84a16603017c2904f7fbf2160b88f2ce Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 8 Jan 2015 12:05:17 +0900 Subject: [PATCH 1/4] Fix an out-of-bounds access and consequent errors while parsing fuzzed MPC files. Consequent errors may vary: segfault, zerodiv and so forth. --- taglib/mpc/mpcproperties.cpp | 20 +++++++++++--------- tests/data/zerodiv.mpc | Bin 0 -> 405 bytes tests/test_mpc.cpp | 9 ++++++++- 3 files changed, 19 insertions(+), 10 deletions(-) create mode 100644 tests/data/zerodiv.mpc diff --git a/taglib/mpc/mpcproperties.cpp b/taglib/mpc/mpcproperties.cpp index d406f8d7..f11f8ecf 100644 --- a/taglib/mpc/mpcproperties.cpp +++ b/taglib/mpc/mpcproperties.cpp @@ -183,7 +183,9 @@ unsigned long readSize(const ByteVector &data, TagLib::uint &sizelength) return size; } -static const unsigned short sftable [4] = { 44100, 48000, 37800, 32000 }; +// This array looks weird, but the same as original MusePack code found at: +// https://www.musepack.net/index.php?pg=src +static const unsigned short sftable [8] = { 44100, 48000, 37800, 32000, 0, 0, 0, 0 }; void MPC::Properties::readSV8(File *file) { @@ -207,17 +209,17 @@ void MPC::Properties::readSV8(File *file) d->sampleFrames = readSize(data.mid(pos), pos); ulong begSilence = readSize(data.mid(pos), pos); - std::bitset<16> flags(TAGLIB_CONSTRUCT_BITSET(data.toUShort(pos, true))); + const ushort flags = data.toUShort(pos, true); pos += 2; - d->sampleRate = sftable[flags[15] * 4 + flags[14] * 2 + flags[13]]; - d->channels = flags[7] * 8 + flags[6] * 4 + flags[5] * 2 + flags[4] + 1; + d->sampleRate = sftable[(flags >> 13) & 0x07]; + d->channels = ((flags >> 4) & 0x0F) + 1; - if((d->sampleFrames - begSilence) != 0) - d->bitrate = (int)(d->streamLength * 8.0 * d->sampleRate / (d->sampleFrames - begSilence)); - d->bitrate = d->bitrate / 1000; - - d->length = (d->sampleFrames - begSilence) / d->sampleRate; + const uint frameCount = d->sampleFrames - begSilence; + if(frameCount != 0 && d->sampleRate != 0) { + d->bitrate = (int)(d->streamLength * 8.0 * d->sampleRate / frameCount / 1000); + d->length = frameCount / d->sampleRate; + } } else if (packetType == "RG") { diff --git a/tests/data/zerodiv.mpc b/tests/data/zerodiv.mpc new file mode 100644 index 0000000000000000000000000000000000000000..d3ea57c75ca8a58f8bcc9bf0388935d59f8d70d5 GIT binary patch literal 405 zcmeYbaP|)N;Gg>Ipa@4}XS)kS0|OX1<|LKoGB_oAYnU*r0Omshf*Zff{mv8pNPtA(!>Pz6wf z0!T@+LUF5wf&$P~h4RdjjQ>DT4AJ|VAp})#WNKm&gS~-)fn$Jch@-n}`~`LfhLsE) z3~X^FD3+ip)Jy}q&kSN6Lx=zalI;x9V8`SZpgQCK|B}RXJkI!o #include #include #include #include #include #include +#include #include "utils.h" using namespace std; @@ -17,6 +17,7 @@ class TestMPC : public CppUnit::TestFixture CPPUNIT_TEST(testPropertiesSV7); CPPUNIT_TEST(testPropertiesSV5); CPPUNIT_TEST(testPropertiesSV4); + CPPUNIT_TEST(testFuzzedFile1); CPPUNIT_TEST_SUITE_END(); public: @@ -61,6 +62,12 @@ public: CPPUNIT_ASSERT_EQUAL(44100, f.audioProperties()->sampleRate()); } + void testFuzzedFile1() + { + MPC::File f(TEST_FILE_PATH_C("zerodiv.mpc")); + CPPUNIT_ASSERT(f.isValid()); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestMPC); From 65664e685553579493c91b38b93bf0f981c27688 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 8 Jan 2015 12:28:20 +0900 Subject: [PATCH 2/4] Check for EOF to fix an infinite loop while parsing fuzzed MPC files. --- taglib/mpc/mpcproperties.cpp | 8 ++++++-- tests/data/infloop.mpc | Bin 0 -> 434 bytes tests/test_mpc.cpp | 7 +++++++ 3 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 tests/data/infloop.mpc diff --git a/taglib/mpc/mpcproperties.cpp b/taglib/mpc/mpcproperties.cpp index f11f8ecf..a162b8ee 100644 --- a/taglib/mpc/mpcproperties.cpp +++ b/taglib/mpc/mpcproperties.cpp @@ -197,10 +197,15 @@ void MPC::Properties::readSV8(File *file) unsigned long packetSize = readSize(file, packetSizeLength); unsigned long dataSize = packetSize - 2 - packetSizeLength; + const ByteVector data = file->readBlock(dataSize); + if(data.size() != dataSize) { + debug("MPC::Properties::readSV8() - dataSize doesn't match the actual data size."); + break; + } + if(packetType == "SH") { // Stream Header // http://trac.musepack.net/wiki/SV8Specification#StreamHeaderPacket - ByteVector data = file->readBlock(dataSize); readSH = true; TagLib::uint pos = 4; @@ -225,7 +230,6 @@ void MPC::Properties::readSV8(File *file) else if (packetType == "RG") { // Replay Gain // http://trac.musepack.net/wiki/SV8Specification#ReplaygainPacket - ByteVector data = file->readBlock(dataSize); readRG = true; int replayGainVersion = data[0]; diff --git a/tests/data/infloop.mpc b/tests/data/infloop.mpc new file mode 100644 index 0000000000000000000000000000000000000000..46861ab378cc3060fb759333490bb4d2852ccd36 GIT binary patch literal 434 zcmeYbaP|&9fgfCgN*5#-zhGv70Qc0qqErT_#GIVO6fuxcNKs;OHUooWQFdl=395O- z=_{dukwB*~WR(_|D5T{VDU_rZm#|@Rz9v*GB(o$Zl_3|+wT3KE>B!W?A_gM^0|UnZ j*APc{*N6)Z Date: Thu, 8 Jan 2015 12:49:33 +0900 Subject: [PATCH 3/4] Check the packet size to fix a segfault error while parsing fuzzed MPC files. --- taglib/mpc/mpcproperties.cpp | 21 +++++++++++++++++++++ tests/data/segfault.mpc | Bin 0 -> 19 bytes tests/test_mpc.cpp | 7 +++++++ 3 files changed, 28 insertions(+) create mode 100644 tests/data/segfault.mpc diff --git a/taglib/mpc/mpcproperties.cpp b/taglib/mpc/mpcproperties.cpp index a162b8ee..ac9d1b45 100644 --- a/taglib/mpc/mpcproperties.cpp +++ b/taglib/mpc/mpcproperties.cpp @@ -206,13 +206,28 @@ void MPC::Properties::readSV8(File *file) if(packetType == "SH") { // Stream Header // http://trac.musepack.net/wiki/SV8Specification#StreamHeaderPacket + + if(dataSize <= 5) { + debug("MPC::Properties::readSV8() - \"SH\" packet is too short to parse."); + break; + } + readSH = true; TagLib::uint pos = 4; d->version = data[pos]; pos += 1; d->sampleFrames = readSize(data.mid(pos), pos); + if(pos > dataSize - 3) { + debug("MPC::Properties::readSV8() - \"SH\" packet is corrupt."); + break; + } + ulong begSilence = readSize(data.mid(pos), pos); + if(pos > dataSize - 2) { + debug("MPC::Properties::readSV8() - \"SH\" packet is corrupt."); + break; + } const ushort flags = data.toUShort(pos, true); pos += 2; @@ -230,6 +245,12 @@ void MPC::Properties::readSV8(File *file) else if (packetType == "RG") { // Replay Gain // http://trac.musepack.net/wiki/SV8Specification#ReplaygainPacket + + if(dataSize <= 9) { + debug("MPC::Properties::readSV8() - \"RG\" packet is too short to parse."); + break; + } + readRG = true; int replayGainVersion = data[0]; diff --git a/tests/data/segfault.mpc b/tests/data/segfault.mpc new file mode 100644 index 0000000000000000000000000000000000000000..2c7e29fb78fc1b5eff50e48411925d50562fd666 GIT binary patch literal 19 ZcmeYbaP|)NV4nKxpa=&85d3Fg0026+1vLNw literal 0 HcmV?d00001 diff --git a/tests/test_mpc.cpp b/tests/test_mpc.cpp index c79d0a8c..1204b0c2 100644 --- a/tests/test_mpc.cpp +++ b/tests/test_mpc.cpp @@ -19,6 +19,7 @@ class TestMPC : public CppUnit::TestFixture CPPUNIT_TEST(testPropertiesSV4); CPPUNIT_TEST(testFuzzedFile1); CPPUNIT_TEST(testFuzzedFile2); + CPPUNIT_TEST(testFuzzedFile3); CPPUNIT_TEST_SUITE_END(); public: @@ -75,6 +76,12 @@ public: CPPUNIT_ASSERT(f.isValid()); } + void testFuzzedFile3() + { + MPC::File f(TEST_FILE_PATH_C("segfault.mpc")); + CPPUNIT_ASSERT(f.isValid()); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestMPC); From e463d14f2e483f21ab14b18a7c84574e60691fd0 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 8 Jan 2015 13:05:56 +0900 Subject: [PATCH 4/4] Check for EOF to fix a segfault while parsing fuzzed MPC files. --- taglib/mpc/mpcproperties.cpp | 37 ++++++++++++++++++++++++------------ tests/data/segfault2.mpc | 1 + tests/test_mpc.cpp | 7 +++++++ 3 files changed, 33 insertions(+), 12 deletions(-) create mode 100644 tests/data/segfault2.mpc diff --git a/taglib/mpc/mpcproperties.cpp b/taglib/mpc/mpcproperties.cpp index ac9d1b45..128eff42 100644 --- a/taglib/mpc/mpcproperties.cpp +++ b/taglib/mpc/mpcproperties.cpp @@ -155,30 +155,36 @@ int MPC::Properties::albumPeak() const // private members //////////////////////////////////////////////////////////////////////////////// -unsigned long readSize(File *file, TagLib::uint &sizelength) +unsigned long readSize(File *file, TagLib::uint &sizeLength, bool &eof) { + sizeLength = 0; + eof = false; + unsigned char tmp; unsigned long size = 0; do { - ByteVector b = file->readBlock(1); + const ByteVector b = file->readBlock(1); + if(b.isEmpty()) { + eof = true; + break; + } + tmp = b[0]; size = (size << 7) | (tmp & 0x7F); - sizelength++; + sizeLength++; } while((tmp & 0x80)); return size; } -unsigned long readSize(const ByteVector &data, TagLib::uint &sizelength) +unsigned long readSize(const ByteVector &data, TagLib::uint &pos) { unsigned char tmp; unsigned long size = 0; - unsigned long pos = 0; do { tmp = data[pos++]; size = (size << 7) | (tmp & 0x7F); - sizelength++; } while((tmp & 0x80) && (pos < data.size())); return size; } @@ -192,10 +198,17 @@ void MPC::Properties::readSV8(File *file) bool readSH = false, readRG = false; while(!readSH && !readRG) { - ByteVector packetType = file->readBlock(2); - uint packetSizeLength = 0; - unsigned long packetSize = readSize(file, packetSizeLength); - unsigned long dataSize = packetSize - 2 - packetSizeLength; + const ByteVector packetType = file->readBlock(2); + + uint packetSizeLength; + bool eof; + const unsigned long packetSize = readSize(file, packetSizeLength, eof); + if(eof) { + debug("MPC::Properties::readSV8() - Reached to EOF."); + break; + } + + const unsigned long dataSize = packetSize - 2 - packetSizeLength; const ByteVector data = file->readBlock(dataSize); if(data.size() != dataSize) { @@ -217,13 +230,13 @@ void MPC::Properties::readSV8(File *file) TagLib::uint pos = 4; d->version = data[pos]; pos += 1; - d->sampleFrames = readSize(data.mid(pos), pos); + d->sampleFrames = readSize(data, pos); if(pos > dataSize - 3) { debug("MPC::Properties::readSV8() - \"SH\" packet is corrupt."); break; } - ulong begSilence = readSize(data.mid(pos), pos); + ulong begSilence = readSize(data, pos); if(pos > dataSize - 2) { debug("MPC::Properties::readSV8() - \"SH\" packet is corrupt."); break; diff --git a/tests/data/segfault2.mpc b/tests/data/segfault2.mpc new file mode 100644 index 00000000..fcfa982f --- /dev/null +++ b/tests/data/segfault2.mpc @@ -0,0 +1 @@ +MPCKSH \ No newline at end of file diff --git a/tests/test_mpc.cpp b/tests/test_mpc.cpp index 1204b0c2..0589740f 100644 --- a/tests/test_mpc.cpp +++ b/tests/test_mpc.cpp @@ -20,6 +20,7 @@ class TestMPC : public CppUnit::TestFixture CPPUNIT_TEST(testFuzzedFile1); CPPUNIT_TEST(testFuzzedFile2); CPPUNIT_TEST(testFuzzedFile3); + CPPUNIT_TEST(testFuzzedFile4); CPPUNIT_TEST_SUITE_END(); public: @@ -82,6 +83,12 @@ public: CPPUNIT_ASSERT(f.isValid()); } + void testFuzzedFile4() + { + MPC::File f(TEST_FILE_PATH_C("segfault2.mpc")); + CPPUNIT_ASSERT(f.isValid()); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestMPC);