Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c4966a36 authored by Sergio Giro's avatar Sergio Giro
Browse files

libutils/Unicode.cpp: Correct length computation and add checks for utf16->utf8 

Inconsistent behaviour between utf16_to_utf8 and utf16_to_utf8_length
is causing a heap overflow.

Correcting the length computation and adding bound checks to the
conversion functions.

Test: ran libutils_tests
Bug: 29250543
Change-Id: I6115e3357141ed245c63c6eb25fc0fd0a9a7a2bb
parent 34ae543d

include/utils/Unicode.h

+2 −2
Original line number Diff line number Diff line
@@ -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.

libutils/String8.cpp

+13 −12
Original line number Diff line number Diff line
@@ -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;

}



// ---------------------------------------------------------------------------

libutils/Unicode.cpp

+52 −4
Original line number Diff line number Diff line
@@ -14,10 +14,14 @@ 
 * limitations under the License.

 */



#include <log/log.h>

#include <utils/Unicode.h>



#include <stddef.h>



#include <string>

#include <sstream>



#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,19 +432,22 @@ ssize_t utf8_length(const char *src) 
    return ret;

}



ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len)

// 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 -1;

        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++);
@@ -443,6 +456,41 @@ ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len) 
    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;

    }



    size_t ret = 0;

    const char16_t* const end = src + src_len;

    while (src < end) {

        if ((*src & 0xFC00) == 0xD800 && (src + 1) < end

                && (*(src + 1) & 0xFC00) == 0xDC00) {

            // surrogate pairs are always 4 bytes.

            ret += 4;

            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;

}



/**

 * Returns 1-4 based on the number of leading bits.

 *

libutils/tests/String8_test.cpp

+19 −0
Original line number Diff line number Diff line
@@ -17,6 +17,7 @@ 
#define LOG_TAG "String8_test"

#include <utils/Log.h>

#include <utils/String8.h>

#include <utils/String16.h>



#include <gtest/gtest.h>
@@ -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());

}



}