commit cd3b344e0dbd19a812d0b4f34f9d089ed7c5c411 Author: Tomas Korbar Date: Tue Mar 19 15:12:18 2024 +0100 Fix CVE-2024-28757 Upstream PRs #841 and #842 diff --git a/expat/lib/xmlparse.c b/expat/lib/xmlparse.c index 2ae64e9..0896b16 100644 --- a/expat/lib/xmlparse.c +++ b/expat/lib/xmlparse.c @@ -6164,7 +6164,7 @@ storeEntityValue(XML_Parser parser, const ENCODING *enc, dtd->keepProcessing = dtd->standalone; goto endEntityValue; } - if (entity->open) { + if (entity->open || (entity == parser->m_declEntity)) { if (enc == parser->m_encoding) parser->m_eventPtr = entityTextPtr; result = XML_ERROR_RECURSIVE_ENTITY_REF; @@ -7680,6 +7680,8 @@ copyString(const XML_Char *s, const XML_Memory_Handling_Suite *memsuite) { static float accountingGetCurrentAmplification(XML_Parser rootParser) { + // 1.........1.........12 => 22 + const size_t lenOfShortestInclude = sizeof("") - 1; const XmlBigCount countBytesOutput = rootParser->m_accounting.countBytesDirect + rootParser->m_accounting.countBytesIndirect; @@ -7687,7 +7689,9 @@ accountingGetCurrentAmplification(XML_Parser rootParser) { = rootParser->m_accounting.countBytesDirect ? (countBytesOutput / (float)(rootParser->m_accounting.countBytesDirect)) - : 1.0f; + : ((lenOfShortestInclude + + rootParser->m_accounting.countBytesIndirect) + / (float)lenOfShortestInclude); assert(! rootParser->m_parentParser); return amplificationFactor; } diff --git a/expat/tests/runtests.c b/expat/tests/runtests.c index 941f61d..93adc45 100644 --- a/expat/tests/runtests.c +++ b/expat/tests/runtests.c @@ -1788,6 +1788,48 @@ START_TEST(test_wfc_no_recursive_entity_refs) { } END_TEST +START_TEST(test_recursive_external_parameter_entity_2) { + struct TestCase { + const char *doc; + enum XML_Status expectedStatus; + }; + + struct TestCase cases[] = { + {"", XML_STATUS_ERROR}, + {"" + "", + XML_STATUS_ERROR}, + {"" + "", + XML_STATUS_OK}, + {"", XML_STATUS_OK}, + }; + + for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) { + const char *const doc = cases[i].doc; + const enum XML_Status expectedStatus = cases[i].expectedStatus; + + XML_Parser parser = XML_ParserCreate(NULL); + assert_true(parser != NULL); + + XML_Parser ext_parser = XML_ExternalEntityParserCreate(parser, NULL, NULL); + assert_true(ext_parser != NULL); + + const enum XML_Status actualStatus + = _XML_Parse_SINGLE_BYTES(ext_parser, doc, (int)strlen(doc), XML_TRUE); + + assert_true(actualStatus == expectedStatus); + if (actualStatus != XML_STATUS_OK) { + assert_true(XML_GetErrorCode(ext_parser) + == XML_ERROR_RECURSIVE_ENTITY_REF); + } + + XML_ParserFree(ext_parser); + XML_ParserFree(parser); + } +} +END_TEST + /* Test incomplete external entities are faulted */ START_TEST(test_ext_entity_invalid_parse) { const char *text = "" + // (22) that is used in function accountingGetCurrentAmplification in + // xmlparse.c. + // 1.........1.........1.........1.........1..4 => 44 + const char doc[] = ""; + const int docLen = (int)sizeof(doc) - 1; + const float maximumToleratedAmplification = 2.0f; + + struct TestCase { + int offsetOfThreshold; + enum XML_Status expectedStatus; + }; + + struct TestCase cases[] = { + {-2, XML_STATUS_ERROR}, {-1, XML_STATUS_ERROR}, {0, XML_STATUS_ERROR}, + {+1, XML_STATUS_OK}, {+2, XML_STATUS_OK}, + }; + + for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) { + const int offsetOfThreshold = cases[i].offsetOfThreshold; + const enum XML_Status expectedStatus = cases[i].expectedStatus; + const unsigned long long activationThresholdBytes + = docLen + offsetOfThreshold; + + XML_Parser parser = XML_ParserCreate(NULL); + assert_true(parser != NULL); + + assert_true(XML_SetBillionLaughsAttackProtectionMaximumAmplification( + parser, maximumToleratedAmplification) + == XML_TRUE); + assert_true(XML_SetBillionLaughsAttackProtectionActivationThreshold( + parser, activationThresholdBytes) + == XML_TRUE); + + XML_Parser ext_parser = XML_ExternalEntityParserCreate(parser, NULL, NULL); + assert_true(ext_parser != NULL); + + const enum XML_Status actualStatus + = _XML_Parse_SINGLE_BYTES(ext_parser, doc, docLen, XML_TRUE); + + assert_true(actualStatus == expectedStatus); + if (actualStatus != XML_STATUS_OK) { + assert_true(XML_GetErrorCode(ext_parser) + == XML_ERROR_AMPLIFICATION_LIMIT_BREACH); + } + + XML_ParserFree(ext_parser); + XML_ParserFree(parser); + } +} +END_TEST + #endif // defined(XML_DTD) static Suite * @@ -12871,6 +12967,8 @@ make_suite(void) { tcase_add_test__ifdef_xml_dtd(tc_basic, test_skipped_parameter_entity); tcase_add_test__ifdef_xml_dtd(tc_basic, test_recursive_external_parameter_entity); + tcase_add_test__ifdef_xml_dtd(tc_basic, + test_recursive_external_parameter_entity_2); tcase_add_test(tc_basic, test_undefined_ext_entity_in_external_dtd); tcase_add_test(tc_basic, test_suspend_xdecl); tcase_add_test(tc_basic, test_abort_epilog); @@ -13120,6 +13218,7 @@ make_suite(void) { tcase_add_test(tc_accounting, test_accounting_precision); tcase_add_test(tc_accounting, test_billion_laughs_attack_protection_api); tcase_add_test(tc_accounting, test_helper_unsigned_char_to_printable); + tcase_add_test(tc_accounting, test_amplification_isolated_external_parser); #endif return s;