Merge pull request #645 from libexpat/issue-612-fix-corruption-from-undefined-entities

Fix corruption from undefined entities (fixes #612, variation of #615)

3 files changed, 66 insertions, 1 deletions

diff --git a/expat/Changes b/expat/Changes
index be6d87a8..e4db7700 100644
--- a/expat/Changes
+++ b/expat/Changes
@@ -3,9 +3,18 @@ NOTE: We are looking for help with a few things:
       If you can help, please get in touch.  Thanks!
 
 Release x.x.x xxx xxxxxxxxxxxx xx xxxx
+        Bug fixes:
+       #612 #645  Fix curruption from undefined entities
+
         Other changes:
             #648  Address compiler warnings
 
+        Special thanks to:
+            Jann Horn
+            Rhodri James
+                 and
+            Google Project Zero
+
 Release 2.4.9 Tue September 20 2022
         Security fixes:
        #629 #640  CVE-2022-40674 -- Heap use-after-free vulnerability in
diff --git a/expat/lib/xmlparse.c b/expat/lib/xmlparse.c
index d3c12129..5e2c16b2 100644
--- a/expat/lib/xmlparse.c
+++ b/expat/lib/xmlparse.c
@@ -4975,10 +4975,10 @@ doProlog(XML_Parser parser, const ENCODING *enc, const char *s, const char *end,
               parser->m_handlerArg, parser->m_declElementType->name,
               parser->m_declAttributeId->name, parser->m_declAttributeType, 0,
               role == XML_ROLE_REQUIRED_ATTRIBUTE_VALUE);
-          poolClear(&parser->m_tempPool);
           handleDefault = XML_FALSE;
         }
       }
+      poolClear(&parser->m_tempPool);
       break;
     case XML_ROLE_DEFAULT_ATTRIBUTE_VALUE:
     case XML_ROLE_FIXED_ATTRIBUTE_VALUE:
diff --git a/expat/tests/runtests.c b/expat/tests/runtests.c
index 938956ac..a8cc1f03 100644
--- a/expat/tests/runtests.c
+++ b/expat/tests/runtests.c
@@ -6734,6 +6734,60 @@ START_TEST(test_empty_element_abort) {
 }
 END_TEST
 
+/* Regression test for GH issue #612: unfinished m_declAttributeType
+ * allocation in ->m_tempPool can corrupt following allocation.
+ */
+static int XMLCALL
+external_entity_unfinished_attlist(XML_Parser parser, const XML_Char *context,
+                                   const XML_Char *base,
+                                   const XML_Char *systemId,
+                                   const XML_Char *publicId) {
+  const char *text = "<!ELEMENT barf ANY>\n"
+                     "<!ATTLIST barf my_attr (blah|%blah;a|foo) #REQUIRED>\n"
+                     "<!--COMMENT-->\n";
+  XML_Parser ext_parser;
+
+  UNUSED_P(base);
+  UNUSED_P(publicId);
+  if (systemId == NULL)
+    return XML_STATUS_OK;
+
+  ext_parser = XML_ExternalEntityParserCreate(parser, context, NULL);
+  if (ext_parser == NULL)
+    fail("Could not create external entity parser");
+
+  if (_XML_Parse_SINGLE_BYTES(ext_parser, text, (int)strlen(text), XML_TRUE)
+      == XML_STATUS_ERROR)
+    xml_failure(ext_parser);
+
+  XML_ParserFree(ext_parser);
+  return XML_STATUS_OK;
+}
+
+START_TEST(test_pool_integrity_with_unfinished_attr) {
+  const char *text = "<?xml version='1.0' encoding='UTF-8'?>\n"
+                     "<!DOCTYPE foo [\n"
+                     "<!ELEMENT foo ANY>\n"
+                     "<!ENTITY % entp SYSTEM \"external.dtd\">\n"
+                     "%entp;\n"
+                     "]>\n"
+                     "<a></a>\n";
+  const XML_Char *expected = XCS("COMMENT");
+  CharData storage;
+
+  CharData_Init(&storage);
+  XML_SetParamEntityParsing(g_parser, XML_PARAM_ENTITY_PARSING_ALWAYS);
+  XML_SetExternalEntityRefHandler(g_parser, external_entity_unfinished_attlist);
+  XML_SetAttlistDeclHandler(g_parser, dummy_attlist_decl_handler);
+  XML_SetCommentHandler(g_parser, accumulate_comment);
+  XML_SetUserData(g_parser, &storage);
+  if (_XML_Parse_SINGLE_BYTES(g_parser, text, (int)strlen(text), XML_TRUE)
+      == XML_STATUS_ERROR)
+    xml_failure(g_parser);
+  CharData_CheckXMLChars(&storage, expected);
+}
+END_TEST
+
 /*
  * Namespaces tests.
  */
@@ -12169,6 +12223,8 @@ make_suite(void) {
   tcase_add_test(tc_basic, test_bad_notation);
   tcase_add_test(tc_basic, test_default_doctype_handler);
   tcase_add_test(tc_basic, test_empty_element_abort);
+  tcase_add_test__ifdef_xml_dtd(tc_basic,
+                                test_pool_integrity_with_unfinished_attr);
 
   suite_add_tcase(s, tc_namespace);
   tcase_add_checked_fixture(tc_namespace, namespace_setup, namespace_teardown);