Expat can be made to close an open entity that is not at the top of the stack of open entities

[hartwork]

Issue

This is the public side of soon-public (access protected) oss-fuzz Expat finding:
Issue 65924: expat:xml_parsebuffer_fuzzer_UTF-16: Direct-leak in processInternalEntity.

The three key (access protected) contained links are:

• Detailed Report: https://oss-fuzz.com/testcase?key=5339329924759552
• Regressed: https://oss-fuzz.com/revisions?job=libfuzzer_asan_i386_expat&range=202401170613:202401180604
• Reproducer Testcase: https://oss-fuzz.com/download?testcase_id=5339329924759552

The reproducer (also attached as file clusterfuzz-testcase-minimized-xml_parsebuffer_fuzzer_UTF-16-5339329924759552.xml.txt) is essentially this:

<!DOCTYPE s[<!ENTITY : 's <'><!ENTITY q '&:;'>]><L>&q;

The original's SHA245 sum is 7fe9e1bf3d97f9268034cb8d7e5e7fe020c69efd41f379f6c5c5ce87c988d1a1.

The regression link effectively links to 2640b1d...86a3623 (which makes good sense since these changes increase fuzzing coverage).

Here's the LeakSanitizer output enriched with with EXPAT_ENTITY_DEBUG=1:

# EXPAT_ENTITY_DEBUG=1 ./fuzz/xml_parsebuffer_fuzzer_UTF-16 [censored]/clusterfuzz-testcase-minimized-xml_parsebuffer_fuzzer_UTF-16-5339329924759552.xml.txt 
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3648292410
INFO: Loaded 1 modules   (7891 inline 8-bit counters): 7891 [0x5569a577baf0, 0x5569a577d9c3), 
INFO: Loaded 1 PC tables (7891 PCs): 7891 [0x5569a577d9c8,0x5569a579c6f8), 
./fuzz/xml_parsebuffer_fuzzer_UTF-16: Running 1 inputs 1 time(s) each.
Running: [censored]/clusterfuzz-testcase-minimized-xml_parsebuffer_fuzzer_UTF-16-5339329924759552.xml.txt
expat: Entities(0x519000000580): Count         1, depth  1/ 1 &q; OPEN  length 3 (xmlparse.c:5797)
expat: Entities(0x519000000580): Count         2, depth  2/ 2   &:; OPEN  length 3 (xmlparse.c:5797)
expat: Entities(0x519000000580): Count         2, depth  2/ 2   &q; CLOSE length 3 (xmlparse.c:5831)
expat: Entities(0x519000004b80): Count         1, depth  1/ 1 &q; OPEN  length 3 (xmlparse.c:5797)
expat: Entities(0x519000004b80): Count         2, depth  2/ 2   &:; OPEN  length 3 (xmlparse.c:5797)
expat: Entities(0x519000004b80): Count         2, depth  2/ 2   &q; CLOSE length 3 (xmlparse.c:5831)

=================================================================
==24584==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0x5569a56633df in malloc /var/tmp/portage/sys-libs/compiler-rt-sanitizers-18.0.0_pre20240113/work/compiler-rt/lib/asan/asan_malloc_linux.cpp:68:3
    #1 0x5569a56c1b71 in processInternalEntity [censored]/expat/lib/xmlparse.c:5791:35
    #2 0x5569a56c95bf in doContent [censored]/expat/lib/xmlparse.c:2915:18
    #3 0x5569a56c1ebd in processInternalEntity [censored]/expat/lib/xmlparse.c:5821:14
    #4 0x5569a56c95bf in doContent [censored]/expat/lib/xmlparse.c:2915:18
    #5 0x5569a56b4cc3 in contentProcessor [censored]/expat/lib/xmlparse.c:2652:27
    #6 0x5569a56b4cc3 in doProlog [censored]/expat/lib/xmlparse.c:4935:14
    #7 0x5569a56ae99a in prologProcessor [censored]/expat/lib/xmlparse.c:4638:10
    #8 0x5569a56ae99a in prologInitProcessor [censored]/expat/lib/xmlparse.c:4440:10
    #9 0x5569a56ac1db in XML_ParseBuffer [censored]/expat/lib/xmlparse.c:2047:25
    #10 0x5569a56a12c7 in ParseOneInput [censored]/expat/fuzz/xml_parsebuffer_fuzzer.c:72:3
    #11 0x5569a56a0b80 in LLVMFuzzerTestOneInput [censored]/expat/fuzz/xml_parsebuffer_fuzzer.c:94:3
    #12 0x5569a55aea79 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /var/tmp/portage/sys-libs/compiler-rt-sanitizers-18.0.0_pre20240113/work/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
    #13 0x5569a5598bf2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /var/tmp/portage/sys-libs/compiler-rt-sanitizers-18.0.0_pre20240113/work/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
    #14 0x5569a559e218 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /var/tmp/portage/sys-libs/compiler-rt-sanitizers-18.0.0_pre20240113/work/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
    #15 0x5569a55c8b92 in main /var/tmp/portage/sys-libs/compiler-rt-sanitizers-18.0.0_pre20240113/work/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #16 0x7f599fedfc89  (/lib64/libc.so.6+0x29c89)

SUMMARY: AddressSanitizer: 40 byte(s) leaked in 1 allocation(s).

INFO: a leak has been found in the initial corpus.

INFO: to ignore leaks on libFuzzer side use -detect_leaks=0.

Analysis

Foreword

It should be noted first that general entity : and its use via &:; are well-formed with regard to Section "Names and Tokens" in 2.3 Common Syntactic Constructs of XML 1.0 Fourth Edition:

[5] Name ::= (Letter | '_' | ':') (NameChar)*
                             ^^^

It should also be noted that Expat allows operation with or without support for XML Namespaces (where : has a special role), and that this finding was yield with function XML_ParserCreate where namespace support is disabled.

There is nothing UTF-16 about this finding despite the related fuzzer and filename, this is UTF-8 even in original form.

Closer Look

At parse time reference &q; is replaced by text &:; which is in turn replaced by text s <. The s in there is what made the fuzzer stop the parser from the character handler. At that point both entities are marked open (see OPEN_INTERNAL_ENTITY *m_openInternalEntities; and its use). When suspending, the existing code on master decides to keep entity q open (since the < has not yet been processed) but close entity : despite not being at the top of the open entity stack. As a result, the other open entity's memory is leaked and that is the symptom that triggered LeakSanitizer and made oss-fuzz uncover the issue.

A candidate fix would be to add a check to only close an entity when it is the top of the stack of open entities. A related pull request coming up in a few minutes, review and testing is welcome. This issue exists primarily to clearly separate problem and solution.

I am not aware of any security aspects of this finding.

For a reproducer that is slightly less cryptic than the original finding, I can offer this alternative:

<!DOCTYPE t1[<!ENTITY e1 'angle<'><!ENTITY e2 '&e1;'>]><t1>&e2;

It should be noted that without stopping the parser while parsing, this isolate sample will not show anything interesting with e.g. xmlwf.

CC @catenacyber @RMJ10 @Snild-Sony

[catenacyber]

Nice thorough analysis and glad the improved coverage found a bug