Re: [xml] Heap use after free in parser.c

[Jay Civelli via xml]

On Mon, Jan 22, 2018 at 7:53 AM, Nick Wellnhofer 
wrote:

> On 08/01/2018 22:43, Jay Civelli wrote:
>
>> On Mon, Jan 8, 2018 at 11:27 AM, Nick Wellnhofer > > wrote:
>>
>> On 02/01/2018 20:08, Jay Civelli via xml wrote:
>>
>> We ran into a heap use after free in Chromium
>> http://crbug.com/793715
>>  that I think I tracked down.
>>
>> I don't have access to this page.
>>
>> You should have access now.
>>
>
> I still don't have access to the original Clusterfuzz report. I only found
> your reduced test case "bad_xml" but I couldn't reproduce the issue with
> xmllint. Given the stack trace and Chromium sources, it seems that you're
> using xmlReaderForMemory in recovery mode:
>
>
> https://chromium.googlesource.com/chromium/src/+/master/thir
> d_party/libxml/chromium/libxml_utils.cc
>
> Note that it's discouraged to use XML_PARSE_RECOVER in production code.
> This flag hides errors in invalid XML and exercises some less-tested code
> paths in libxml2.
>
Thanks, I'll look into removing it.

>
> For future reports, it would be helpful to provide test cases that show
> the problem with xmllint. The following flags should make xmllint behave
> similar to the Chromium code in question:
>
> xmllint --stream --memory --recover file.xml

Got it, will do in the future.

>
>
> Good idea, done in new attached patch. Note that I changed the error from
>> the existing from XML_ERR_INVALID_ENCODING to XML_ERR_INVALID_CHAR which
>> seemed to make more sense.
>>
>
> I committed a minimal fix that only adds a call to xmlHaltParser.
>
Great, thanks. I tested locally, it does address the issue.

>
>
> https://git.gnome.org/browse/libxml2/commit/?id=ab362ab0ad3a
> f54406ae8237a525405c6e2a705b
>
> Nick
>
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml

Re: [xml] Heap use after free in parser.c

[Nick Wellnhofer]

On 08/01/2018 22:43, Jay Civelli wrote:
On Mon, Jan 8, 2018 at 11:27 AM, Nick Wellnhofer > wrote:


On 02/01/2018 20:08, Jay Civelli via xml wrote:

We ran into a heap use after free in Chromium http://crbug.com/793715
 that I think I tracked down.

I don't have access to this page.

You should have access now.


I still don't have access to the original Clusterfuzz report. I only found 
your reduced test case "bad_xml" but I couldn't reproduce the issue with 
xmllint. Given the stack trace and Chromium sources, it seems that you're 
using xmlReaderForMemory in recovery mode:



https://chromium.googlesource.com/chromium/src/+/master/third_party/libxml/chromium/libxml_utils.cc

Note that it's discouraged to use XML_PARSE_RECOVER in production code. This 
flag hides errors in invalid XML and exercises some less-tested code paths in 
libxml2.


For future reports, it would be helpful to provide test cases that show the 
problem with xmllint. The following flags should make xmllint behave similar 
to the Chromium code in question:


xmllint --stream --memory --recover file.xml

Good idea, done in new attached patch. Note that I changed the error from the 
existing from XML_ERR_INVALID_ENCODING to XML_ERR_INVALID_CHAR which seemed to 
make more sense.


I committed a minimal fix that only adds a call to xmlHaltParser.


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

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

Re: [xml] Heap use after free in parser.c

[Jay Civelli via xml]

Hi Nick,
Did you have a chance to look at my latest patch? (attached in my previous
email)

Thanks.

Jay

On Mon, Jan 8, 2018 at 1:43 PM, Jay Civelli  wrote:

> On Mon, Jan 8, 2018 at 11:27 AM, Nick Wellnhofer 
> wrote:
>
>> On 02/01/2018 20:08, Jay Civelli via xml wrote:
>>
>>> We ran into a heap use after free in Chromium http://crbug.com/793715 <
>>> http://crbug.com/793715> that I think I tracked down.
>>>
>>
>> I don't have access to this page.
>
> You should have access now.
>
>>
>>
>> I have a tentative patch attached to address it.
>>> In parser.c, if a call to xmlCharEncInput() fails and has grown the
>>> buffer, the ctxt object could still point to the old deleted buffer.
>>>
>>
>> Maybe it's better to call xmlHaltParser if xmlCharEncInput fails. That's
>> what the other code path in xmlParseChunk does.
>
> Good idea, done in new attached patch. Note that I changed the error from
> the existing from XML_ERR_INVALID_ENCODING to XML_ERR_INVALID_CHAR which
> seemed to make more sense.
>
> Jay
>
>
>
>>
>>
>> Nick
>>
>
>
___
xml mailing list, project page  http://xmlsoft.org/
xml@gnome.org
https://mail.gnome.org/mailman/listinfo/xml

Re: [xml] Heap use after free in parser.c

[Jay Civelli via xml]

On Mon, Jan 8, 2018 at 11:27 AM, Nick Wellnhofer 
wrote:

> On 02/01/2018 20:08, Jay Civelli via xml wrote:
>
>> We ran into a heap use after free in Chromium http://crbug.com/793715 <
>> http://crbug.com/793715> that I think I tracked down.
>>
>
> I don't have access to this page.

You should have access now.

>
>
> I have a tentative patch attached to address it.
>> In parser.c, if a call to xmlCharEncInput() fails and has grown the
>> buffer, the ctxt object could still point to the old deleted buffer.
>>
>
> Maybe it's better to call xmlHaltParser if xmlCharEncInput fails. That's
> what the other code path in xmlParseChunk does.

Good idea, done in new attached patch. Note that I changed the error from
the existing from XML_ERR_INVALID_ENCODING to XML_ERR_INVALID_CHAR which
seemed to make more sense.

Jay



>
>
> Nick
>
From 89632441fba22400cc5b1e413766aa2f32ff5f91 Mon Sep 17 00:00:00 2001
From: Jay Civelli 
Date: Mon, 8 Jan 2018 13:38:39 -0800
Subject: [PATCH] Fix heap use after free.

In parser.c, if a call to xmlCharEncInput() fails and has grown
the buffer, the ctxt object could still point to the old deleted
buffer.
Halt the parsing in such a case.
---
 parser.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/parser.c b/parser.c
index a30dd18e..823793c4 100644
--- a/parser.c
+++ b/parser.c
@@ -12214,9 +12214,9 @@ xmldecl_done:
 		nbchars = xmlCharEncInput(in, terminate);
 		if (nbchars < 0) {
 		/* TODO 2.6.0 */
-		xmlGenericError(xmlGenericErrorContext,
-"xmlParseChunk: encoder error\n");
-		return(XML_ERR_INVALID_ENCODING);
+ctxt->errNo = XML_ERR_INVALID_CHAR;
+xmlHaltParser(ctxt);
+return (XML_ERR_INVALID_CHAR);
 		}
 		xmlBufSetInputBaseCur(in->buffer, ctxt->input, base, current);
 	}
-- 
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] Heap use after free in parser.c

[Nick Wellnhofer]

On 02/01/2018 20:08, Jay Civelli via xml wrote:
We ran into a heap use after free in Chromium http://crbug.com/793715 
 that I think I tracked down.


I don't have access to this page.


I have a tentative patch attached to address it.
In parser.c, if a call to xmlCharEncInput() fails and has grown the buffer, 
the ctxt object could still point to the old deleted buffer.


Maybe it's better to call xmlHaltParser if xmlCharEncInput fails. That's what 
the other code path in xmlParseChunk does.


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

[xml] Heap use after free in parser.c

[Jay Civelli via xml]

Hi,
We ran into a heap use after free in Chromium http://crbug.com/793715 that
I think I tracked down. I have a tentative patch attached to address it.
In parser.c, if a call to xmlCharEncInput() fails and has grown the buffer,
the ctxt object could still point to the old deleted buffer.

Thanks.

Jay
From 178f65add031108cc13ee8446fb8a5bd5ad9be88 Mon Sep 17 00:00:00 2001
From: Jay Civelli 
Date: Thu, 28 Dec 2017 10:27:48 -0800
Subject: [PATCH] Fix heap use after free.

In parser.c, if a call to xmlCharEncInput() fails and has grown the
buffer, the ctxt object could still point to the old deleted buffer.
Make sure we always update the ctxt.
---
 parser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/parser.c b/parser.c
index a30dd18e..3a0b640b 100644
--- a/parser.c
+++ b/parser.c
@@ -12212,13 +12212,13 @@ xmldecl_done:
 		size_t current = ctxt->input->cur - ctxt->input->base;
 
 		nbchars = xmlCharEncInput(in, terminate);
+		xmlBufSetInputBaseCur(in->buffer, ctxt->input, base, current);
 		if (nbchars < 0) {
 		/* TODO 2.6.0 */
 		xmlGenericError(xmlGenericErrorContext,
 "xmlParseChunk: encoder error\n");
 		return(XML_ERR_INVALID_ENCODING);
 		}
-		xmlBufSetInputBaseCur(in->buffer, ctxt->input, base, current);
 	}
 	}
 }
-- 
2.15.1.620.gb9897f4670-goog

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