From 1c71c5a77e037038c581a68774c96fad9a79030b Mon Sep 17 00:00:00 2001
From: Edwin Mons <edwin.mons@isode.com>
Date: Wed, 24 Oct 2018 15:37:33 +0200
Subject: Fix buffer overrun in LibIDNConverter

When Swift::LibIDNConverter::getStringPrepared was called with an input
of 1024 or more characters, stringprep would be called on a memory
region that wasn't NUL-terminated. It also blindly trimmed the input to
1024 bytes, even though there may be input longer than that that still
results in a valid 1023 byte prepped string.

IDNConverterTest has been converted to gtest, as cppunit cannot deal
with testing for std::exceptions being thrown on at least macOS

Test-Information:

Unit tests pass on macOS 10.13 and Debian 9
Before fix, the newly added unit tests triggered an ASan abort due to a
buffer overrun.
After fix, all unit tests pass, even with ASan enabled.

Change-Id: Ia3e51a39f5db1de32b8f8bb388f81ca041136df7

diff --git a/Swiften/IDN/LibIDNConverter.cpp b/Swiften/IDN/LibIDNConverter.cpp
index 0c01352..2325015 100644
--- a/Swiften/IDN/LibIDNConverter.cpp
+++ b/Swiften/IDN/LibIDNConverter.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012-2016 Isode Limited.
+ * Copyright (c) 2012-2018 Isode Limited.
  * All rights reserved.
  * See the COPYING file for more information.
  */
@@ -24,7 +24,7 @@ extern "C" {
 using namespace Swift;
 
 namespace {
-    static const int MAX_STRINGPREP_SIZE = 1024;
+    static const size_t MAX_STRINGPREP_SIZE = 1024;
 
     const Stringprep_profile* getLibIDNProfile(IDNConverter::StringPrepProfile profile) {
         switch(profile) {
@@ -44,7 +44,8 @@ namespace {
             return ContainerType();
         }
 
-        input.resize(MAX_STRINGPREP_SIZE);
+        // Ensure we have enough space for stringprepping, and that input is always NUL terminated
+        input.resize(std::max(MAX_STRINGPREP_SIZE, input.size() + 1));
         if (stringprep(&input[0], MAX_STRINGPREP_SIZE, static_cast<Stringprep_profile_flags>(0), getLibIDNProfile(profile)) == 0) {
             return input;
         }
diff --git a/Swiften/IDN/SConscript b/Swiften/IDN/SConscript
index 28596f7..0afad0e 100644
--- a/Swiften/IDN/SConscript
+++ b/Swiften/IDN/SConscript
@@ -23,6 +23,7 @@ swiften_env.Append(SWIFTEN_OBJECTS = [objects])
 if env["TEST"] :
     test_env = myenv.Clone()
     test_env.UseFlags(swiften_env["CPPUNIT_FLAGS"])
+    test_env.UseFlags(myenv.get("GOOGLETEST_FLAGS", ""))
     env.Append(UNITTEST_OBJECTS = test_env.SwiftenObject([
                 File("UnitTest/IDNConverterTest.cpp"),
                 File("UnitTest/UTF8ValidatorTest.cpp")
diff --git a/Swiften/IDN/UnitTest/IDNConverterTest.cpp b/Swiften/IDN/UnitTest/IDNConverterTest.cpp
index 508a28c..c5f94d0 100644
--- a/Swiften/IDN/UnitTest/IDNConverterTest.cpp
+++ b/Swiften/IDN/UnitTest/IDNConverterTest.cpp
@@ -1,64 +1,80 @@
 /*
- * Copyright (c) 2010-2016 Isode Limited.
+ * Copyright (c) 2010-2018 Isode Limited.
  * All rights reserved.
  * See the COPYING file for more information.
  */
 
 #include <memory>
 
-#include <cppunit/extensions/HelperMacros.h>
-#include <cppunit/extensions/TestFactoryRegistry.h>
+#include <gtest/gtest.h>
 
 #include <Swiften/IDN/IDNConverter.h>
 #include <Swiften/IDN/PlatformIDNConverter.h>
 
 using namespace Swift;
 
-class IDNConverterTest : public CppUnit::TestFixture {
-        CPPUNIT_TEST_SUITE(IDNConverterTest);
-        CPPUNIT_TEST(testStringPrep);
-        CPPUNIT_TEST(testStringPrep_Empty);
-        CPPUNIT_TEST(testGetEncoded);
-        CPPUNIT_TEST(testGetEncoded_International);
-        CPPUNIT_TEST(testGetEncoded_Invalid);
-        CPPUNIT_TEST_SUITE_END();
-
-    public:
-        void setUp() {
-            testling = std::shared_ptr<IDNConverter>(PlatformIDNConverter::create());
-        }
-
-        void testStringPrep() {
-            std::string result = testling->getStringPrepared("tron\xc3\x87on", IDNConverter::NamePrep);
-
-            CPPUNIT_ASSERT_EQUAL(std::string("tron\xc3\xa7on"), result);
-        }
-
-        void testStringPrep_Empty() {
-            CPPUNIT_ASSERT_EQUAL(std::string(""), testling->getStringPrepared("", IDNConverter::NamePrep));
-            CPPUNIT_ASSERT_EQUAL(std::string(""), testling->getStringPrepared("", IDNConverter::XMPPNodePrep));
-            CPPUNIT_ASSERT_EQUAL(std::string(""), testling->getStringPrepared("", IDNConverter::XMPPResourcePrep));
-        }
-
-        void testGetEncoded() {
-            boost::optional<std::string> result = testling->getIDNAEncoded("www.swift.im");
-            CPPUNIT_ASSERT(!!result);
-            CPPUNIT_ASSERT_EQUAL(std::string("www.swift.im"), *result);
-        }
-
-        void testGetEncoded_International() {
-            boost::optional<std::string> result = testling->getIDNAEncoded("www.tron\xc3\x87on.com");
-            CPPUNIT_ASSERT(!!result);
-            CPPUNIT_ASSERT_EQUAL(std::string("www.xn--tronon-zua.com"), *result);
-        }
-
-        void testGetEncoded_Invalid() {
-            boost::optional<std::string> result = testling->getIDNAEncoded("www.foo,bar.com");
-            CPPUNIT_ASSERT(!result);
-        }
-
-    private:
-        std::shared_ptr<IDNConverter> testling;
+class IDNConverterTest : public ::testing::Test {
+
+protected:
+    virtual void SetUp() {
+        testling_ = std::shared_ptr<IDNConverter>(PlatformIDNConverter::create());
+    }
+
+    std::shared_ptr<IDNConverter> testling_;
 };
 
-CPPUNIT_TEST_SUITE_REGISTRATION(IDNConverterTest);
+TEST_F(IDNConverterTest, testStringPrep) {
+    std::string result = testling_->getStringPrepared("tron\xc3\x87on", IDNConverter::NamePrep);
+
+    ASSERT_EQ(std::string("tron\xc3\xa7on"), result);
+}
+
+TEST_F(IDNConverterTest, testStringPrep_Empty) {
+    ASSERT_EQ(std::string(""), testling_->getStringPrepared("", IDNConverter::NamePrep));
+    ASSERT_EQ(std::string(""), testling_->getStringPrepared("", IDNConverter::XMPPNodePrep));
+    ASSERT_EQ(std::string(""), testling_->getStringPrepared("", IDNConverter::XMPPResourcePrep));
+}
+
+TEST_F(IDNConverterTest, testStringPrep_MaximumOutputSize) {
+    const std::string input(1023, 'x');
+    ASSERT_EQ(input, testling_->getStringPrepared(input, IDNConverter::NamePrep));
+    ASSERT_EQ(input, testling_->getStringPrepared(input, IDNConverter::XMPPNodePrep));
+    ASSERT_EQ(input, testling_->getStringPrepared(input, IDNConverter::XMPPResourcePrep));
+}
+
+TEST_F(IDNConverterTest, testStringPrep_TooLong) {
+    const std::string input(1024, 'x');
+    ASSERT_THROW(testling_->getStringPrepared(input, IDNConverter::NamePrep), std::exception);
+    ASSERT_THROW(testling_->getStringPrepared(input, IDNConverter::XMPPNodePrep), std::exception);
+    ASSERT_THROW(testling_->getStringPrepared(input, IDNConverter::XMPPResourcePrep), std::exception);
+}
+
+TEST_F(IDNConverterTest, testStringPrep_ShrinkingBelow1023) {
+    std::string input;
+    std::string expected;
+    // The four byte \u03b1\u0313 UTF-8 string will shrink to the three byte \u1f00
+    for (auto i = 0; i < 300; ++i) {
+        input +="\xce\xb1\xcc\x93"; // UTF-8 repesentation of U+03B1 U+0313
+        expected += "\xe1\xbc\x80"; // UTF-8 representation of U+1F00
+    }
+    ASSERT_EQ(expected, testling_->getStringPrepared(input, IDNConverter::NamePrep));
+    ASSERT_EQ(expected, testling_->getStringPrepared(input, IDNConverter::XMPPNodePrep));
+    ASSERT_EQ(expected, testling_->getStringPrepared(input, IDNConverter::XMPPResourcePrep));
+}
+
+TEST_F(IDNConverterTest, testGetEncoded) {
+    boost::optional<std::string> result = testling_->getIDNAEncoded("www.swift.im");
+    ASSERT_TRUE(!!result);
+    ASSERT_EQ(std::string("www.swift.im"), *result);
+}
+
+TEST_F(IDNConverterTest, testGetEncoded_International) {
+    boost::optional<std::string> result = testling_->getIDNAEncoded("www.tron\xc3\x87on.com");
+    ASSERT_TRUE(result);
+    ASSERT_EQ(std::string("www.xn--tronon-zua.com"), *result);
+}
+
+TEST_F(IDNConverterTest, testGetEncoded_Invalid) {
+    boost::optional<std::string> result = testling_->getIDNAEncoded("www.foo,bar.com");
+    ASSERT_FALSE(result);
+}
-- 
cgit v0.10.2-6-g49f6