CVE-2020-10531: Out-of-bounds Write (CWE-787)
value: An issue was discovered in International Components for Unicode (ICU) for C/C++ through 66.1. An integer overflow, leading to a heap-based buffer overflow, exists in the UnicodeString::doAppend() function in common/unistr.cpp.
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 8.8
Published: 2020-03-12T19:15Z
Repository: https://github.com/unicode-org/icu
Language: C++
Files changed: icu4c/source/common/unistr.cpp, icu4c/source/test/intltest/ustrtest.cpp, icu4c/source/test/intltest/ustrtest.h
Commit date: 2020-02-03 13:22:30 -0800
Commit message: ICU-20958 Prevent SEGV_MAPERR in append

See #971
Diff: diff --git a/icu4c/source/common/unistr.cpp b/icu4c/source/common/unistr.cpp
index 901bb3358b..077b4d6ef2 100644
--- a/icu4c/source/common/unistr.cpp
+++ b/icu4c/source/common/unistr.cpp
@@ -1561,11 +1561,15 @@ UnicodeString::doAppend(const UChar *srcChars, int32_t srcStart, int32_t srcLeng
       return *this;
     }
   }
 
   int32_t oldLength = length();
-  int32_t newLength = oldLength + srcLength;
+  int32_t newLength;
+  if (uprv_add32_overflow(oldLength, srcLength, &newLength)) {
+    setToBogus();
+    return *this;
+  }
 
   // Check for append onto ourself
   const UChar* oldArray = getArrayStart();
   if (isBufferWritable() &&
       oldArray < srcChars + srcLength &&
diff --git a/icu4c/source/test/intltest/ustrtest.cpp b/icu4c/source/test/intltest/ustrtest.cpp
index b6515ea813..ad38bdf53a 100644
--- a/icu4c/source/test/intltest/ustrtest.cpp
+++ b/icu4c/source/test/intltest/ustrtest.cpp
@@ -65,10 +65,11 @@ void UnicodeStringTest::runIndexedTest( int32_t index, UBool exec, const char* &
     TESTCASE_AUTO(TestMoveSwap);
     TESTCASE_AUTO(TestUInt16Pointers);
     TESTCASE_AUTO(TestWCharPointers);
     TESTCASE_AUTO(TestNullPointers);
     TESTCASE_AUTO(TestUnicodeStringInsertAppendToSelf);
+    TESTCASE_AUTO(TestLargeAppend);
     TESTCASE_AUTO_END;
 }
 
 void
 UnicodeStringTest::TestBasicManipulation()
@@ -2308,5 +2309,66 @@ void UnicodeStringTest::TestUnicodeStringInsertAppendToSelf() {
     str = UnicodeString(TRUE, u"abcde", 5);
     sub = str.tempSubString(1, 3);
     str.insert(2, sub);
     assertEquals("", u"abbcdcde", str);
 }
+
+void UnicodeStringTest::TestLargeAppend() {
+    if(quick) return;
+
+    IcuTestErrorCode status(*this, "TestLargeAppend");
+    // Make a large UnicodeString
+    int32_t len = 0xAFFFFFF;
+    UnicodeString str;
+    char16_t *buf = str.getBuffer(len);
+    // A fast way to set buffer to valid Unicode.
+    // 4E4E is a valid unicode character
+    uprv_memset(buf, 0x4e, len * 2);
+    str.releaseBuffer(len);
+    UnicodeString dest;
+    // Append it 16 times
+    // 0xAFFFFFF times 16 is 0xA4FFFFF1,
+    // which is greater than INT32_MAX, which is 0x7FFFFFFF.
+    int64_t total = 0;
+    for (int32_t i = 0; i < 16; i++) {
+        dest.append(str);
+        total += len;
+        if (total <= INT32_MAX) {
+            assertFalse("dest is not bogus", dest.isBogus());
+        } else {
+            assertTrue("dest should be bogus", dest.isBogus());
+        }
+    }
+    dest.remove();
+    total = 0;
+    for (int32_t i = 0; i < 16; i++) {
+        dest.append(str);
+        total += len;
+        if (total + len <= INT32_MAX) {
+            assertFalse("dest is not bogus", dest.isBogus());
+        } else if (total <= INT32_MAX) {
+            // Check that a string of exactly the maximum size works
+            UnicodeString str2;
+            int32_t remain = INT32_MAX - total;
+            char16_t *buf2 = str2.getBuffer(remain);
+            if (buf2 == nullptr) {
+                // if somehow memory allocation fail, return the test
+                return;
+            }
+            uprv_memset(buf2, 0x4e, remain * 2);
+            str2.releaseBuffer(remain);
+            dest.append(str2);
+            total += remain;
+            assertEquals("When a string of exactly the maximum size works", (int64_t)INT32_MAX, total);
+            assertEquals("When a string of exactly the maximum size works", INT32_MAX, dest.length());
+            assertFalse("dest is not bogus", dest.isBogus());
+
+            // Check that a string size+1 goes bogus
+            str2.truncate(1);
+            dest.append(str2);
+            total++;
+            assertTrue("dest should be bogus", dest.isBogus());
+        } else {
+            assertTrue("dest should be bogus", dest.isBogus());
+        }
+    }
+}
diff --git a/icu4c/source/test/intltest/ustrtest.h b/icu4c/source/test/intltest/ustrtest.h
index 218befdcc6..4a356a92c7 100644
--- a/icu4c/source/test/intltest/ustrtest.h
+++ b/icu4c/source/test/intltest/ustrtest.h
@@ -95,8 +95,9 @@ class UnicodeStringTest: public IntlTest {
 
     void TestUInt16Pointers();
     void TestWCharPointers();
     void TestNullPointers();
     void TestUnicodeStringInsertAppendToSelf();
+    void TestLargeAppend();
 };
 
 #endif

Vulnerable code:   int32_t newLength = oldLength + srcLength;
Fixed code:   int32_t newLength;
  if (uprv_add32_overflow(oldLength, srcLength, &newLength)) {
    setToBogus();
    return *this;
  }
    TESTCASE_AUTO(TestLargeAppend);

void UnicodeStringTest::TestLargeAppend() {
    if(quick) return;

    IcuTestErrorCode status(*this, "TestLargeAppend");
    // Make a large UnicodeString
    int32_t len = 0xAFFFFFF;
    UnicodeString str;
    char16_t *buf = str.getBuffer(len);
    // A fast way to set buffer to valid Unicode.
    // 4E4E is a valid unicode character
    uprv_memset(buf, 0x4e, len * 2);
    str.releaseBuffer(len);
    UnicodeString dest;
    // Append it 16 times
    // 0xAFFFFFF times 16 is 0xA4FFFFF1,
    // which is greater than INT32_MAX, which is 0x7FFFFFFF.
    int64_t total = 0;
    for (int32_t i = 0; i < 16; i++) {
        dest.append(str);
        total += len;
        if (total <= INT32_MAX) {
            assertFalse("dest is not bogus", dest.isBogus());
        } else {
            assertTrue("dest should be bogus", dest.isBogus());
        }
    }
    dest.remove();
    total = 0;
    for (int32_t i = 0; i < 16; i++) {
        dest.append(str);
        total += len;
        if (total + len <= INT32_MAX) {
            assertFalse("dest is not bogus", dest.isBogus());
        } else if (total <= INT32_MAX) {
            // Check that a string of exactly the maximum size works
            UnicodeString str2;
            int32_t remain = INT32_MAX - total;
            char16_t *buf2 = str2.getBuffer(remain);
            if (buf2 == nullptr) {
                // if somehow memory allocation fail, return the test
                return;
            }
            uprv_memset(buf2, 0x4e, remain * 2);
            str2.releaseBuffer(remain);
            dest.append(str2);
            total += remain;
            assertEquals("When a string of exactly the maximum size works", (int64_t)INT32_MAX, total);
            assertEquals("When a string of exactly the maximum size works", INT32_MAX, dest.length());
            assertFalse("dest is not bogus", dest.isBogus());

            // Check that a string size+1 goes bogus
            str2.truncate(1);
            dest.append(str2);
            total++;
            assertTrue("dest should be bogus", dest.isBogus());
        } else {
            assertTrue("dest should be bogus", dest.isBogus());
        }
    }
}
    void TestLargeAppend();