From f9a747dceb694e7adcf65705f2b784ba806e7301 Mon Sep 17 00:00:00 2001
From: Tsuda Kageyu <tsuda.kageyu@gmail.com>
Date: Thu, 10 Nov 2016 20:02:30 +0900
Subject: [PATCH] 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");
-- 
2.40.0