commit 513535dbf339c5c5dd38b77809c6a88969c05109 Author: Tomas Korbar Date: Mon Feb 21 13:42:02 2022 +0100 CVE-2022-23852 diff --git a/lib/xmlparse.c b/lib/xmlparse.c index 2ebd80f..d4f30b7 100644 --- a/lib/xmlparse.c +++ b/lib/xmlparse.c @@ -75,6 +75,9 @@ typedef char ICHAR; /* Round up n to be a multiple of sz, where sz is a power of 2. */ #define ROUND_UP(n, sz) (((n) + ((sz) - 1)) & ~((sz) - 1)) +/* Do safe (NULL-aware) pointer arithmetic */ +#define EXPAT_SAFE_PTR_DIFF(p, q) (((p) && (q)) ? ((p) - (q)) : 0) + /* Handle the case where memmove() doesn't exist. */ #ifndef HAVE_MEMMOVE #ifdef HAVE_BCOPY @@ -1678,50 +1681,70 @@ XML_ParseBuffer(XML_Parser parser, int len, int isFinal) void * XMLCALL XML_GetBuffer(XML_Parser parser, int len) { + if (parser == NULL) + return NULL; if (len < 0) { - errorCode = XML_ERROR_NO_MEMORY; + parser->m_errorCode = XML_ERROR_NO_MEMORY; return NULL; } - switch (ps_parsing) { + switch (parser->m_parsingStatus.parsing) { case XML_SUSPENDED: - errorCode = XML_ERROR_SUSPENDED; + parser->m_errorCode = XML_ERROR_SUSPENDED; return NULL; case XML_FINISHED: - errorCode = XML_ERROR_FINISHED; + parser->m_errorCode = XML_ERROR_FINISHED; return NULL; default: ; } - if (len > bufferLim - bufferEnd) { - int neededSize = (int) ((unsigned)len + (unsigned)(bufferEnd - bufferPtr)); + if (len > EXPAT_SAFE_PTR_DIFF(parser->m_bufferLim, parser->m_bufferEnd)) { + int keep; + /* Do not invoke signed arithmetic overflow: */ + int neededSize = (int)((unsigned)len + + (unsigned)EXPAT_SAFE_PTR_DIFF( + parser->m_bufferEnd, parser->m_bufferPtr)); if (neededSize < 0) { - errorCode = XML_ERROR_NO_MEMORY; + parser->m_errorCode = XML_ERROR_NO_MEMORY; return NULL; } -#ifdef XML_CONTEXT_BYTES - int keep = (int)(bufferPtr - buffer); + keep = (int)EXPAT_SAFE_PTR_DIFF(parser->m_bufferPtr, parser->m_buffer); if (keep > XML_CONTEXT_BYTES) keep = XML_CONTEXT_BYTES; + /* Detect and prevent integer overflow */ + if (keep > INT_MAX - neededSize) { + parser->m_errorCode = XML_ERROR_NO_MEMORY; + return NULL; + } neededSize += keep; -#endif /* defined XML_CONTEXT_BYTES */ - if (neededSize <= bufferLim - buffer) { + if (neededSize + <= EXPAT_SAFE_PTR_DIFF(parser->m_bufferLim, parser->m_buffer)) { #ifdef XML_CONTEXT_BYTES - if (keep < bufferPtr - buffer) { - int offset = (int)(bufferPtr - buffer) - keep; - memmove(buffer, &buffer[offset], bufferEnd - bufferPtr + keep); - bufferEnd -= offset; - bufferPtr -= offset; + if (keep < EXPAT_SAFE_PTR_DIFF(parser->m_bufferPtr, parser->m_buffer)) { + int offset + = (int)EXPAT_SAFE_PTR_DIFF(parser->m_bufferPtr, parser->m_buffer) + - keep; + /* The buffer pointers cannot be NULL here; we have at least some bytes + * in the buffer */ + memmove(parser->m_buffer, &parser->m_buffer[offset], + parser->m_bufferEnd - parser->m_bufferPtr + keep); + parser->m_bufferEnd -= offset; + parser->m_bufferPtr -= offset; } #else - memmove(buffer, bufferPtr, bufferEnd - bufferPtr); - bufferEnd = buffer + (bufferEnd - bufferPtr); - bufferPtr = buffer; -#endif /* not defined XML_CONTEXT_BYTES */ - } - else { + if (parser->m_buffer && parser->m_bufferPtr) { + memmove(parser->m_buffer, parser->m_bufferPtr, + EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr)); + parser->m_bufferEnd + = parser->m_buffer + + EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr); + parser->m_bufferPtr = parser->m_buffer; + } +#endif /* not defined XML_CONTEXT_BYTES */ + } else { char *newBuf; - int bufferSize = (int)(bufferLim - bufferPtr); + int bufferSize + = (int)EXPAT_SAFE_PTR_DIFF(parser->m_bufferLim, parser->m_bufferPtr); if (bufferSize == 0) bufferSize = INIT_BUFFER_SIZE; do { @@ -1729,43 +1752,51 @@ XML_GetBuffer(XML_Parser parser, int len) bufferSize = (int) (2U * (unsigned) bufferSize); } while (bufferSize < neededSize && bufferSize > 0); if (bufferSize <= 0) { - errorCode = XML_ERROR_NO_MEMORY; + parser->m_errorCode = XML_ERROR_NO_MEMORY; return NULL; } newBuf = (char *)MALLOC(bufferSize); if (newBuf == 0) { - errorCode = XML_ERROR_NO_MEMORY; + parser->m_errorCode = XML_ERROR_NO_MEMORY; return NULL; } - bufferLim = newBuf + bufferSize; + parser->m_bufferLim = newBuf + bufferSize; #ifdef XML_CONTEXT_BYTES - if (bufferPtr) { - int keep = (int)(bufferPtr - buffer); - if (keep > XML_CONTEXT_BYTES) - keep = XML_CONTEXT_BYTES; - memcpy(newBuf, &bufferPtr[-keep], bufferEnd - bufferPtr + keep); - FREE(buffer); - buffer = newBuf; - bufferEnd = buffer + (bufferEnd - bufferPtr) + keep; - bufferPtr = buffer + keep; - } - else { - bufferEnd = newBuf + (bufferEnd - bufferPtr); - bufferPtr = buffer = newBuf; + if (parser->m_bufferPtr) { + memcpy(newBuf, &parser->m_bufferPtr[-keep], + EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr) + + keep); + FREE(parser->m_buffer); + parser->m_buffer = newBuf; + parser->m_bufferEnd + = parser->m_buffer + + EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr) + + keep; + parser->m_bufferPtr = parser->m_buffer + keep; + } else { + /* This must be a brand new buffer with no data in it yet */ + parser->m_bufferEnd = newBuf; + parser->m_bufferPtr = parser->m_buffer = newBuf; } #else - if (bufferPtr) { - memcpy(newBuf, bufferPtr, bufferEnd - bufferPtr); - FREE(buffer); + if (parser->m_bufferPtr) { + memcpy(newBuf, parser->m_bufferPtr, + EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr)); + FREE(parser->m_buffer); + parser->m_bufferEnd + = newBuf + + EXPAT_SAFE_PTR_DIFF(parser->m_bufferEnd, parser->m_bufferPtr); + } else { + /* This must be a brand new buffer with no data in it yet */ + parser->m_bufferEnd = newBuf; } - bufferEnd = newBuf + (bufferEnd - bufferPtr); - bufferPtr = buffer = newBuf; + parser->m_bufferPtr = parser->m_buffer = newBuf; #endif /* not defined XML_CONTEXT_BYTES */ } - eventPtr = eventEndPtr = NULL; - positionPtr = NULL; + parser->m_eventPtr = parser->m_eventEndPtr = NULL; + parser->m_positionPtr = NULL; } - return bufferEnd; + return parser->m_bufferEnd; } enum XML_Status XMLCALL diff --git a/tests/runtests.c b/tests/runtests.c index 233956e..b99e375 100644 --- a/tests/runtests.c +++ b/tests/runtests.c @@ -13,6 +13,7 @@ #include #include #include +#include #include "expat.h" #include "chardata.h" @@ -149,6 +150,30 @@ dummy_start_element(void *userData, {} +/* Test for signed integer overflow CVE-2022-23852 */ +#if defined(XML_CONTEXT_BYTES) +START_TEST(test_get_buffer_3_overflow) { + XML_Parser parser = XML_ParserCreate(NULL); + assert(parser != NULL); + + const char *const text = "\n"; + const int expectedKeepValue = (int)strlen(text); + + // After this call, variable "keep" in XML_GetBuffer will + // have value expectedKeepValue + if (XML_Parse(parser, text, (int)strlen(text), XML_FALSE /* isFinal */) + == XML_STATUS_ERROR) + xml_failure(parser); + + assert(expectedKeepValue > 0); + if (XML_GetBuffer(parser, INT_MAX - expectedKeepValue + 1) != NULL) + fail("enlarging buffer not failed"); + + XML_ParserFree(parser); +} +END_TEST +#endif // defined(XML_CONTEXT_BYTES) + /* * Character & encoding tests. */ @@ -1483,6 +1508,9 @@ make_suite(void) suite_add_tcase(s, tc_basic); tcase_add_checked_fixture(tc_basic, basic_setup, basic_teardown); +#if defined(XML_CONTEXT_BYTES) + tcase_add_test(tc_basic, test_get_buffer_3_overflow); +#endif tcase_add_test(tc_basic, test_nul_byte); tcase_add_test(tc_basic, test_u0000_char); tcase_add_test(tc_basic, test_bom_utf8);