diff --git a/expat/Changes b/expat/Changes index 9c1b5c7b..6198e4ff 100644 --- a/expat/Changes +++ b/expat/Changes @@ -2,6 +2,22 @@ NOTE: We are looking for help with a few things: https://github.com/libexpat/libexpat/labels/help%20wanted If you can help, please get in touch. Thanks! +Release X.X.X XXX XXXXXXX XX XXXX + Security fixes: + #561 CVE-2022-25236 -- Passing (one or more) namespace separator + characters in "xmlns[:prefix]" attribute values + made Expat send malformed tag names to the XML + processor on top of Expat which can cause + arbitrary damage (e.g. code execution) depending + on such unexpectable cases are handled inside the XML + processor; validation was not their job but Expat's. + Exploits with code execution are known to exist. + + Special thanks to: + Ivan Fratric + and + Google Project Zero + Release 2.4.4 Sun January 30 2022 Security fixes: #550 CVE-2022-23852 -- Fix signed integer overflow diff --git a/expat/lib/xmlparse.c b/expat/lib/xmlparse.c index 7376aab1..c98e2e9f 100644 --- a/expat/lib/xmlparse.c +++ b/expat/lib/xmlparse.c @@ -718,8 +718,7 @@ XML_ParserCreate(const XML_Char *encodingName) { XML_Parser XMLCALL XML_ParserCreateNS(const XML_Char *encodingName, XML_Char nsSep) { - XML_Char tmp[2]; - *tmp = nsSep; + XML_Char tmp[2] = {nsSep, 0}; return XML_ParserCreate_MM(encodingName, NULL, tmp); } @@ -1344,8 +1343,7 @@ XML_ExternalEntityParserCreate(XML_Parser oldParser, const XML_Char *context, would be otherwise. */ if (parser->m_ns) { - XML_Char tmp[2]; - *tmp = parser->m_namespaceSeparator; + XML_Char tmp[2] = {parser->m_namespaceSeparator, 0}; parser = parserCreate(encodingName, &parser->m_mem, tmp, newDtd); } else { parser = parserCreate(encodingName, &parser->m_mem, NULL, newDtd); @@ -3761,6 +3759,17 @@ addBinding(XML_Parser parser, PREFIX *prefix, const ATTRIBUTE_ID *attId, if (! mustBeXML && isXMLNS && (len > xmlnsLen || uri[len] != xmlnsNamespace[len])) isXMLNS = XML_FALSE; + + // NOTE: While Expat does not validate namespace URIs against RFC 3986, + // we have to at least make sure that the XML processor on top of + // Expat (that is splitting tag names by namespace separator into + // 2- or 3-tuples (uri-local or uri-local-prefix)) cannot be confused + // by an attacker putting additional namespace separator characters + // into namespace declarations. That would be ambiguous and not to + // be expected. + if (parser->m_ns && (uri[len] == parser->m_namespaceSeparator)) { + return XML_ERROR_SYNTAX; + } } isXML = isXML && len == xmlLen; isXMLNS = isXMLNS && len == xmlnsLen; diff --git a/expat/tests/runtests.c b/expat/tests/runtests.c index d07203f2..bc5344b1 100644 --- a/expat/tests/runtests.c +++ b/expat/tests/runtests.c @@ -7220,6 +7220,35 @@ START_TEST(test_ns_double_colon_doctype) { } END_TEST +START_TEST(test_ns_separator_in_uri) { + struct test_case { + enum XML_Status expectedStatus; + const char *doc; + }; + struct test_case cases[] = { + {XML_STATUS_OK, ""}, + {XML_STATUS_ERROR, ""}, + }; + + size_t i = 0; + size_t failCount = 0; + for (; i < sizeof(cases) / sizeof(cases[0]); i++) { + XML_Parser parser = XML_ParserCreateNS(NULL, '\n'); + XML_SetElementHandler(parser, dummy_start_element, dummy_end_element); + if (XML_Parse(parser, cases[i].doc, (int)strlen(cases[i].doc), + /*isFinal*/ XML_TRUE) + != cases[i].expectedStatus) { + failCount++; + } + XML_ParserFree(parser); + } + + if (failCount) { + fail("Namespace separator handling is broken"); + } +} +END_TEST + /* Control variable; the number of times duff_allocator() will successfully * allocate */ #define ALLOC_ALWAYS_SUCCEED (-1) @@ -11905,6 +11934,7 @@ make_suite(void) { tcase_add_test(tc_namespace, test_ns_utf16_doctype); tcase_add_test(tc_namespace, test_ns_invalid_doctype); tcase_add_test(tc_namespace, test_ns_double_colon_doctype); + tcase_add_test(tc_namespace, test_ns_separator_in_uri); suite_add_tcase(s, tc_misc); tcase_add_checked_fixture(tc_misc, NULL, basic_teardown);