CVE-2017-16931: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: parser.c in libxml2 before 2.9.5 mishandles parameter-entity references because the NEXTL macro calls the xmlParserHandlePEReference function in the case of a '%' character in a DTD name.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 9.8
Published: 2017-11-23T21:29Z
Repository: https://github.com/GNOME/libxml2
Language: Other
Files changed: Makefile.am, parser.c, result/errors10/781205.xml.err, result/errors10/781361.xml.err, result/valid/766956.xml.err, result/valid/766956.xml.err.rdr, runtest.c, test/errors10/781205.xml, test/errors10/781361.xml, test/valid/766956.xml, test/valid/dtds/766956.dtd
Commit date: 2017-06-05 18:38:33 +0200
Commit message: Fix handling of parameter-entity references

There were two bugs where parameter-entity references could lead to an
unexpected change of the input buffer in xmlParseNameComplex and
xmlDictLookup being called with an invalid pointer.

Percent sign in DTD Names
=========================

The NEXTL macro used to call xmlParserHandlePEReference. When parsing
"complex" names inside the DTD, this could result in entity expansion
which created a new input buffer. The fix is to simply remove the call
to xmlParserHandlePEReference from the NEXTL macro. This is safe because
no users of the macro require expansion of parameter entities.

- xmlParseNameComplex
- xmlParseNCNameComplex
- xmlParseNmtoken

The percent sign is not allowed in names, which are grammatical tokens.

- xmlParseEntityValue

Parameter-entity references in entity values are expanded but this
happens in a separate step in this function.

- xmlParseSystemLiteral

Parameter-entity references are ignored in the system literal.

- xmlParseAttValueComplex
- xmlParseCharDataComplex
- xmlParseCommentComplex
- xmlParsePI
- xmlParseCDSect

Parameter-entity references are ignored outside the DTD.

- xmlLoadEntityContent

This function is only called from xmlStringLenDecodeEntities and
entities are replaced in a separate step immediately after the function
call.

This bug could also be triggered with an internal subset and double
entity expansion.

This fixes bug 766956 initially reported by Wei Lei and independently by
Chromium's ClusterFuzz, Hanno Böck, and Marco Grassi. Thanks to everyone
involved.

xmlParseNameComplex with XML_PARSE_OLD10
========================================

When parsing Names inside an expanded parameter entity with the
XML_PARSE_OLD10 option, xmlParseNameComplex would call xmlGROW via the
GROW macro if the input buffer was exhausted. At the end of the
parameter entity's replacement text, this function would then call
xmlPopInput which invalidated the input buffer.

There should be no need to invoke GROW in this situation because the
buffer is grown periodically every XML_PARSER_CHUNK_SIZE characters and,
at least for UTF-8, in xmlCurrentChar. This also matches the code path
executed when XML_PARSE_OLD10 is not set.

This fixes bugs 781205 (CVE-2017-9049) and 781361 (CVE-2017-9050).
Thanks to Marcel Böhme and Thuan Pham for the report.

Additional hardening
====================

A separate check was added in xmlParseNameComplex to validate the
buffer size.

Diff: diff --git a/Makefile.am b/Makefile.am
index 6fc8ffa9..10e716a5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -425,10 +425,28 @@ Errtests : xmllint$(EXEEXT)
 	      diff $(srcdir)/result/errors/$$name result.$$name ; \
 	      diff $(srcdir)/result/errors/$$name.err error.$$name` ; \
 	      if [ -n "$$log" ] ; then echo $$name result ; echo "$$log" ; fi ; \
 	      rm result.$$name error.$$name ; \
 	  fi ; fi ; done)
+	@echo "## Error cases regression tests (old 1.0)"
+	-@(for i in $(srcdir)/test/errors10/*.xml ; do \
+	  name=`basename $$i`; \
+	  if [ ! -d $$i ] ; then \
+	  if [ ! -f $(srcdir)/result/errors10/$$name ] ; then \
+	      echo New test file $$name ; \
+	      $(CHECKER) $(top_builddir)/xmllint --oldxml10 $$i \
+	         2> $(srcdir)/result/errors10/$$name.err \
+		 > $(srcdir)/result/errors10/$$name ; \
+	      grep "MORY ALLO" .memdump  | grep -v "MEMORY ALLOCATED : 0"; \
+	  else \
+	      log=`$(CHECKER) $(top_builddir)/xmllint --oldxml10 $$i 2> error.$$name > result.$$name ; \
+	      grep "MORY ALLO" .memdump  | grep -v "MEMORY ALLOCATED : 0"; \
+	      diff $(srcdir)/result/errors10/$$name result.$$name ; \
+	      diff $(srcdir)/result/errors10/$$name.err error.$$name` ; \
+	      if [ -n "$$log" ] ; then echo $$name result ; echo "$$log" ; fi ; \
+	      rm result.$$name error.$$name ; \
+	  fi ; fi ; done)
 	@echo "## Error cases stream regression tests"
 	-@(for i in $(srcdir)/test/errors/*.xml ; do \
 	  name=`basename $$i`; \
 	  if [ ! -d $$i ] ; then \
 	  if [ ! -f $(srcdir)/result/errors/$$name.str ] ; then \
diff --git a/parser.c b/parser.c
index df2efa55..a175ac4e 100644
--- a/parser.c
+++ b/parser.c
@@ -2119,11 +2119,10 @@ static void xmlGROW (xmlParserCtxtPtr ctxt) {
 #define NEXTL(l) do {							\
     if (*(ctxt->input->cur) == '\n') {					\
 	ctxt->input->line++; ctxt->input->col = 1;			\
     } else ctxt->input->col++;						\
     ctxt->input->cur += l;				\
-    if (*ctxt->input->cur == '%') xmlParserHandlePEReference(ctxt);	\
   } while (0)
 
 #define CUR_CHAR(l) xmlCurrentChar(ctxt, &l)
 #define CUR_SCHAR(s, l) xmlStringCurrentChar(ctxt, s, &l)
 
@@ -3410,24 +3409,27 @@ xmlParseNameComplex(xmlParserCtxtPtr ctxt) {
                     return(NULL);
 	    }
 	    len += l;
 	    NEXTL(l);
 	    c = CUR_CHAR(l);
-	    if (c == 0) {
-		count = 0;
-		GROW;
-                if (ctxt->instate == XML_PARSER_EOF)
-                    return(NULL);
-		c = CUR_CHAR(l);
-	    }
 	}
     }
     if ((len > XML_MAX_NAME_LENGTH) &&
         ((ctxt->options & XML_PARSE_HUGE) == 0)) {
         xmlFatalErr(ctxt, XML_ERR_NAME_TOO_LONG, "Name");
         return(NULL);
     }
+    if (ctxt->input->cur - ctxt->input->base < len) {
+        /*
+         * There were a couple of bugs where PERefs lead to to a change
+         * of the buffer. Check the buffer size to avoid passing an invalid
+         * pointer to xmlDictLookup.
+         */
+        xmlFatalErr(ctxt, XML_ERR_INTERNAL_ERROR,
+                    "unexpected change of input buffer");
+        return (NULL);
+    }
     if ((*ctxt->input->cur == '\n') && (ctxt->input->cur[-1] == '\r'))
         return(xmlDictLookup(ctxt->dict, ctxt->input->cur - (len + 1), len));
     return(xmlDictLookup(ctxt->dict, ctxt->input->cur - len, len));
 }
 
diff --git a/result/errors10/781205.xml b/result/errors10/781205.xml
new file mode 100644
index 00000000..e69de29b
diff --git a/result/errors10/781205.xml.err b/result/errors10/781205.xml.err
new file mode 100644
index 00000000..da15c3f7
--- /dev/null
+++ b/result/errors10/781205.xml.err
@@ -0,0 +1,21 @@
+Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration
+
+ %a; 
+    ^
+Entity: line 1: 
+<:0000
+^
+Entity: line 1: parser error : DOCTYPE improperly terminated
+ %a; 
+    ^
+Entity: line 1: 
+<:0000
+^
+namespace error : Failed to parse QName ':0000'
+ %a; 
+    ^
+<:0000
+      ^
+./test/errors10/781205.xml:4: parser error : Couldn't find end of Start Tag :0000 line 1
+
+^
diff --git a/result/errors10/781361.xml b/result/errors10/781361.xml
new file mode 100644
index 00000000..e69de29b
diff --git a/result/errors10/781361.xml.err b/result/errors10/781361.xml.err
new file mode 100644
index 00000000..655f41a2
--- /dev/null
+++ b/result/errors10/781361.xml.err
@@ -0,0 +1,13 @@
+./test/errors10/781361.xml:4: parser error : xmlParseElementDecl: 'EMPTY', 'ANY' or '(' expected
+
+^
+./test/errors10/781361.xml:4: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration
+
+
+^
+./test/errors10/781361.xml:4: parser error : DOCTYPE improperly terminated
+
+^
+./test/errors10/781361.xml:4: parser error : Start tag expected, '<' not found
+
+^
diff --git a/result/valid/766956.xml b/result/valid/766956.xml
new file mode 100644
index 00000000..e69de29b
diff --git a/result/valid/766956.xml.err b/result/valid/766956.xml.err
new file mode 100644
index 00000000..34b1dae6
--- /dev/null
+++ b/result/valid/766956.xml.err
@@ -0,0 +1,9 @@
+test/valid/dtds/766956.dtd:2: parser error : PEReference: expecting ';'
+%ä%ent;
+   ^
+Entity: line 1: parser error : Content error in the external subset
+ %ent; 
+      ^
+Entity: line 1: 
+value
+^
diff --git a/result/valid/766956.xml.err.rdr b/result/valid/766956.xml.err.rdr
new file mode 100644
index 00000000..77603462
--- /dev/null
+++ b/result/valid/766956.xml.err.rdr
@@ -0,0 +1,10 @@
+test/valid/dtds/766956.dtd:2: parser error : PEReference: expecting ';'
+%ä%ent;
+   ^
+Entity: line 1: parser error : Content error in the external subset
+ %ent; 
+      ^
+Entity: line 1: 
+value
+^
+./test/valid/766956.xml : failed to parse
diff --git a/runtest.c b/runtest.c
index b2ce693b..378b38e3 100644
--- a/runtest.c
+++ b/runtest.c
@@ -4212,10 +4212,13 @@ testDesc testDescriptions[] = {
       errParseTest, "./test/namespaces/*", "result/namespaces/", "", ".err",
       0 },
     { "Error cases regression tests",
       errParseTest, "./test/errors/*.xml", "result/errors/", "", ".err",
       0 },
+    { "Error cases regression tests (old 1.0)",
+      errParseTest, "./test/errors10/*.xml", "result/errors10/", "", ".err",
+      XML_PARSE_OLD10 },
 #ifdef LIBXML_READER_ENABLED
     { "Error cases stream regression tests",
       streamParseTest, "./test/errors/*.xml", "result/errors/", NULL, ".str",
       0 },
     { "Reader regression tests",
diff --git a/test/errors10/781205.xml b/test/errors10/781205.xml
new file mode 100644
index 00000000..d9e9e839
--- /dev/null
+++ b/test/errors10/781205.xml
@@ -0,0 +1,3 @@
+<!DOCTYPE D [
+  <!ENTITY % a "<:0000">
+  %a;
diff --git a/test/errors10/781361.xml b/test/errors10/781361.xml
new file mode 100644
index 00000000..67476bcb
--- /dev/null
+++ b/test/errors10/781361.xml
@@ -0,0 +1,3 @@
+<!DOCTYPE doc [
+  <!ENTITY % elem "<!ELEMENT e0000000000">
+  %elem;
diff --git a/test/valid/766956.xml b/test/valid/766956.xml
new file mode 100644
index 00000000..19a95a0e
--- /dev/null
+++ b/test/valid/766956.xml
@@ -0,0 +1,2 @@
+<!DOCTYPE test SYSTEM "dtds/766956.dtd">
+<test/>
diff --git a/test/valid/dtds/766956.dtd b/test/valid/dtds/766956.dtd
new file mode 100644
index 00000000..dddde68b
--- /dev/null
+++ b/test/valid/dtds/766956.dtd
@@ -0,0 +1,2 @@
+<!ENTITY % ent "value">
+%ä%ent;

Vulnerable code:     if (*ctxt->input->cur == '%') xmlParserHandlePEReference(ctxt);	\
	    if (c == 0) {
		count = 0;
		GROW;
                if (ctxt->instate == XML_PARSER_EOF)
                    return(NULL);
		c = CUR_CHAR(l);
	    }
Fixed code: 	@echo "## Error cases regression tests (old 1.0)"
	-@(for i in $(srcdir)/test/errors10/*.xml ; do \
	  name=`basename $$i`; \
	  if [ ! -d $$i ] ; then \
	  if [ ! -f $(srcdir)/result/errors10/$$name ] ; then \
	      echo New test file $$name ; \
	      $(CHECKER) $(top_builddir)/xmllint --oldxml10 $$i \
	         2> $(srcdir)/result/errors10/$$name.err \
		 > $(srcdir)/result/errors10/$$name ; \
	      grep "MORY ALLO" .memdump  | grep -v "MEMORY ALLOCATED : 0"; \
	  else \
	      log=`$(CHECKER) $(top_builddir)/xmllint --oldxml10 $$i 2> error.$$name > result.$$name ; \
	      grep "MORY ALLO" .memdump  | grep -v "MEMORY ALLOCATED : 0"; \
	      diff $(srcdir)/result/errors10/$$name result.$$name ; \
	      diff $(srcdir)/result/errors10/$$name.err error.$$name` ; \
	      if [ -n "$$log" ] ; then echo $$name result ; echo "$$log" ; fi ; \
	      rm result.$$name error.$$name ; \
	  fi ; fi ; done)
    if (ctxt->input->cur - ctxt->input->base < len) {
        /*
         * There were a couple of bugs where PERefs lead to to a change
         * of the buffer. Check the buffer size to avoid passing an invalid
         * pointer to xmlDictLookup.
         */
        xmlFatalErr(ctxt, XML_ERR_INTERNAL_ERROR,
                    "unexpected change of input buffer");
        return (NULL);
    }
Entity: line 1: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration

 %a; 
    ^
Entity: line 1: 
<:0000
^
Entity: line 1: parser error : DOCTYPE improperly terminated
 %a; 
    ^
Entity: line 1: 
<:0000
^
namespace error : Failed to parse QName ':0000'
 %a; 
    ^
<:0000
      ^
./test/errors10/781205.xml:4: parser error : Couldn't find end of Start Tag :0000 line 1

^
./test/errors10/781361.xml:4: parser error : xmlParseElementDecl: 'EMPTY', 'ANY' or '(' expected

^
./test/errors10/781361.xml:4: parser error : internal error: xmlParseInternalSubset: error detected in Markup declaration


^
./test/errors10/781361.xml:4: parser error : DOCTYPE improperly terminated

^
./test/errors10/781361.xml:4: parser error : Start tag expected, '<' not found

^
test/valid/dtds/766956.dtd:2: parser error : PEReference: expecting ';'
%ä%ent;
   ^
Entity: line 1: parser error : Content error in the external subset
 %ent; 
      ^
Entity: line 1: 
value
^
test/valid/dtds/766956.dtd:2: parser error : PEReference: expecting ';'
%ä%ent;
   ^
Entity: line 1: parser error : Content error in the external subset
 %ent; 
      ^
Entity: line 1: 
value
^
./test/valid/766956.xml : failed to parse
    { "Error cases regression tests (old 1.0)",
      errParseTest, "./test/errors10/*.xml", "result/errors10/", "", ".err",
      XML_PARSE_OLD10 },
<!DOCTYPE D [
  <!ENTITY % a "<:0000">
  %a;
<!DOCTYPE doc [
  <!ENTITY % elem "<!ELEMENT e0000000000">
  %elem;
<!DOCTYPE test SYSTEM "dtds/766956.dtd">
<test/>
<!ENTITY % ent "value">
%ä%ent;