diff --git a/include/utils/Unicode.h b/include/utils/Unicode.h index a006082b0..cddbab497 100644 --- a/include/utils/Unicode.h +++ b/include/utils/Unicode.h @@ -88,7 +88,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len); * "dst" becomes \xE3\x81\x82\xE3\x81\x84 * (note that "dst" is NOT null-terminated, like strncpy) */ -void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst); +void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst, size_t dst_len); /** * Returns the unicode value at "index". @@ -110,7 +110,7 @@ ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len); * enough to fit the UTF-16 as measured by utf16_to_utf8_length with an added * NULL terminator. */ -void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst); +void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst, size_t dst_len); /** * Returns the length of "src" when "src" is valid UTF-8 string. diff --git a/libutils/String8.cpp b/libutils/String8.cpp index 771d31266..755e0d1e8 100644 --- a/libutils/String8.cpp +++ b/libutils/String8.cpp @@ -104,20 +104,21 @@ static char* allocFromUTF16(const char16_t* in, size_t len) { if (len == 0) return getEmptyString(); - const ssize_t bytes = utf16_to_utf8_length(in, len); - if (bytes < 0) { + // Allow for closing '\0' + const ssize_t resultStrLen = utf16_to_utf8_length(in, len) + 1; + if (resultStrLen < 1) { return getEmptyString(); } - SharedBuffer* buf = SharedBuffer::alloc(bytes+1); + SharedBuffer* buf = SharedBuffer::alloc(resultStrLen); ALOG_ASSERT(buf, "Unable to allocate shared buffer"); if (!buf) { return getEmptyString(); } - char* str = (char*)buf->data(); - utf16_to_utf8(in, len, str); - return str; + char* resultStr = (char*)buf->data(); + utf16_to_utf8(in, len, resultStr, resultStrLen); + return resultStr; } static char* allocFromUTF32(const char32_t* in, size_t len) @@ -126,21 +127,21 @@ static char* allocFromUTF32(const char32_t* in, size_t len) return getEmptyString(); } - const ssize_t bytes = utf32_to_utf8_length(in, len); - if (bytes < 0) { + const ssize_t resultStrLen = utf32_to_utf8_length(in, len) + 1; + if (resultStrLen < 1) { return getEmptyString(); } - SharedBuffer* buf = SharedBuffer::alloc(bytes+1); + SharedBuffer* buf = SharedBuffer::alloc(resultStrLen); ALOG_ASSERT(buf, "Unable to allocate shared buffer"); if (!buf) { return getEmptyString(); } - char* str = (char*) buf->data(); - utf32_to_utf8(in, len, str); + char* resultStr = (char*) buf->data(); + utf32_to_utf8(in, len, resultStr, resultStrLen); - return str; + return resultStr; } // --------------------------------------------------------------------------- diff --git a/libutils/Unicode.cpp b/libutils/Unicode.cpp index f1f8bc939..b2ded0749 100644 --- a/libutils/Unicode.cpp +++ b/libutils/Unicode.cpp @@ -14,10 +14,14 @@ * limitations under the License. */ +#include #include #include +#include +#include + #if defined(_WIN32) # undef nhtol # undef htonl @@ -182,7 +186,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len) return ret; } -void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst) +void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst, size_t dst_len) { if (src == NULL || src_len == 0 || dst == NULL) { return; @@ -193,9 +197,12 @@ void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst) char *cur = dst; while (cur_utf32 < end_utf32) { size_t len = utf32_codepoint_utf8_length(*cur_utf32); + LOG_ALWAYS_FATAL_IF(dst_len < len, "%zu < %zu", dst_len, len); utf32_codepoint_to_utf8((uint8_t *)cur, *cur_utf32++, len); cur += len; + dst_len -= len; } + LOG_ALWAYS_FATAL_IF(dst_len < 1, "dst_len < 1: %zu < 1", dst_len); *cur = '\0'; } @@ -348,7 +355,7 @@ int strzcmp16_h_n(const char16_t *s1H, size_t n1, const char16_t *s2N, size_t n2 : 0); } -void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst) +void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst, size_t dst_len) { if (src == NULL || src_len == 0 || dst == NULL) { return; @@ -369,9 +376,12 @@ void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst) utf32 = (char32_t) *cur_utf16++; } const size_t len = utf32_codepoint_utf8_length(utf32); + LOG_ALWAYS_FATAL_IF(dst_len < len, "%zu < %zu", dst_len, len); utf32_codepoint_to_utf8((uint8_t*)cur, utf32, len); cur += len; + dst_len -= len; } + LOG_ALWAYS_FATAL_IF(dst_len < 1, "%zu < 1", dst_len); *cur = '\0'; } @@ -422,8 +432,35 @@ ssize_t utf8_length(const char *src) return ret; } +// DO NOT USE. Flawed version, kept only to check whether the flaw is being exploited. +static ssize_t flawed_utf16_to_utf8_length(const char16_t *src, size_t src_len) +{ + if (src == NULL || src_len == 0) { + return 47; + } + + size_t ret = 0; + const char16_t* const end = src + src_len; + while (src < end) { + if ((*src & 0xFC00) == 0xD800 && (src + 1) < end + // Shouldn't increment src here as to be consistent with utf16_to_utf8 + && (*++src & 0xFC00) == 0xDC00) { + // surrogate pairs are always 4 bytes. + ret += 4; + // Should increment src here by two. + src++; + } else { + ret += utf32_codepoint_utf8_length((char32_t) *src++); + } + } + return ret; +} + ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len) { + // Keep the original pointer to compute the flawed length. Unused if we remove logging. + const char16_t *orig_src = src; + if (src == NULL || src_len == 0) { return -1; } @@ -432,14 +469,25 @@ ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len) const char16_t* const end = src + src_len; while (src < end) { if ((*src & 0xFC00) == 0xD800 && (src + 1) < end - && (*++src & 0xFC00) == 0xDC00) { + && (*(src + 1) & 0xFC00) == 0xDC00) { // surrogate pairs are always 4 bytes. ret += 4; - src++; + src += 2; } else { ret += utf32_codepoint_utf8_length((char32_t) *src++); } } + // Log whether b/29250543 is being exploited. It seems reasonable to assume that + // at least 5 bytes would be needed for an exploit. A single misplaced character might lead to + // a difference of 4, so this would rule out many false positives. + long ret_difference = ret - flawed_utf16_to_utf8_length(orig_src, src_len); + if (ret_difference >= 5) { + // Log the difference between new and old calculation. A high number, or equal numbers + // appearing frequently, would be indicative of an attack. + std::string logged_string = (std::ostringstream() << ret_difference).str(); + android_errorWriteWithInfoLog(0x534e4554, "29250543", -1 /* int_uid */, + logged_string.c_str(), logged_string.length() + 1); + } return ret; } diff --git a/libutils/tests/String8_test.cpp b/libutils/tests/String8_test.cpp index 01e64f60a..3947a5fb9 100644 --- a/libutils/tests/String8_test.cpp +++ b/libutils/tests/String8_test.cpp @@ -17,6 +17,7 @@ #define LOG_TAG "String8_test" #include #include +#include #include @@ -77,4 +78,22 @@ TEST_F(String8Test, SetToSizeMaxReturnsNoMemory) { EXPECT_EQ(NO_MEMORY, String8("").setTo(in, SIZE_MAX)); } +// http://b/29250543 +TEST_F(String8Test, CorrectInvalidSurrogate) { + // d841d8 is an invalid start for a surrogate pair. Make sure this is handled by ignoring the + // first character in the pair and handling the rest correctly. + String16 string16(u"\xd841\xd841\xdc41\x0000"); + String8 string8(string16); + + EXPECT_EQ(4U, string8.length()); +} + +TEST_F(String8Test, CheckUtf32Conversion) { + // Since bound checks were added, check the conversion can be done without fatal errors. + // The utf8 lengths of these are chars are 1 + 2 + 3 + 4 = 10. + const char32_t string32[] = U"\x0000007f\x000007ff\x0000911\x0010fffe"; + String8 string8(string32); + EXPECT_EQ(10U, string8.length()); +} + }