Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Email to this user bounced
Closed: Jan 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment
Heap-buffer-overflow in xmlStringLenDecodeEntities
Reported by a...@ut.ee, Dec 11 2011 Back to list
VULNERABILITY DETAILS
A buffer overflow occurs in libxml/src/parser.c:2589:xmlStringLenDecodeEntities when decoding an entity reference with a long name.

The code at parser.c:2589 is:

    if (nbchars > buffer_size - i - XML_PARSER_BUFFER_SIZE) {
        growBuffer(buffer, XML_PARSER_BUFFER_SIZE);
    }
    for (;i > 0;i--)
        buffer[nbchars++] = *cur++;

It checks whether additional i elements fit to the buffer, increases the buffer when necessary, and copies the data. The problem is that growBuffer grows the buffer by a fixed amount, so with enough data the overflow still occurs.

There is a similar case on line 3772, but growBuffer is handled correctly.

VERSION
Chrome Version: 18.0.969.0 (Developer Build 113953 Linux) custom
Operating System: Linux 2.6.32, Ubuntu 10.04

REPRODUCTION CASE
Open poc.xhtml in chromium.

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: tab
Crash state:

[I added printf("nbchars=%d, buffer_size=%d\n", nbchars, buffer_size); to the data copying loop.]

nbchars=2040, buffer_size=700
nbchars=2041, buffer_size=700
nbchars=2042, buffer_size=700
nbchars=2043, buffer_size=700
nbchars=2044, buffer_size=700
nbchars=2045, buffer_size=700
nbchars=2046, buffer_size=700
nbchars=2047, buffer_size=700
nbchars=2048, buffer_size=700

Program received signal SIGSEGV, Segmentation fault.
0xb28c8716 in DieFromMemoryCorruption ()
    at third_party/tcmalloc/chromium/src/tcmalloc.cc:1828
1828      *p += 3;  // Segv.
(gdb) x/1i $eip
=> 0xb28c8716 <DieFromMemoryCorruption+20>:     movzbl (%eax),%eax
(gdb) info reg
eax            0x3      3
ecx            0xb824cff4       -1205547020
edx            0x2e0e4420       772686880
ebx            0xb824cff4       -1205547020
esp            0xbfffd768       0xbfffd768
ebp            0xbfffd778       0xbfffd778
esi            0xffffef3e       -4290
edi            0xb834a280       -1204510080
eip            0xb28c8716       0xb28c8716 <DieFromMemoryCorruption+20>
eflags         0x210206 [ PF IF RF ID ]
cs             0x73     115
ss             0x7b     123
ds             0x7b     123
es             0x7b     123
fs             0x0      0
gs             0x33     51
(gdb) bt
#0  0xb28c8716 in DieFromMemoryCorruption ()
    at third_party/tcmalloc/chromium/src/tcmalloc.cc:1828
#1  0xb28c88e9 in ValidateAllocatedRegion (ptr=0xb8391080, cl=26)
    at third_party/tcmalloc/chromium/src/tcmalloc.cc:1921
#2  0xb28c7b56 in do_free_with_callback (ptr=0xb8391080, 
    invalid_free_fn=0xb28c60e4 <InvalidFree>)
    at third_party/tcmalloc/chromium/src/tcmalloc.cc:1221
#3  0xb28c7d7e in do_free (ptr=0xb8391080)
    at third_party/tcmalloc/chromium/src/tcmalloc.cc:1249
#4  0xb62ab827 in tc_free ()
#5  0xb450e63e in xmlParseAttValueComplex (ctxt=0xb837de00, attlen=0xbfffda04, 
    normalize=0) at third_party/libxml/src/parser.c:3759
#6  0xb451b7bf in xmlParseAttValueInternal (ctxt=0xb837de00, len=0xbfffda04, 
    alloc=0xbfffda00, normalize=0) at third_party/libxml/src/parser.c:8600
#7  0xb451b90a in xmlParseAttribute2 (ctxt=0xb837de00, pref=0x0, 
    elem=0xb857fa26 "body", prefix=0xbfffda50, value=0xbfffda48, 
    len=0xbfffda04, alloc=0xbfffda00) at third_party/libxml/src/parser.c:8656
#8  0xb451bd98 in xmlParseStartTag2 (ctxt=0xb837de00, pref=0xbfffdaf4, 
    URI=0xbfffdaf0, tlen=0xbfffdb14) at third_party/libxml/src/parser.c:8814
#9  0xb4521ae7 in xmlParseTryOrFinish (ctxt=0xb837de00, terminate=0)
    at third_party/libxml/src/parser.c:10867
#10 0xb4523f3c in xmlParseChunk (ctxt=0xb837de00, chunk=0xb857d05a "S", 
    size=8578, terminate=0) at third_party/libxml/src/parser.c:11645
(more stack frames follow)
 
poc.xhtml
4.2 KB View Download
Labels: -Pri-0 -Area-Undefined Pri-1 Area-WebKit SecSeverity-High SecImpacts-Stable
Owner: cevans@chromium.org
Status: Assigned
Affects all versions. Overflow corrupts the heap. I've found that sometimes the overflow in the PoC will not immediately crash the renderer, but causing the renderer to use the heap some more (eg. by clicking anywhere on the page) will trigger an exception.

There is an obvious fix for this if you compare the bad code with line 3772:
BAD @ 2589:
		if (nbchars > buffer_size - i - XML_PARSER_BUFFER_SIZE) {
		    growBuffer(buffer, XML_PARSER_BUFFER_SIZE);
		}
GOOD @ 3772:
		    while (len > buf_size - i - 10) {
			growBuffer(buf, i + 10);
		    }


@Chris: do you want to take this one?
I just confirmed that changing if to while seems to fix the issue as expected.

This may have been a bad copy of code from elsewhere in the code of parser.c:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/libxml/src/parser.c&q=growBuffer&exact_package=chromium
Maybe we should have a look to make sure this problem doesn't affect other parts of the code?
Comment 3 by a...@ut.ee, Dec 11 2011
I spent some time with parser.c and HTMLparser.c. Couldn't find anymore obvious overflows though.
Labels: reward-topanel
@skylined: are you sure you meant to assign to me? For obvious reasons...
Owner: ----
Status: Available
Heh, no! You just relax and change nappies :)
Labels: Mstone-16
@skylined: it seems like you have a fix and it is fairly simple? How about cutting out the middle man and uploading the patch yourself? Make sure to change README.chromium too to describe the patch (see http://src.chromium.org/viewvc/chrome?view=rev&revision=100953 for an example of exactly what to do).

Also, please paste the patch inline into this bug (since it is small) and then cc: veillard@gmail.com who is the upstream libxml maintainer.
Summary: Heap-buffer-overflow in xmlStringLenDecodeEntities (was: NULL)
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4427213

Uploader: inferno@chromium.org

Crash Type: Heap-buffer-overflow WRITE 1
Crash Address: 0x7f2aa98bff3c
Crash State:
  - crash stack -
  xmlStringLenDecodeEntities
  xmlParseAttValueInternal
  xmlParseStartTag2
  

Minimized Testcase (4.19 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97-qX4BNSV9PsAq_IiXhViKBbi422WIm0RA86ifgLRtKimQ8fxdzNmI4zYwbT1Vc8h9Cm0HXLbehUAXu6M_nQF4gW1M7UDh8v0MzHLYjelr4-Fenz4v1anJ10lVe0SY4UEJt4yYohkZKRpTsJ00wSO4Wrqnsg
Cc: veill...@gmail.com
Labels: OS-All
Comment 10 by veill...@gmail.com, Dec 14 2011
Hum,

do you have a CVE for this ? I'm trying to see if that's something we
tracked independently,
it seems not, but a CVE would help :-)
Problem identification looks right, but I need to double check

  thanks for the heads-up !

Daniel
Comment 11 by veill...@gmail.com, Dec 14 2011
I just looked and we should avoid the while, an if is better in this case
as an allocation error in growBuffer leads to a proper failure. we just need
to allocate the amount we computed as being needed.

Daniel

diff --git a/parser.c b/parser.c
index 4e5dcb9..c55e41d 100644
--- a/parser.c
+++ b/parser.c
@@ -2709,7 +2709,7 @@ xmlStringLenDecodeEntities(xmlParserCtxtPtr ctxt, const xm
 
                buffer[nbchars++] = '&';
                if (nbchars > buffer_size - i - XML_PARSER_BUFFER_SIZE) {
-                   growBuffer(buffer, XML_PARSER_BUFFER_SIZE);
+                   growBuffer(buffer, i + XML_PARSER_BUFFER_SIZE);
                }
                for (;i > 0;i--)
                    buffer[nbchars++] = *cur++;

Labels: CVE-2011-3919
@veillard - Sorry, I just saw your request. I've allocated CVE-2011-3919 for you.
Comment 13 by veill...@gmail.com, Dec 15 2011
Could someone verify in your framework that the patch in comment #11 fixes it for
you ? It should and avoid looping over realloc (in growBuffer) which is often a
costly operation

  thanks,

Daniel
Daniel, i verified that your fix works perfectly with ASAN and stops the buffer overflow.
Comment 15 by veill...@gmail.com, Dec 16 2011
Cool, thanks :-) !

Daniel
Comment 16 by veill...@gmail.com, Dec 16 2011
Cool, thanks :-) !

Daniel
Thanks Daniel, that is a lot better. Looking again at the code in comment #1, that "growBuffer" call inside the "while" that I copied from makes no sense - it will always be false on the second loop, so an if should suffice there. There are a few more places like it where "while" is used where "if" would be just as good.

In an attempt to prevent future regressions and/or similar mistakes, I'd like to fix this by introducing a new macro that does both the check and grow. I've replaced all existing code that does a check+grow with a call to this macro. This fixes the buffer overflow and the unneeded "while" loops. This macro will make sure the check and the grow always use the same size. Thoughts?



parser.patch
4.4 KB View Download
@skylined: is the fix you and Daniel settled on landed in Chromium yet?
I want this fixed in the Chrome 16 stable update, which is coming up really soon.
I had a chat with Anthony and we need to take care all the merges for m16 by this Tuesday (Jan 3rd). For a m16 patch that hasn't baked on trunk, i would prefer to go with Daniel's one word patch in c#11 and then we can do the skylined's patch as a followup. Skylined, do you have time to submit a chromium patch (similar to other libxml patches) and committed on tuesday ?
Yes, let's just use the upstream patch. It's simple and risk-free.

It was already committed upstream:
http://git.gnome.org/browse/libxml2/commit/?id=5bd3c061823a8499b27422aee04ea20aae24f03e

So all we need to do is copy the 3-character addition into our own libxml source tree and update README.chromium to reference the above upstream commit URL.

I would have done this myself last night, but it seems I powered off my work Linux machine for my absence.
Owner: cevans@chromium.org
Status: Assigned
I can get this all done on Tuesday, assuming the merge deadline is the usual 7pm Tuesday.
Yes, Anthony said end of day for the deadline.
Ok. I'll kick by the office to say hi and pick up some stuff and take care of it then :)
Project Member Comment 24 by bugdroid1@chromium.org, Jan 3 2012
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=116175

------------------------------------------------------------------------
r116175 | cevans@chromium.org | Tue Jan 03 11:53:21 PST 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/libxml/src/parser.c?r1=116175&r2=116174&pathrev=116175
 M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/libxml/README.chromium?r1=116175&r2=116174&pathrev=116175

Pull entity fix from upstream.

BUG= 107128 
Review URL: http://codereview.chromium.org/9072008
------------------------------------------------------------------------
Labels: Merge-Approved
Status: FixUnreleased
Project Member Comment 26 by bugdroid1@chromium.org, Jan 3 2012
Labels: merge-merged-912
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=116213

------------------------------------------------------------------------
r116213 | cevans@chromium.org | Tue Jan 03 14:03:07 PST 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/912/src/third_party/libxml/README.chromium?r1=116213&r2=116212&pathrev=116213
 M http://src.chromium.org/viewvc/chrome/branches/912/src/third_party/libxml/src/parser.c?r1=116213&r2=116212&pathrev=116213

Merge 116175 - Pull entity fix from upstream.

BUG= 107128 
Review URL: http://codereview.chromium.org/9072008

TBR=cevans@chromium.org
Review URL: http://codereview.chromium.org/9014028
------------------------------------------------------------------------
Project Member Comment 27 by bugdroid1@chromium.org, Jan 3 2012
Labels: merge-merged-963
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=116214

------------------------------------------------------------------------
r116214 | cevans@chromium.org | Tue Jan 03 14:04:38 PST 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/963/src/third_party/libxml/src/parser.c?r1=116214&r2=116213&pathrev=116214
 M http://src.chromium.org/viewvc/chrome/branches/963/src/third_party/libxml/README.chromium?r1=116214&r2=116213&pathrev=116214

Merge 116175 - Pull entity fix from upstream.

BUG= 107128 
Review URL: http://codereview.chromium.org/9072008

TBR=cevans@chromium.org
Review URL: http://codereview.chromium.org/9072018
------------------------------------------------------------------------
Labels: -Restrict-View-SecurityTeam -Merge-Approved Restrict-View-SecurityNotify Merge-Merged
@asd@ut.ee: what name should we use for credit in our release notes / hall of fame?
Oops, I didn't realized there was something pending on this.
I had commited the minimal patch in libvirt git but it's not skylined' version.
I just looked at his patch and it's fine too, it's just a code cleanup IMHO does
not change semantic. So either way the build should be fine.

Daniel
Comment 30 by a...@ut.ee, Jan 4 2012
@scarybeasts: Please use 'Jüri Aedla'.
Labels: -Area-WebKit
Labels: Area-Internals
Labels: -reward-topanel reward-1000 reward-unpaid
Thank you Jüri!

And also congratulations! This is a very nice bug, and reported well, so it certainly qualifies for a $1000 Chromium Security Reward.

With any luck, we'll be able to get the fix to users tomorrow.
Comment 34 by a...@ut.ee, Jan 5 2012
Thanks!
Labels: -reward-unpaid
Comment 36 by cdn@chromium.org, May 15 2012
Status: Fixed
Marking old security bugs Fixed..
Cc: happyaro...@gmail.com
CC'ing Debian libxml maintainer.
Labels: -Restrict-View-SecurityNotify
Labels: -reward-1000 reward-4000 reward-unpaid
Increasing reward by $3000 to $4000 as per http://blog.chromium.org/2012/08/chromium-vulnerability-rewards-program.html

Labels: -reward-unpaid
Top-up reward paid as part of a $4000 batch.
Project Member Comment 41 by bugdroid1@chromium.org, Oct 14 2012
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member Comment 42 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Type-Security -SecSeverity-High -SecImpacts-Stable -Mstone-16 -Area-Internals Security-Impact-Stable Cr-Internals Security-Severity-High Type-Bug-Security M-16
Project Member Comment 43 by bugdroid1@chromium.org, Mar 13 2013
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Project Member Comment 44 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Severity-High Security_Severity-High
Project Member Comment 45 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member Comment 46 by clusterf...@chromium.org, Jun 13 2013
ClusterFuzz has detected this issue as fixed in range 116169:116185.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4427213

Uploader: inferno@chromium.org

Crash Type: Heap-buffer-overflow WRITE 1
Crash Address: 0x7f2aa98bff3c
Crash State:
  - crash stack -
  xmlStringLenDecodeEntities
  xmlParseAttValueInternal
  xmlParseStartTag2
  
Fixed: https://cluster-fuzz.appspot.com/revisions?range=116169:116185

Minimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv97-qX4BNSV9PsAq_IiXhViKBbi422WIm0RA86ifgLRtKimQ8fxdzNmI4zYwbT1Vc8h9Cm0HXLbehUAXu6M_nQF4gW1M7UDh8v0MzHLYjelr4-Fenz4v1anJ10lVe0SY4UEJt4yYohkZKRpTsJ00wSO4Wrqnsg

If you suspect that the result above is incorrect, try re-doing that job on the testcase report page.
Project Member Comment 47 by sheriffbot@chromium.org, Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 48 by sheriffbot@chromium.org, Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Sign in to add a comment