GNOME Bugzilla – Bug 775200
global-buffer-overflow in htmlParseTryOrFinish (HTMLparser.c:5403)
Last modified: 2019-01-30 10:30:33 UTC
Created attachment 340871 [details] poc found with AFL+AddressSanitizer: ./configure --disable-shared AFL_USE_ASAN=1 make clean all xmllint --html --push poc ERROR: AddressSanitizer: global-buffer-overflow on address 0x088052c1 at pc 0x082f55b2 bp 0xbfd06418 sp 0xbfd06408 READ of size 1 at 0x088052c1 thread T0 #0 0x82f55b1 in htmlParseTryOrFinish /vagrant/libxml2/HTMLparser.c:5403 #1 0x82f55b1 in htmlParseChunk /vagrant/libxml2/HTMLparser.c:6148 #2 0x805e1dd in parseAndPrintFile /vagrant/libxml2/xmllint.c:2222 #3 0x80519d1 in main /vagrant/libxml2/xmllint.c:3767 #4 0xb6ffa636 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x18636) #5 0x805765b (/vagrant/libxml2/xmllint+0x805765b) I gdb program and found int nodePush(xmlParserCtxtPtr ctxt, xmlNodePtr value) { ... if ((((unsigned int) ctxt->nodeNr) > xmlParserMaxDepth) && ((ctxt->options & XML_PARSE_HUGE) == 0)) { xmlFatalErrMsgInt(ctxt, XML_ERR_INTERNAL_ERROR, "Excessive depth in document: %d use XML_PARSE_HUGE option\n", xmlParserMaxDepth); xmlHaltParser(ctxt); return(-1); } ... } when xmlParserMaxDepth reached, xmlHaltParser was called, it set ctxt->input->base = ctxt->input->cur, then htmlParseTryOrFinish compute avail = xmlBufUse(in->buf->buffer) - (in->cur - in->base），cause overflow
HTMLparser.c: if (avail < 1) goto done; cur = in->cur[0]; if (cur == 0) { SKIP(1); continue; } The problem is that xmlHaltParser sets cur/base to a different pointer not associated with in->buf->buffer , so the "avail" calculation is pretty much broken as soon as that happens. the switch (ctxt->instate) { should be done somehow befoer continuing to access in->cur
(I can confirm the issue, but the overflow will only work as long as it reads 0 bytes.)
Created attachment 351897 [details] [review] libxml2-CVE-2017-8872.patch perhaps this patch?
Re: comment 3, how is that effectively different to the if (avail < 1) goto done; statement right after? In Comment 2 it sounds like avail is incorrect before that.
No, xmlHaltParser sets cur and base to the same value, so avail will be the full buffer size again and not 0 ... (It is this snippet in parser.c::xmlHaltParser) ctxt->input->cur = BAD_CAST""; ctxt->input->base = ctxt->input->cur; this kind of 0 skipping needs to take the parser state in account :/
I'm sorry but I still don't understand. I think with more context around your patch, there's this: if ((avail == 0) && (terminate)) { htmlAutoCloseOnEnd(ctxt); if ((ctxt->nameNr == 0) && (ctxt->instate != XML_PARSER_EOF)) { /* * SAX: end of the document processing. */ ctxt->instate = XML_PARSER_EOF; if ((ctxt->sax) && (ctxt->sax->endDocument != NULL)) ctxt->sax->endDocument(ctxt->userData); > goto done; } } if (avail < 1) goto done; Either avail == 0 or avail != 0. If avail == 0, then also avail < 1 and that last goto statement will run. If avail != 0, then the goto statement you are adding won't run because of the if ((avail == 0) && ... statement that surrounds it.
Created attachment 355527 [details] [review] xx.pat this makes xmlHaltParser "empty" the buffer, as it resets cur and avail too here. this seems to cure this specific issue, and also passes the testsuite
Any updates on this issue?
Created attachment 366193 [details] [review] Out-of-bounds read in htmlParseTryOrFinish (CVE-2017-8872) Hi The issue is not yet fixed up to master (as per ad88b54f1a28a8565964a370b5d387927b633c0d) and the patch from Marcus in Comment #7 does not apply anymore cleanly as Mattia Rizzolo correctly noted in the Debian Bug https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=862450#22. Attaching a rebased version of the patch from Marcus. Regards, Salvatore
with attachment 366193 [details] [review], one regression tests failed. remove this patch, all test can pass. Please take a look, thanks. ## Error cases regression tests file result/errors/759573.xml.err is 1983 bytes, result is 1557 bytes Error for ./test/errors/759573.xml failed File ./test/errors/759573.xml generated an error FAIL: Error cases regression tests Below is the difference of result/errors/759573.xml.err and actual error we get(I save it in tmp.err). I guess maybe some error msg is missed since cut the buffer not properly. --- result/errors/759573.xml.err +++ tmp.err @@ -21,14 +21,11 @@ ^ ./test/errors/759573.xml:1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration -<?h?><!DOCTYPEt[<!ELEMENT t (A)><!ENTITY % xx '%<![INCLUDE[000%ஸ00 - ^ + +^ ./test/errors/759573.xml:1: parser error : DOCTYPE improperly terminated -<?h?><!DOCTYPEt[<!ELEMENT t (A)><!ENTITY % xx '%<![INCLUDE[000%ஸ00 - ^ -./test/errors/759573.xml:1: parser error : StartTag: invalid element name -<?h?><!DOCTYPEt[<!ELEMENT t (A)><!ENTITY % xx '%<![INCLUDE[000%ஸ00 - ^ -./test/errors/759573.xml:1: parser error : Extra content at the end of the document -<?h?><!DOCTYPEt[<!ELEMENT t (A)><!ENTITY % xx '%<![INCLUDE[000%ஸ00 - ^ + +^ +./test/errors/759573.xml:1: parser error : Start tag expected, '<' not found + +^
Looks like https://gitlab.gnome.org/GNOME/libxml2/issues/26 is the same as this, and was merged.
Fixed with: https://gitlab.gnome.org/GNOME/libxml2/commit/123234f2cfcd9e9b9f83047eee1dc17b4c3f4407 I wasn't aware of this bug report, but the fix is nearly identical.