From f9a747dceb694e7adcf65705f2b784ba806e7301 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 10 Nov 2016 20:02:30 +0900 Subject: [PATCH 1/2] Avoid adding fields with invalid keys to Vorbis Comments. According to the spec, '\x7F' is not allowed. --- taglib/ogg/xiphcomment.cpp | 21 ++++++++++++++++----- tests/test_xiphcomment.cpp | 20 +++++++++++++++++--- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/taglib/ogg/xiphcomment.cpp b/taglib/ogg/xiphcomment.cpp index db46aecb..d6b8e11c 100644 --- a/taglib/ogg/xiphcomment.cpp +++ b/taglib/ogg/xiphcomment.cpp @@ -261,10 +261,14 @@ bool Ogg::XiphComment::checkKey(const String &key) { if(key.size() < 1) return false; - for(String::ConstIterator it = key.begin(); it != key.end(); it++) - // forbid non-printable, non-ascii, '=' (#61) and '~' (#126) - if (*it < 32 || *it >= 128 || *it == 61 || *it == 126) + + // A key may consist of ASCII 0x20 through 0x7D, 0x3D ('=') excluded. + + for(String::ConstIterator it = key.begin(); it != key.end(); it++) { + if(*it < 0x20 || *it > 0x7D || *it == 0x3D) return false; + } + return true; } @@ -275,11 +279,18 @@ String Ogg::XiphComment::vendorID() const void Ogg::XiphComment::addField(const String &key, const String &value, bool replace) { + if(!checkKey(key)) { + debug("Ogg::XiphComment::addField() - Invalid key. Field not added."); + return; + } + + const String upperKey = key.upper(); + if(replace) - removeFields(key.upper()); + removeFields(upperKey); if(!key.isEmpty() && !value.isEmpty()) - d->fieldListMap[key.upper()].append(value); + d->fieldListMap[upperKey].append(value); } void Ogg::XiphComment::removeField(const String &key, const String &value) diff --git a/tests/test_xiphcomment.cpp b/tests/test_xiphcomment.cpp index 699f60a5..d2a28a1f 100644 --- a/tests/test_xiphcomment.cpp +++ b/tests/test_xiphcomment.cpp @@ -42,7 +42,8 @@ class TestXiphComment : public CppUnit::TestFixture CPPUNIT_TEST(testSetYear); CPPUNIT_TEST(testTrack); CPPUNIT_TEST(testSetTrack); - CPPUNIT_TEST(testInvalidKeys); + CPPUNIT_TEST(testInvalidKeys1); + CPPUNIT_TEST(testInvalidKeys2); CPPUNIT_TEST(testClearComment); CPPUNIT_TEST(testRemoveFields); CPPUNIT_TEST(testPicture); @@ -90,19 +91,32 @@ public: CPPUNIT_ASSERT_EQUAL(String("3"), cmt.fieldListMap()["TRACKNUMBER"].front()); } - void testInvalidKeys() + void testInvalidKeys1() { PropertyMap map; map[""] = String("invalid key: empty string"); map["A=B"] = String("invalid key: contains '='"); map["A~B"] = String("invalid key: contains '~'"); + map["A\x7F\B"] = String("invalid key: contains '\x7F'"); + map[L"A\x3456\B"] = String("invalid key: Unicode"); Ogg::XiphComment cmt; PropertyMap unsuccessful = cmt.setProperties(map); - CPPUNIT_ASSERT_EQUAL((unsigned int)3, unsuccessful.size()); + CPPUNIT_ASSERT_EQUAL((unsigned int)5, unsuccessful.size()); CPPUNIT_ASSERT(cmt.properties().isEmpty()); } + void testInvalidKeys2() + { + Ogg::XiphComment cmt; + cmt.addField("", "invalid key: empty string"); + cmt.addField("A=B", "invalid key: contains '='"); + cmt.addField("A~B", "invalid key: contains '~'"); + cmt.addField("A\x7F\B", "invalid key: contains '\x7F'"); + cmt.addField(L"A\x3456\B", "invalid key: Unicode"); + CPPUNIT_ASSERT_EQUAL(0U, cmt.fieldCount()); + } + void testClearComment() { ScopedFileCopy copy("empty", ".ogg"); From b98a984b66f69cacfcbf259ac3b2d77ea3c9415f Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 11 Nov 2016 00:07:32 +0900 Subject: [PATCH 2/2] Fix handling of lowercase 'metadata_block_picture' fields in Vorbis comments. Also refactored some redundant code for parsing pictures. --- taglib/ogg/xiphcomment.cpp | 122 ++++++++++++++------------------ tests/data/lowercase-fields.ogg | Bin 0 -> 4477 bytes tests/test_xiphcomment.cpp | 18 +++++ 3 files changed, 71 insertions(+), 69 deletions(-) create mode 100644 tests/data/lowercase-fields.ogg diff --git a/taglib/ogg/xiphcomment.cpp b/taglib/ogg/xiphcomment.cpp index d6b8e11c..f56bf810 100644 --- a/taglib/ogg/xiphcomment.cpp +++ b/taglib/ogg/xiphcomment.cpp @@ -447,85 +447,69 @@ void Ogg::XiphComment::parse(const ByteVector &data) const unsigned int commentLength = data.toUInt(pos, false); pos += 4; - ByteVector entry = data.mid(pos, commentLength); - + const ByteVector entry = data.mid(pos, commentLength); pos += commentLength; // Don't go past data end + if(pos > data.size()) break; - // Handle Pictures separately - if(entry.startsWith("METADATA_BLOCK_PICTURE=")) { - - // We need base64 encoded data including padding - if((entry.size() - 23) > 3 && ((entry.size() - 23) % 4) == 0) { - - // Decode base64 picture data - ByteVector picturedata = ByteVector::fromBase64(entry.mid(23)); - if(picturedata.size()) { - - // Decode Flac Picture - FLAC::Picture * picture = new FLAC::Picture(); - if(picture->parse(picturedata)) { - - d->pictureList.append(picture); - - // continue to next field - continue; - } - else { - delete picture; - debug("Failed to decode FlacPicture block"); - } - } - else { - debug("Failed to decode base64 encoded data"); - } - } - else { - debug("Invalid base64 encoded data"); - } - } - - // Handle old picture standard - if(entry.startsWith("COVERART=")) { - - if((entry.size() - 9) > 3 && ((entry.size() - 9) % 4) == 0) { - - // Decode base64 picture data - ByteVector picturedata = ByteVector::fromBase64(entry.mid(9)); - if (picturedata.size()) { - - // Assume it's some type of image file - FLAC::Picture * picture = new FLAC::Picture(); - picture->setData(picturedata); - picture->setMimeType("image/"); - picture->setType(FLAC::Picture::Other); - d->pictureList.append(picture); - - // continue to next field - continue; - } - else { - debug("Failed to decode base64 encoded data"); - } - } - else { - debug("Invalid base64 encoded data"); - } - } - // Check for field separator - int sep = entry.find('='); + + const int sep = entry.find('='); if(sep < 1) { - debug("Discarding invalid comment field."); + debug("Ogg::XiphComment::parse() - Discarding a field. Separator not found."); continue; } - // Parse key and value - String key = String(entry.mid(0, sep), String::UTF8); - String value = String(entry.mid(sep + 1), String::UTF8); - addField(key, value, false); + // Parse the key + + const String key = String(entry.mid(0, sep), String::UTF8).upper(); + if(!checkKey(key)) { + debug("Ogg::XiphComment::parse() - Discarding a field. Invalid key."); + continue; + } + + if(key == "METADATA_BLOCK_PICTURE" || key == "COVERART") { + + // Handle Pictures separately + + const ByteVector picturedata = ByteVector::fromBase64(entry.mid(sep + 1)); + if(picturedata.isEmpty()) { + debug("Ogg::XiphComment::parse() - Discarding a field. Invalid base64 data"); + continue; + } + + if(key[0] == L'M') { + + // Decode FLAC Picture + + FLAC::Picture * picture = new FLAC::Picture(); + if(picture->parse(picturedata)) { + d->pictureList.append(picture); + } + else { + delete picture; + debug("Ogg::XiphComment::parse() - Failed to decode FLAC Picture block"); + } + } + else { + + // Assume it's some type of image file + + FLAC::Picture * picture = new FLAC::Picture(); + picture->setData(picturedata); + picture->setMimeType("image/"); + picture->setType(FLAC::Picture::Other); + d->pictureList.append(picture); + } + } + else { + + // Parse the text + + addField(key, String(entry.mid(sep + 1), String::UTF8), false); + } } } diff --git a/tests/data/lowercase-fields.ogg b/tests/data/lowercase-fields.ogg new file mode 100644 index 0000000000000000000000000000000000000000..0ddd49357a906c70d6f3e5f222f0fa5404e4e2cb GIT binary patch literal 4477 zcmeHLeNatHl+?=l0qu3&|>lZIIRhca`BP{0# z{ezHojK%z(U=`3a9~62fJ@Qrimjm3>|NMSg5gyXC76eMnjddFenrej&=Bi>wzfdF0 zkjXM-8)fov5Qu}PvZ>i@Y2Iv5>kUE`WzgsiwZPqDYObuQY_5Evs^O*T`WG6_)y*wU zrp+oSB;BYQ8>_bGzF4ML?5)l#ev#&E*`C=@(?*8)MKs4qH+giUrW;i?-DvdZ4 z)|Zx0br_gxHa9nzLiPV0XbQJJCWSR zZ5L#0E1wWsdPKfH(ovqef2zlb=n1c=T6uP-iX*a%H(lf!TAxvO3td%2!vRO19JIJUw-$u`7XTF zC1&cvt3m0+C{r_->G=p17R9Ld@>VeOY7pY|WjXyNPJf+q(b3JZA2+eu^IEOp-6`;JTa2-11!VJb;{0I?}KdRYhwqZh817+ z&i9qZdbJ~pwY~XcZLYVSA1M}lEtBULdebL?8t5&J@q*l9@0q^R=n$1M(l+Il(7e5q zH+J@N$AMZu+}7iLZ49W0VRN^4>j+T!AGJ|$Z>4z_S{2jWJbOdX8&7u!fB%u^?Wwas z6^!5bvKMx8-7s-%n?s_&L$QAxI5^5*nXozK0ABR;s7 z(x!v}4?{|hsDnr4-*2iMq67S<3&gv9?OMsu;6PW#g!KK3Otb0sk@sVti%CC&ZFy$+ z45ih{ z>?wNIG@DtWf40I{yl1;-PxYyXUi9xW*+myK37!gKv3nymd$tWJC}~i_paxXmt*?ItN?3mmIFTw>*PG z-e)cZZl64Q>xb$}=78f6P4K)XJdeWF6c7x$))*RH!N=*ix=A`QBM3)zN;B%yE>r5udgBij-K<;}55PvD9v14s^l541-l$HzYG(I) zebq74ZU0EVq;~AX3+WyFv7aZs;x58|J%m>~RTtBIQ^fk=IRn|j^SxOuNuXjWa9#(( zysYbu0_CU_DuWC{J@Qpk*{hGqS1+tLp@sdW9 zks}^8=EvbtF7~J4`JM=!E$~M<%Vko{_L*h6hgje+pqdprkp>2)Tn9g*td)h?geo8rVZv)Khavv;xWkool8Y#1}yZT=5l5`%Mwy#{R>=!)~W=%!t6hFYzUy!XA`LLvM+|DD{i~TcHQ0Sv5BEfFZ zO2miV)NaCvQN)ltnv{qSQV3n$9}Z7PVcj_`60Sy8SuXw2em>d9tDaP(a4Bc{V(;tP z^aV(RG(;kN%@=)cCgq{DG*3bJnY_nMX(3a3UQ3AWld2TIdj|5Oc!UfL+K10Au-B;= zxG5Hj>8_^X2BbVVP0V7(^ffZ+N2cpd~d($EJbR}vsE&>4*tqf!RTnqX^w$uRwiaPZsB85lJhh@)|Myy;SmT zJ&=-{Ffv+XcT*gAD;_!iaVQ^-eAmbvolwmwYoP(kdXecqY$$s4VfR0x(jYlwGPFmU z2=otQ7P|MdtH(B-)=Zb|1dp<;aNJMv{d`LdN%##M@IbSntaF5Pq%Y#W&&nFkS|(Mg zqJwi)(nFS5k|53&R~Hd>iqfVXFX`0wA9hjN;majm=|jG`vd-c0sxA%j!DnTi>1)hp0*60`767+ug4lG8TAb40%(~ zvW8A^8eDt0K}U~O3X=4iNxE#j!8v3*vypttek_?KEwuX=upkhBOvFG8!( z9a!KF#N|$3tgNYPXl!nM*=l!m^+14xl89c0o2yuvlOwnA6O*14tbOWfp(uGB4q-g> sCxFn3*z8b7MMrQpo4Mt)PyH$s&*@t}P3`*azVd$7kG-Ji*dKTK7v71uJOBUy literal 0 HcmV?d00001 diff --git a/tests/test_xiphcomment.cpp b/tests/test_xiphcomment.cpp index d2a28a1f..64ae0b21 100644 --- a/tests/test_xiphcomment.cpp +++ b/tests/test_xiphcomment.cpp @@ -47,6 +47,7 @@ class TestXiphComment : public CppUnit::TestFixture CPPUNIT_TEST(testClearComment); CPPUNIT_TEST(testRemoveFields); CPPUNIT_TEST(testPicture); + CPPUNIT_TEST(testLowercaseFields); CPPUNIT_TEST_SUITE_END(); public: @@ -192,6 +193,23 @@ public: } } + void testLowercaseFields() + { + const ScopedFileCopy copy("lowercase-fields", ".ogg"); + { + Vorbis::File f(copy.fileName().c_str()); + List lst = f.tag()->pictureList(); + CPPUNIT_ASSERT_EQUAL(String("TEST TITLE"), f.tag()->title()); + CPPUNIT_ASSERT_EQUAL(String("TEST ARTIST"), f.tag()->artist()); + CPPUNIT_ASSERT_EQUAL((unsigned int)1, lst.size()); + f.save(); + } + { + Vorbis::File f(copy.fileName().c_str()); + CPPUNIT_ASSERT(f.find("METADATA_BLOCK_PICTURE") > 0); + } + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestXiphComment);