[CVE-2019-15903] Heap overflow in XML_GetCurrentLineNumber

[yuweol]

Hello,

Heap overflow found when I call below apis with a crafted input value.

parser = XML_ParserCreate(NULL);
XML_SetParamEntityParsing(parser, XML_PARAM_ENTITY_PARSING_ALWAYS);
if (XML_Parse(parser, input, (int)strlen(input), XML_TRUE) != XML_STATUS_SUSPENDED) {
fprintf(stderr, "%d", XML_GetCurrentLineNumber(parser));
}

Attached file has both a source code to replay this bug and the bug report of address sanitizer.
This bug found R_2_2_6, R_2_2_7.

I used following commands to build libexpat and build the source code for replaying the bug.

[1] ./configure CC=clang CXX=clang++ CFLAGS="-m32 -fsanitize=address -g" CXXFLAGS="-m32 -fsanitize=address" LDFLAGS="-m32 -fsanitize=address"
[2] make
[3] clang -DHAVE_CONFIG_H -I. -I.. -I./../lib -DHAVE_EXPAT_CONFIG_H -m32 -fsanitize=address -g -Wall -Wmissing-prototypes -Wstrict-prototypes -fexceptions -fno-strict-aliasing -o bug bug.c lib/.libs/libexpat.a && ./bug

I hope this report help expat to be secured.

expat.zip

[hartwork]

Hi @yuweol, thanks for the report! A closer look might take a few days.

[hartwork]

My analysis so far:

Cause of this very crash

• XML_GetCurrentLineNumber finds that parser->m_eventPtr >= parser->m_positionPtr and that the line number needs to be increased further, to be up to date.
• The function operates with the assumption (A1) that parser->m_eventPtr and parser->m_positionPtr are both pointers into a common array.
• When that assumption is wrong, XmlUpdatePosition will run forward from parser->m_positionPtrtowardsparser->m_eventPtr` reading memory it's not supposed to read. This is where ASan says "no".
• XmlUpdatePosition itself works fine — HAS_CHAR(enc, ptr, end) remains true and BYTE_TYPE(enc, ptr) soon remains at BT_TRAIL, looping ptr += MINBPC(enc); until ASan wakes up. The value of end passed to it is the problem, not the code in this function.
• (On a side note, normal_updatePosition is PREFIX(updatePosition) in xmltok_impl.c with #define PREFIX(ident) normal_##ident, btw.)
• Back to above mentioned assumption A1. A1 is violated when Expat turns to "inlining" the internal entity's content and then processing the result. doContent sets parser->m_eventPtr to an array not longer common with parser->m_positionPtr.

Underlying issue

A minified version of the trouble input is this:

<!DOCTYPE d [
<!ENTITY % e "]><d">
%e;

When %e; is "inlined" to ]><d the parser treats ]> as the end of the prolog
and (simplified) proceeds parsing <d as if it was part of the XML document body.

A simplified call tree for this situation would look like this:

doProlog
  case XML_ROLE_PARAM_ENTITY_REF
    processInternalEntity
      doProlog
        case XML_ROLE_INSTANCE_START
          contentProcessor
            doContent

This file will can — depending on isFinal and the number of calls to XML_Parse — fool Expat to believe that it's dealing with a well-formed document…

<!DOCTYPE doc [
<!ENTITY % foo "]><doc/>">
%foo;

… when -p (for XML_SetParamEntityParsing(parser, XML_PARAM_ENTITY_PARSING_ALWAYS)) is passed:

# xmlwf -p <<<$'<!DOCTYPE doc [\n<!ENTITY % foo "]><doc/>">\n%foo;'  # needs Bash
<no error output>

The same can be observed in Firefox (e.g. version 66.0.3).

Towards a fix

To my understanding, Expat should reject prolog termination ]> done by entity values. A work-in-progress pull request to make that happen is on its way.

I'm grateful for comments about this understanding and direction for a fix.

CC @yuweol @RMJ10 @DerDakon

[hartwork]

Quick update: I have requested a CVE for this from MITRE a few minutes ago.

[hartwork]

Merged by now, closing.