Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CVE-2022-43680] XML_ParserFree may free parser->m_dtd memory in out-of-memory situations when it should not #649

Closed
hartwork opened this issue Sep 21, 2022 · 0 comments · Fixed by #650
Assignees
Milestone

Comments

@hartwork
Copy link
Member

hartwork commented Sep 21, 2022

Pull request #616 titles "Bugfixes" contains two bug reports. This new ticket is about the "UAF due to DTD destruction" half of it, with the intention of making the issue more easily accessible.

If I squash together the two related commits…

…we get a single commit with this bug description…

If the allocation in parserInit fails:

parser->m_protocolEncodingName = copyString(encodingName, &(parser->m_mem));

Then we'll call XML_ParserFree on the parser inside parserCreate:

if (encodingName && ! parser->m_protocolEncodingName) {
  XML_ParserFree(parser);
  return NULL;
}

If we're inside XML_ExternalEntityParserCreate, this can mean that XML_ParserFree is called on a parser whose m_dtd pointer is shared with the document's root parser. Since the flag m_isParamEntity is only set in XML_ExternalEntityParserCreate after parserCreate returns, this call to XML_ParserFree will incorrectly destroy the shared dtd.

This fix moves the setting of m_isParamEntity into parserCreate, since the dtd parameter is only non-NULL in this case.

and this candidate patch for review:

--- a/expat/lib/xmlparse.c
+++ b/expat/lib/xmlparse.c
@@ -1028,10 +1028,16 @@ parserCreate(const XML_Char *encodingName,
   }
   parser->m_dataBufEnd = parser->m_dataBuf + INIT_DATA_BUF_SIZE;
 
-  if (dtd)
+  if (dtd) {
     parser->m_dtd = dtd;
-  else {
+#ifdef XML_DTD
+    parser->m_isParamEntity = XML_TRUE;
+#endif
+  } else {
     parser->m_dtd = dtdCreate(&parser->m_mem);
+#ifdef XML_DTD
+    parser->m_isParamEntity = XML_FALSE;
+#endif
     if (parser->m_dtd == NULL) {
       FREE(parser, parser->m_dataBuf);
       FREE(parser, parser->m_atts);
@@ -1148,7 +1154,6 @@ parserInit(XML_Parser parser, const XML_Char *encodingName) {
   parser->m_parentParser = NULL;
   parser->m_parsingStatus.parsing = XML_INITIALIZED;
 #ifdef XML_DTD
-  parser->m_isParamEntity = XML_FALSE;
   parser->m_useForeignDTD = XML_FALSE;
   parser->m_paramEntityParsing = XML_PARAM_ENTITY_PARSING_NEVER;
 #endif
@@ -1406,7 +1411,6 @@ XML_ExternalEntityParserCreate(XML_Parser oldParser, const XML_Char *context,
        pointers in parser->m_dtd with ones that get destroyed with the external
        PE parser. This would leave those prefixes with dangling pointers.
     */
-    parser->m_isParamEntity = XML_TRUE;
     XmlPrologStateInitExternalEntity(&parser->m_prologState);
     parser->m_processor = externalParEntInitProcessor;
   }

CC @c01db33f

@hartwork hartwork added the bug label Sep 21, 2022
@hartwork hartwork added this to the 2.5.0 milestone Sep 21, 2022
hartwork added a commit that referenced this issue Sep 21, 2022
hartwork added a commit that referenced this issue Sep 21, 2022
@hartwork hartwork mentioned this issue Sep 21, 2022
@hartwork hartwork self-assigned this Sep 21, 2022
hartwork added a commit that referenced this issue Oct 7, 2022
hartwork added a commit that referenced this issue Oct 17, 2022
hartwork added a commit that referenced this issue Oct 19, 2022
@hartwork hartwork changed the title XML_ParserFree may free parser->m_dtd memory in out-of-memory situations when it should not [CVE-2022-43680] XML_ParserFree may free parser->m_dtd memory in out-of-memory situations when it should not Oct 24, 2022
hartwork added a commit that referenced this issue Oct 24, 2022
hartwork added a commit that referenced this issue Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant