From 494735de30c946bc7d684c65ff8ece05beeb232d Mon Sep 17 00:00:00 2001 From: Lee Thomason Date: Wed, 4 Dec 2024 18:07:15 -0800 Subject: [PATCH 1/2] fix the potential overflow in char refs --- tinyxml2.cpp | 28 +++++++++++++++------------- xmltest.cpp | 31 ++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/tinyxml2.cpp b/tinyxml2.cpp index 1605999..ad4f62c 100644 --- a/tinyxml2.cpp +++ b/tinyxml2.cpp @@ -472,11 +472,12 @@ const char* XMLUtil::GetCharacterRef( const char* p, char* value, int* length ) // Presume an entity, and pull it out. *length = 0; + static const uint32_t MAX_CODE_POINT = 0x10FFFF; + if ( *(p+1) == '#' && *(p+2) ) { - unsigned long ucs = 0; - TIXMLASSERT( sizeof( ucs ) >= 4 ); + uint32_t ucs = 0; ptrdiff_t delta = 0; - unsigned mult = 1; + uint32_t mult = 1; static const char SEMICOLON = ';'; if ( *(p+2) == 'x' ) { @@ -487,7 +488,6 @@ const char* XMLUtil::GetCharacterRef( const char* p, char* value, int* length ) } q = strchr( q, SEMICOLON ); - if ( !q ) { return 0; } @@ -497,7 +497,7 @@ const char* XMLUtil::GetCharacterRef( const char* p, char* value, int* length ) --q; while ( *q != 'x' ) { - unsigned int digit = 0; + uint32_t digit = 0; if ( *q >= '0' && *q <= '9' ) { digit = *q - '0'; @@ -512,11 +512,12 @@ const char* XMLUtil::GetCharacterRef( const char* p, char* value, int* length ) return 0; } TIXMLASSERT( digit < 16 ); - TIXMLASSERT( digit == 0 || mult <= UINT_MAX / digit ); const unsigned int digitScaled = mult * digit; - TIXMLASSERT( ucs <= ULONG_MAX - digitScaled ); ucs += digitScaled; - TIXMLASSERT( mult <= UINT_MAX / 16 ); + if (ucs > MAX_CODE_POINT) { + return 0; + } + mult *= 16; --q; } @@ -540,22 +541,23 @@ const char* XMLUtil::GetCharacterRef( const char* p, char* value, int* length ) while ( *q != '#' ) { if ( *q >= '0' && *q <= '9' ) { - const unsigned int digit = *q - '0'; + const uint32_t digit = *q - '0'; TIXMLASSERT( digit < 10 ); - TIXMLASSERT( digit == 0 || mult <= UINT_MAX / digit ); - const unsigned int digitScaled = mult * digit; - TIXMLASSERT( ucs <= ULONG_MAX - digitScaled ); + const uint32_t digitScaled = mult * digit; ucs += digitScaled; + if (ucs > MAX_CODE_POINT) { + return 0; + } } else { return 0; } - TIXMLASSERT( mult <= UINT_MAX / 10 ); mult *= 10; --q; } } // convert the UCS to UTF-8 + TIXMLASSERT(ucs <= MAX_CODE_POINT); ConvertUTF32ToUTF8( ucs, value, length ); return p + delta + 1; } diff --git a/xmltest.cpp b/xmltest.cpp index ae97604..6a33c8d 100644 --- a/xmltest.cpp +++ b/xmltest.cpp @@ -1642,7 +1642,7 @@ int main( int argc, const char ** argv ) static const char* result = "\xef\xbb\xbf"; XMLTest( "BOM and default declaration", result, printer.CStr(), false ); - XMLTest( "CStrSize", 42, printer.CStrSize(), false ); + XMLTest( "CStrSize", true, printer.CStrSize() == 42, false ); } { const char* xml = "Text"; + XMLDocument doc; + doc.Parse(xml); + const char* value = doc.FirstChildElement()->Attribute("value"); + const char* value2 = doc.FirstChildElement()->Attribute("value2"); + XMLTest("Test attribute encode", false, doc.Error()); + XMLTest("Test decimal value", value, "12A34"); + XMLTest("Test hex encode", value2, "56B78"); + } + + { + const char* xml = "Text"; + XMLDocument doc; + doc.Parse(xml); + const char* value = doc.FirstChildElement()->Attribute("value"); + const char* value2 = doc.FirstChildElement()->Attribute("value2"); + const char* value3 = doc.FirstChildElement()->Attribute("value3"); + const char* value4 = doc.FirstChildElement()->Attribute("value4"); + const char* value5 = doc.FirstChildElement()->Attribute("value5"); + XMLTest("Test attribute encode", false, doc.Error()); + XMLTest("Test attribute encode too long value", value, "&#ABC9000000065;"); // test long value + XMLTest("Test attribute encode out of unicode range", value2, "�"); // out of unicode range + XMLTest("Test attribute encode out of int max value", value3, "�"); // out of int max value + XMLTest("Test attribute encode with a Hex value", value4, "E"); // hex value in unicode value + XMLTest("Test attribute encode with a Hex value", value5, "!"); // hex value in unicode value + } + // ----------- Performance tracking -------------- { #if defined( _MSC_VER ) From ff48ea1baeafa82835bc211c261e0b7979080248 Mon Sep 17 00:00:00 2001 From: Lee Thomason Date: Mon, 9 Dec 2024 21:17:33 -0800 Subject: [PATCH 2/2] improve fix --- tinyxml2.cpp | 131 +++++++++++++++++++++++---------------------------- 1 file changed, 58 insertions(+), 73 deletions(-) diff --git a/tinyxml2.cpp b/tinyxml2.cpp index ad4f62c..b5e85aa 100644 --- a/tinyxml2.cpp +++ b/tinyxml2.cpp @@ -467,104 +467,89 @@ void XMLUtil::ConvertUTF32ToUTF8( unsigned long input, char* output, int* length } -const char* XMLUtil::GetCharacterRef( const char* p, char* value, int* length ) +const char* XMLUtil::GetCharacterRef(const char* p, char* value, int* length) { - // Presume an entity, and pull it out. + // Assume an entity, and pull it out. *length = 0; - static const uint32_t MAX_CODE_POINT = 0x10FFFF; + static const uint32_t MAX_CODE_POINT = 0x10FFFF; - if ( *(p+1) == '#' && *(p+2) ) { + if (*(p + 1) == '#' && *(p + 2)) { uint32_t ucs = 0; ptrdiff_t delta = 0; uint32_t mult = 1; static const char SEMICOLON = ';'; - if ( *(p+2) == 'x' ) { + bool hex = false; + uint32_t radix = 10; + const char* q = 0; + char terminator = '#'; + + if (*(p + 2) == 'x') { // Hexadecimal. - const char* q = p+3; - if ( !(*q) ) { - return 0; - } + hex = true; + radix = 16; + terminator = 'x'; - q = strchr( q, SEMICOLON ); - if ( !q ) { - return 0; - } - TIXMLASSERT( *q == SEMICOLON ); - - delta = q-p; - --q; - - while ( *q != 'x' ) { - uint32_t digit = 0; - - if ( *q >= '0' && *q <= '9' ) { - digit = *q - '0'; - } - else if ( *q >= 'a' && *q <= 'f' ) { - digit = *q - 'a' + 10; - } - else if ( *q >= 'A' && *q <= 'F' ) { - digit = *q - 'A' + 10; - } - else { - return 0; - } - TIXMLASSERT( digit < 16 ); - const unsigned int digitScaled = mult * digit; - ucs += digitScaled; - if (ucs > MAX_CODE_POINT) { - return 0; - } - - mult *= 16; - --q; - } + q = p + 3; } else { // Decimal. - const char* q = p+2; - if ( !(*q) ) { + q = p + 2; + } + if (!(*q)) { + return 0; + } + + q = strchr(q, SEMICOLON); + if (!q) { + return 0; + } + TIXMLASSERT(*q == SEMICOLON); + + delta = q - p; + --q; + + while (*q != terminator) { + uint32_t digit = 0; + + if (*q >= '0' && *q <= '9') { + digit = *q - '0'; + } + else if (hex && (*q >= 'a' && *q <= 'f')) { + digit = *q - 'a' + 10; + } + else if (hex && (*q >= 'A' && *q <= 'F')) { + digit = *q - 'A' + 10; + } + else { return 0; } + TIXMLASSERT(digit < radix); - q = strchr( q, SEMICOLON ); - - if ( !q ) { - return 0; + const unsigned int digitScaled = mult * digit; + ucs += digitScaled; + mult *= radix; + + // Security check: could a value exist that is out of range? + // Easily; limit to the MAX_CODE_POINT, which also allows for a + // bunch of leading zeroes. + if (mult > MAX_CODE_POINT) { + mult = MAX_CODE_POINT; } - TIXMLASSERT( *q == SEMICOLON ); - - delta = q-p; --q; - - while ( *q != '#' ) { - if ( *q >= '0' && *q <= '9' ) { - const uint32_t digit = *q - '0'; - TIXMLASSERT( digit < 10 ); - const uint32_t digitScaled = mult * digit; - ucs += digitScaled; - if (ucs > MAX_CODE_POINT) { - return 0; - } - } - else { - return 0; - } - mult *= 10; - --q; - } + } + // Out of range: + if (ucs > MAX_CODE_POINT) { + return 0; } // convert the UCS to UTF-8 - TIXMLASSERT(ucs <= MAX_CODE_POINT); - ConvertUTF32ToUTF8( ucs, value, length ); + ConvertUTF32ToUTF8(ucs, value, length); return p + delta + 1; } - return p+1; + return p + 1; } - void XMLUtil::ToStr( int v, char* buffer, int bufferSize ) { TIXML_SNPRINTF( buffer, bufferSize, "%d", v );