From 6d8259c43c7d675987d96bf990b6b57402d27f28 Mon Sep 17 00:00:00 2001 From: root Date: Wed, 19 Mar 2025 12:52:04 -0400 Subject: [PATCH 09/10] Restrict XML Entity Expansion Depth in libexpat CVE-2024-8176 The embedded libexpat library is vulnerable to a stack overflow due to uncontrolled recursion when processing deeply nested XML entities. This can cause the application to crash, resulting in a denial of service (DoS) or potentially leading to memory corruption, depending on the user's environment and how the library is used. The issue is triggered by supplying a specially crafted XML document designed to create a long chain of recursive entities. libexpat addressed this upstream in https://github.com/libexpat/libexpat/pull/973 but the embedded copy within xmlrpc-c is so old there is no chance of applying this without rebasing it. Instead a recursion counter is added to the parser to limit the depth. Resolves: RHEL-57536 --- lib/expat/xmlparse/xmlparse.c | 40 +++++++++++++--- test/Makefile | 30 ++++++++++-- test/overflow_attr_test.c | 88 +++++++++++++++++++++++++++++++++++ test/overflow_entity_test.c | 86 ++++++++++++++++++++++++++++++++++ 4 files changed, 234 insertions(+), 10 deletions(-) create mode 100644 test/overflow_attr_test.c create mode 100644 test/overflow_entity_test.c diff --git a/lib/expat/xmlparse/xmlparse.c b/lib/expat/xmlparse/xmlparse.c index c440ac7..467910a 100644 --- a/lib/expat/xmlparse/xmlparse.c +++ b/lib/expat/xmlparse/xmlparse.c @@ -29,6 +29,8 @@ See the file copying.txt for copying permission. #include "xmldef.h" #include "xmlparse.h" +#define MAX_RECURSE 28 + static const char * extractXmlSample(const char * const start, const char * const end, @@ -312,6 +314,7 @@ typedef struct { enum XML_ParamEntityParsing m_paramEntityParsing; XML_Parser m_parentParser; unsigned long m_hash_secret_salt; + int m_recurse_lock; } Parser; #define userData (((Parser *)parser)->m_userData) @@ -388,6 +391,7 @@ typedef struct { #define namespaceSeparator (((Parser *)parser)->m_namespaceSeparator) #define parentParser (((Parser *)parser)->m_parentParser) #define hash_secret_salt (((Parser *)parser)->m_hash_secret_salt) +#define recurse_lock (((Parser *)parser)->m_recurse_lock) @@ -515,7 +519,7 @@ poolGrow(STRING_POOL * const poolP, size_t const newSize = offsetof(BLOCK, s) + blockSize * sizeof(XML_Char); - BLOCK * const newBlocksP = malloc(newSize); + BLOCK * const newBlocksP = calloc(offsetof(BLOCK, s) + blockSize, sizeof(XML_Char)); if (newBlocksP) { newBlocksP->size = blockSize; @@ -1143,6 +1147,7 @@ const XML_Char *getContext(XML_Parser parser) { HASH_TABLE_ITER iter; int needSep = 0; + Parser * const parserP = (Parser *) parser; if (dtd.defaultPrefix.binding) { int i; @@ -1191,7 +1196,7 @@ const XML_Char *getContext(XML_Parser parser) ENTITY *e = (ENTITY *)hashTableIterNext(&iter); if (!e) break; - if (!e->open) + if (!e->open || parserP->m_recurse_lock > MAX_RECURSE) continue; if (needSep && !poolAppendChar(&tempPool, CONTEXT_SEP)) return 0; @@ -1210,6 +1215,7 @@ static int setContext(XML_Parser parser, const XML_Char *context) { const XML_Char *s = context; + Parser * const parserP = (Parser *) parser; while (*context != XML_T('\0')) { if (*s == CONTEXT_SEP || *s == XML_T('\0')) { @@ -1218,8 +1224,9 @@ int setContext(XML_Parser parser, const XML_Char *context) return 0; e = (ENTITY *)lookup(parser, &dtd.generalEntities, poolStart(&tempPool), 0); - if (e) + if (e) { e->open = 1; + } if (*s != XML_T('\0')) s++; context = s; @@ -1860,7 +1867,7 @@ appendAttributeValue(XML_Parser const xmlParserP, return XML_ERROR_UNDEFINED_ENTITY; } } - else if (entity->open) { + else if (entity->open || parser->m_recurse_lock > MAX_RECURSE) { if (enc == parser->m_encoding) eventPtr = ptr; return XML_ERROR_RECURSIVE_ENTITY_REF; @@ -1879,10 +1886,12 @@ appendAttributeValue(XML_Parser const xmlParserP, enum XML_Error result; const XML_Char *textEnd = entity->textPtr + entity->textLen; entity->open = 1; + parser->m_recurse_lock++; result = appendAttributeValue(xmlParserP, internalEncoding, isCdata, (char *)entity->textPtr, (char *)textEnd, pool); entity->open = 0; + parser->m_recurse_lock--; if (result) return result; } @@ -1952,7 +1961,7 @@ storeEntityValue(XML_Parser const xmlParserP, eventPtr = entityTextPtr; return XML_ERROR_UNDEFINED_ENTITY; } - if (entity->open) { + if (entity->open || parser->m_recurse_lock > MAX_RECURSE) { if (enc == parser->m_encoding) eventPtr = entityTextPtr; return XML_ERROR_RECURSIVE_ENTITY_REF; @@ -2641,8 +2650,13 @@ doInternalEntityRef(XML_Parser const xmlParserP, *errorP = NULL; } else { OPEN_INTERNAL_ENTITY openEntity; + if (entityP->open || parserP->m_recurse_lock > MAX_RECURSE) { + *errorCodeP = XML_ERROR_RECURSIVE_ENTITY_REF; + return; + } entityP->open = 1; /* recursion control */ + parserP->m_recurse_lock++; openEntity.next = parserP->m_openInternalEntities; @@ -2659,6 +2673,7 @@ doInternalEntityRef(XML_Parser const xmlParserP, 0, errorCodeP, errorP); entityP->open = 0; /* recursion control */ + parserP->m_recurse_lock--; parserP->m_openInternalEntities = openEntity.next; } } @@ -2681,8 +2696,14 @@ doExternalEntityRef(XML_Parser const xmlParserP, if (parserP->m_externalEntityRefHandler) { const XML_Char * contextP; + if (entityP->open || parserP->m_recurse_lock > MAX_RECURSE) { + *errorCodeP = XML_ERROR_RECURSIVE_ENTITY_REF; + return; + } entityP->open = 1; + parserP->m_recurse_lock++; contextP = getContext(xmlParserP); + parserP->m_recurse_lock--; entityP->open = 0; if (!contextP) @@ -2753,7 +2774,7 @@ doEntityRef(XML_Parser const xmlParserP, *errorCodeP = XML_ERROR_NONE; } } else { - if (entityP->open) + if (entityP->open || parserP->m_recurse_lock > MAX_RECURSE) *errorCodeP = XML_ERROR_RECURSIVE_ENTITY_REF; else if (entityP->notation) *errorCodeP = XML_ERROR_BINARY_ENTITY_REF; @@ -4059,7 +4080,7 @@ doProlog(XML_Parser const xmlParserP, *errorCodeP = XML_ERROR_UNDEFINED_ENTITY; return; } - if (entity->open) { + if (entity->open || parser->m_recurse_lock > MAX_RECURSE) { *errorCodeP = XML_ERROR_RECURSIVE_ENTITY_REF; return; } @@ -4076,16 +4097,19 @@ doProlog(XML_Parser const xmlParserP, if (externalEntityRefHandler) { dtd.complete = 0; entity->open = 1; + parser->m_recurse_lock++; if (!externalEntityRefHandler(externalEntityRefHandlerArg, 0, entity->base, entity->systemId, entity->publicId)) { entity->open = 0; + parser->m_recurse_lock--; *errorCodeP = XML_ERROR_EXTERNAL_ENTITY_HANDLING; return; } entity->open = 0; + parser->m_recurse_lock--; if (dtd.complete) break; } @@ -4199,6 +4223,7 @@ processInternalParamEntity(XML_Parser const parser, const char *s, *end, *next; int tok; OPEN_INTERNAL_ENTITY openEntity; + Parser * const parserP = (Parser *) parser; entity->open = 1; openEntity.next = openInternalEntities; @@ -4292,6 +4317,7 @@ xmlrpc_XML_ParserCreate(const XML_Char * const encodingName) { parser->m_paramEntityParsing = XML_PARAM_ENTITY_PARSING_NEVER; parser->m_hash_secret_salt = 0; parser->m_ns = 0; + parser->m_recurse_lock = 0; poolInit(&parser->m_tempPool); poolInit(&parser->m_temp2Pool); parser->m_protocolEncodingName = diff --git a/test/Makefile b/test/Makefile index 1242910..59c698c 100644 --- a/test/Makefile +++ b/test/Makefile @@ -16,9 +16,9 @@ LDADD_CGI_SERVER = \ default: all -INCLUDES = -I$(BLDDIR) -Isrcdir/include -Isrcdir/lib/util/include \ +INCLUDES = -I$(BLDDIR) -I$(SRCDIR)/include -I$(SRCDIR)/lib/util/include \ -PROGS = test cgitest1 +PROGS = test cgitest1 overflow_attr_test overflow_entity_test all: $(PROGS) $(SUBDIRS:%=%/all) @@ -86,6 +86,28 @@ OBJS = $(TEST_OBJS) cgitest1.o $(OBJS):%.o:%.c $(CC) -c $(INCLUDES) $(CFLAGS_ALL) $< +OVERFLOW_ATTR_OBJS = overflow_attr_test.o + +overflow_attr_test: $(OVERFLOW_ATTR_OBJS) $(LIBXMLRPC_SERVER_A) \ + $(LIBXMLRPC_A) $(LIBXMLRPC_UTIL_A) $(LIBXMLRPC_XML) + $(CCLD) -o $@ $(LDFLAGS_ALL) \ + $(OVERFLOW_ATTR_OBJS) $(LDADD_CLIENT) $(LDADD_ABYSS_SERVER) $(CASPRINTF) +OBJS = overflow_attr_test.o + +$(OBJS):%.o:%.c + $(CC) -c $(INCLUDES) -I$(SRCDIR)/lib/expat/xmlparse/ $(CFLAGS_ALL) $< + +OVERFLOW_ENTITY_OBJS = overflow_entity_test.o + +overflow_entity_test: $(OVERFLOW_ENTITY_OBJS) $(LIBXMLRPC_SERVER_A) \ + $(LIBXMLRPC_A) $(LIBXMLRPC_UTIL_A) $(LIBXMLRPC_XML) + $(CCLD) -o $@ $(LDFLAGS_ALL) \ + $(OVERFLOW_ENTITY_OBJS) $(LDADD_CLIENT) $(LDADD_ABYSS_SERVER) $(CASPRINTF) +OBJS = overflow_entity_test.o + +$(OBJS):%.o:%.c + $(CC) -c $(INCLUDES) -I$(SRCDIR)/lib/expat/xmlparse/ $(CFLAGS_ALL) $< + # Note the difference between 'check' and 'runtests'. 'check' means to check # our own correctness. 'runtests' means to run the tests that check our # parent's correctness @@ -94,8 +116,10 @@ $(OBJS):%.o:%.c check: .PHONY: runtests_local -runtests_local: test cgitest1 +runtests_local: test cgitest1 overflow_attr_test overflow_entity_test ./test + ./overflow_attr_test + ./overflow_entity_test .PHONY: runtests runtests: runtests_local cpp/runtests benchmark/runtests diff --git a/test/overflow_attr_test.c b/test/overflow_attr_test.c new file mode 100644 index 0000000..eb06556 --- /dev/null +++ b/test/overflow_attr_test.c @@ -0,0 +1,88 @@ +/* gcc -g -std=c11 -D_POSIX_C_SOURCE=200809L -I lib/expat/xmlparse -lxmlrpc_xmlparse -lxmlrpc_xmltok overflow_attr_test.c -o overflow_attr_test */ + +#include +#include +#include +#include +#include +#include "xmlparse.h" +#include + +#define XML_STATUS_OK 1 +#define XML_STATUS_ERROR 0 + +typedef unsigned char XML_Bool; +#define XML_TRUE ((XML_Bool)1) +#define XML_FALSE ((XML_Bool)0) + +#define SIZE_PER_LINE 50 + +static int +doParse(int N_LINES) { + XML_Parser parser; + char tmp[SIZE_PER_LINE]; + int ret = 0; + int rval = 0; + + if ((parser = xmlrpc_XML_ParserCreate(NULL)) == NULL) + errx(1, "XML_ParserCreate"); + + char *const text = (char *)malloc((N_LINES + 4) * SIZE_PER_LINE); + if (text == NULL) { + printf("malloc failed"); + return 1; + } + + // Create the XML + snprintf(text, SIZE_PER_LINE, + "\n"); + + for (size_t i = 1; i < N_LINES; ++i) { + snprintf(tmp, SIZE_PER_LINE, " \n", + (long unsigned)i, (long unsigned)(i - 1)); + strncat(text, tmp, SIZE_PER_LINE); + } + + snprintf(tmp, SIZE_PER_LINE, "]> mainText\n", + (long unsigned)(N_LINES - 1)); + strncat(text, tmp, SIZE_PER_LINE); + + ret = xmlrpc_XML_Parse(parser, text, strlen(text), XML_TRUE); + if (ret == XML_STATUS_ERROR) { + int err = xmlrpc_XML_GetErrorCode(parser); + if (err == XML_ERROR_RECURSIVE_ENTITY_REF) { + if (N_LINES <= 28) { + printf("%d Unexpected FAIL (%s)\n", N_LINES, xmlrpc_XML_ErrorString(err)); + } else { + printf("%d expected failure OK (%s)\n", N_LINES, xmlrpc_XML_ErrorString(err)); + } + } else { + printf("%d Unexpected FAIL (%s)\n", N_LINES, xmlrpc_XML_ErrorString(err)); + rval = 1; + } + } else { + printf("%d OK\n", N_LINES); + } + free(text); + xmlrpc_XML_ParserFree(parser); + return rval; +} + +int main(int argc, char *argv[]) { + int status = 0; + + for (int i = 2; i <= 28; i++ ) + status += doParse(i); + + status += doParse(29); + status += doParse(30); + status += doParse(100); + status += doParse(1000); + status += doParse(10000); + status += doParse(60000); + + if (status == 0) + printf("PASSED\n"); + return (status != 0); +} diff --git a/test/overflow_entity_test.c b/test/overflow_entity_test.c new file mode 100644 index 0000000..d0d1290 --- /dev/null +++ b/test/overflow_entity_test.c @@ -0,0 +1,86 @@ +/* gcc -g -std=c11 -D_POSIX_C_SOURCE=200809L -I lib/expat/xmlparse -lxmlrpc_xmlparse -lxmlrpc_xmltok overflow_entity_test.c -o overflow_entity_test */ + +#include +#include +#include +#include +#include +#include "xmlparse.h" +#include + +#define XML_STATUS_OK 1 +#define XML_STATUS_ERROR 0 + +typedef unsigned char XML_Bool; +#define XML_TRUE ((XML_Bool)1) +#define XML_FALSE ((XML_Bool)0) + +#define SIZE_PER_LINE 50 + +static int +doParse(int N_LINES) { + XML_Parser parser; + char tmp[SIZE_PER_LINE]; + int ret = 0; + int rval = 0; + + if ((parser = xmlrpc_XML_ParserCreate(NULL)) == NULL) + errx(1, "XML_ParserCreate"); + + char *const text = (char *)malloc((N_LINES + 4) * SIZE_PER_LINE); + if (text == NULL) { + printf("malloc failed"); + return 1; + } + + // Create the XML + snprintf(text, SIZE_PER_LINE, + "\n"); + + for (size_t i = 1; i < N_LINES; ++i) { + snprintf(tmp, SIZE_PER_LINE, " \n", + (long unsigned)i, (long unsigned)(i - 1)); + strncat(text, tmp, SIZE_PER_LINE); + } + + snprintf(tmp, SIZE_PER_LINE, "]> &s%lu;\n", + (long unsigned)(N_LINES - 1)); + strncat(text, tmp, SIZE_PER_LINE); + + ret = xmlrpc_XML_Parse(parser, text, strlen(text), XML_TRUE); + if (ret == XML_STATUS_ERROR){ + int err = xmlrpc_XML_GetErrorCode(parser); + if (err == XML_ERROR_RECURSIVE_ENTITY_REF) { + if (N_LINES <= 28) { + printf("%d Unexpected FAIL (%s)\n", N_LINES, xmlrpc_XML_ErrorString(err)); + rval = 1; + } else { + printf("%d expected failure OK (%s)\n", N_LINES, xmlrpc_XML_ErrorString(err)); + } + } + } else { + printf("%d OK\n", N_LINES); + } + free(text); + xmlrpc_XML_ParserFree(parser); + return rval; +} + +int main(int argc, char *argv[]) { + int status = 0; + + for (int i = 2; i <= 28; i++ ) + status += doParse(i); + + status += doParse(29); + status += doParse(30); + status += doParse(100); + status += doParse(1000); + status += doParse(10000); + status += doParse(60000); + + if (status == 0) + printf("PASSED\n"); + return (status != 0); +} -- 2.43.5