Re: [xml] [PATCH] Check hex or decimal entity for overflow

[Daniel Veillard]

On Tue, Jan 23, 2018 at 09:51:38PM -0800, Jay Civelli wrote:
> On Tue, Jan 23, 2018 at 8:21 PM, Daniel Veillard 
> wrote:
> 
> > On Mon, Jan 22, 2018 at 04:37:17PM +0100, Nick Wellnhofer wrote:
> > > On 09/01/2018 00:55, Joel Hockey wrote:
> > > > Updated patch with XML_ERR_INVALID_CHAR.
> > >
> > > Should be fixed with
> > >
> > >
> > > https://git.gnome.org/browse/libxml2/commit/?id=60dded12cbf1
> > 705927803c5ed615a7a0132aebbd
> > >
> > > As noted previously, this only affects "recovery" mode. The commit
> > addresses
> >
> >   And I repeatedly asked people to *not* use recover mode of the XML parser
> > which is not conformant to the XML spec, unless this is upon an explicit
> > recovery operation, not a default process. I *really* hope that chrome or
> > chromium is *not* using the recovery mode by default for XML parsing
> > in the browser. I guarantee nothing about the recovery mode in the long
> > term
> > and I already wrote that I would remove it from the parser if people were
> > abusing this option.
> >
> 
> Chromium was using recover mode when parsing XML for some operations
> (extension related for example), but not when rendering XML.
> It was changed in https://chromium-review.googlesource.com/c/chromium/
> src/+/879106 so the recover mode is not used anywhere in Chromium now.

  Now, that's very good news :-)

thanks for the update !

Daniel

> Jay
> 
>   XML is fairly tidy, we can't let the general usage diverge from the spec.
> >
> > Daniel
> >
> > > the issue at an earlier point in the parsing process and makes sure not
> > to
> > > return invalid entity content in recovery mode at all.
> > >
> > > Nick
> > > ___
> > > xml mailing list, project page  http://xmlsoft.org/
> > > xml@gnome.org
> > > https://mail.gnome.org/mailman/listinfo/xml
> >
> > --
> > Daniel Veillard  | Red Hat Developers Tools
> > http://developer.redhat.com/
> > veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
> > http://veillard.com/ | virtualization library  http://libvirt.org/
> > ___
> > xml mailing list, project page  http://xmlsoft.org/
> > xml@gnome.org
> > https://mail.gnome.org/mailman/listinfo/xml
> >

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml

Re: [xml] [PATCH] Check hex or decimal entity for overflow

[Jay Civelli via xml]

On Tue, Jan 23, 2018 at 8:21 PM, Daniel Veillard 
wrote:

> On Mon, Jan 22, 2018 at 04:37:17PM +0100, Nick Wellnhofer wrote:
> > On 09/01/2018 00:55, Joel Hockey wrote:
> > > Updated patch with XML_ERR_INVALID_CHAR.
> >
> > Should be fixed with
> >
> >
> > https://git.gnome.org/browse/libxml2/commit/?id=60dded12cbf1
> 705927803c5ed615a7a0132aebbd
> >
> > As noted previously, this only affects "recovery" mode. The commit
> addresses
>
>   And I repeatedly asked people to *not* use recover mode of the XML parser
> which is not conformant to the XML spec, unless this is upon an explicit
> recovery operation, not a default process. I *really* hope that chrome or
> chromium is *not* using the recovery mode by default for XML parsing
> in the browser. I guarantee nothing about the recovery mode in the long
> term
> and I already wrote that I would remove it from the parser if people were
> abusing this option.
>

Chromium was using recover mode when parsing XML for some operations
(extension related for example), but not when rendering XML.
It was changed in https://chromium-review.googlesource.com/c/chromium/
src/+/879106 so the recover mode is not used anywhere in Chromium now.

Jay

  XML is fairly tidy, we can't let the general usage diverge from the spec.
>
> Daniel
>
> > the issue at an earlier point in the parsing process and makes sure not
> to
> > return invalid entity content in recovery mode at all.
> >
> > Nick
> > ___
> > xml mailing list, project page  http://xmlsoft.org/
> > xml@gnome.org
> > https://mail.gnome.org/mailman/listinfo/xml
>
> --
> Daniel Veillard  | Red Hat Developers Tools
> http://developer.redhat.com/
> veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
> http://veillard.com/ | virtualization library  http://libvirt.org/
> ___
> xml mailing list, project page  http://xmlsoft.org/
> xml@gnome.org
> https://mail.gnome.org/mailman/listinfo/xml
>
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml

Re: [xml] [PATCH] Check hex or decimal entity for overflow

[Daniel Veillard]

On Mon, Jan 22, 2018 at 04:37:17PM +0100, Nick Wellnhofer wrote:
> On 09/01/2018 00:55, Joel Hockey wrote:
> > Updated patch with XML_ERR_INVALID_CHAR.
> 
> Should be fixed with
> 
> 
> https://git.gnome.org/browse/libxml2/commit/?id=60dded12cbf1705927803c5ed615a7a0132aebbd
> 
> As noted previously, this only affects "recovery" mode. The commit addresses

  And I repeatedly asked people to *not* use recover mode of the XML parser
which is not conformant to the XML spec, unless this is upon an explicit
recovery operation, not a default process. I *really* hope that chrome or
chromium is *not* using the recovery mode by default for XML parsing
in the browser. I guarantee nothing about the recovery mode in the long term
and I already wrote that I would remove it from the parser if people were
abusing this option.
  XML is fairly tidy, we can't let the general usage diverge from the spec.
 
Daniel

> the issue at an earlier point in the parsing process and makes sure not to
> return invalid entity content in recovery mode at all.
> 
> Nick
> ___
> xml mailing list, project page  http://xmlsoft.org/
> xml@gnome.org
> https://mail.gnome.org/mailman/listinfo/xml

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml

Re: [xml] [PATCH] Check hex or decimal entity for overflow

[Nick Wellnhofer]

On 09/01/2018 00:55, Joel Hockey wrote:

Updated patch with XML_ERR_INVALID_CHAR.


Should be fixed with


https://git.gnome.org/browse/libxml2/commit/?id=60dded12cbf1705927803c5ed615a7a0132aebbd

As noted previously, this only affects "recovery" mode. The commit addresses 
the issue at an earlier point in the parsing process and makes sure not to 
return invalid entity content in recovery mode at all.


Nick
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml

Re: [xml] [PATCH] Check hex or decimal entity for overflow

[Joel Hockey]

Updated patch with XML_ERR_INVALID_CHAR.

On Tue, Jan 9, 2018 at 5:55 AM, Nick Wellnhofer  wrote:

> On 08/01/2018 02:06, Joel Hockey wrote:
>
>> The entity parsing code in tree.c is getting integer overflow when a very
>> long, invalid hex (or decimal) entity is used:  e.g. #xabcdefabcdef;
>>
>
> This is probably the same issue as
>
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=3874
>
> Also see
>
> https://bugzilla.gnome.org/show_bug.cgi?id=783052
>
> The issue only arises in "recovery" mode (XML_PARSE_RECOVER). In the past,
> I tried to fix similar issues by not adding nodes containing invalid
> character references at all in an earlier stage of the parsing code, but
> I'm fine with your approach.
>
> For these cases, I am setting the error to XML_TREE_UNTERMINATED_ENTITY.
>> The other 2 existing codes are XML_TREE_INVALID_HEX, XML_TREE_INVALID_DEC.
>> I thought unterminated is the better choice, but maybe a new code such as
>> XML_TREE_INVALID_CHAR could be used.
>>
>
> Regarding the error code, we could simply use XML_ERR_INVALID_CHAR or not
> report an error at all since invalid numeric character references are
> already detected and reported earlier.
>
> Nick
>
From 0382b8a9754b4a9d8e4edffabf916f071b3806fc Mon Sep 17 00:00:00 2001
From: Joel Hockey 
Date: Wed, 3 Jan 2018 18:52:36 -0800
Subject: [PATCH] Check hex or decimal entity for overflow

---
 tree.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/tree.c b/tree.c
index 959421bd..2089a932 100644
--- a/tree.c
+++ b/tree.c
@@ -1527,6 +1527,12 @@ xmlStringGetNodeList(const xmlDoc *doc, const xmlChar *value) {
 			charval = 0;
 			break;
 		}
+		if (charval > 0x10) {
+			xmlTreeErr(XML_ERR_INVALID_CHAR, (xmlNodePtr) doc,
+			   NULL);
+			charval = 0;
+			break;
+		}
 		cur++;
 		tmp = *cur;
 		}
@@ -1545,6 +1551,12 @@ xmlStringGetNodeList(const xmlDoc *doc, const xmlChar *value) {
 			charval = 0;
 			break;
 		}
+		if (charval > 0x10) {
+			xmlTreeErr(XML_ERR_INVALID_CHAR, (xmlNodePtr) doc,
+			   NULL);
+			charval = 0;
+			break;
+		}
 		cur++;
 		tmp = *cur;
 		}
-- 
2.16.0.rc0.223.g4a4ac83678-goog

___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml

Re: [xml] [PATCH] Check hex or decimal entity for overflow

[Nick Wellnhofer]

On 08/01/2018 02:06, Joel Hockey wrote:
The entity parsing code in tree.c is getting integer overflow when a very 
long, invalid hex (or decimal) entity is used:  e.g. #xabcdefabcdef;


This is probably the same issue as

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=3874

Also see

https://bugzilla.gnome.org/show_bug.cgi?id=783052

The issue only arises in "recovery" mode (XML_PARSE_RECOVER). In the past, I 
tried to fix similar issues by not adding nodes containing invalid character 
references at all in an earlier stage of the parsing code, but I'm fine with 
your approach.


For these cases, I am setting the error to XML_TREE_UNTERMINATED_ENTITY.  The 
other 2 existing codes are XML_TREE_INVALID_HEX, XML_TREE_INVALID_DEC.  I 
thought unterminated is the better choice, but maybe a new code such as 
XML_TREE_INVALID_CHAR could be used.


Regarding the error code, we could simply use XML_ERR_INVALID_CHAR or not 
report an error at all since invalid numeric character references are already 
detected and reported earlier.


Nick
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml

[xml] [PATCH] Check hex or decimal entity for overflow

[Joel Hockey]

This is another fuzzing bug from chromium.

The entity parsing code in tree.c is getting integer overflow when a very
long, invalid hex (or decimal) entity is used:  e.g. #xabcdefabcdef;

For these cases, I am setting the error to XML_TREE_UNTERMINATED_ENTITY.
The other 2 existing codes are XML_TREE_INVALID_HEX, XML_TREE_INVALID_DEC.
I thought unterminated is the better choice, but maybe a new code such as
XML_TREE_INVALID_CHAR could be used.

See crbug.com/796804
From c3d07d925ad85d3a26a609bc544b388426255df4 Mon Sep 17 00:00:00 2001
From: Joel Hockey 
Date: Wed, 3 Jan 2018 18:52:36 -0800
Subject: [PATCH] Check hex or decimal entity for overflow

---
 tree.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/tree.c b/tree.c
index 959421bd..ab48909a 100644
--- a/tree.c
+++ b/tree.c
@@ -1527,6 +1527,12 @@ xmlStringGetNodeList(const xmlDoc *doc, const xmlChar *value) {
 			charval = 0;
 			break;
 		}
+		if (charval > 0x10) {
+			xmlTreeErr(XML_TREE_UNTERMINATED_ENTITY, (xmlNodePtr) doc,
+			   NULL);
+			charval = 0;
+			break;
+		}
 		cur++;
 		tmp = *cur;
 		}
@@ -1545,6 +1551,12 @@ xmlStringGetNodeList(const xmlDoc *doc, const xmlChar *value) {
 			charval = 0;
 			break;
 		}
+		if (charval > 0x10) {
+			xmlTreeErr(XML_TREE_UNTERMINATED_ENTITY, (xmlNodePtr) doc,
+			   NULL);
+			charval = 0;
+			break;
+		}
 		cur++;
 		tmp = *cur;
 		}
-- 
2.16.0.rc0.223.g4a4ac83678-goog

___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml