Swift
summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorEdwin Mons <edwin.mons@isode.com>2018-10-24 13:37:33 (GMT)
committerEdwin Mons <edwin.mons@isode.com>2018-10-24 16:15:21 (GMT)
commit1c71c5a77e037038c581a68774c96fad9a79030b (patch)
tree3ee82bd0d84aa1d4c33d69948ca10952bda9cd35
parent0f4a77303fedfaa57977d6ca528799305eac9367 (diff)
downloadswift-1c71c5a77e037038c581a68774c96fad9a79030b.zip
swift-1c71c5a77e037038c581a68774c96fad9a79030b.tar.bz2
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
Diffstat
-rw-r--r--Swiften/IDN/LibIDNConverter.cpp7
-rw-r--r--Swiften/IDN/SConscript1
-rw-r--r--Swiften/IDN/UnitTest/IDNConverterTest.cpp114
3 files changed, 70 insertions, 52 deletions
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);
+}