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 2 users

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Dec 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 xsltApplyTemplates

Reported by nicolas....@agarri.fr, Jul 24 2012

Issue description

VULNERABILITY DETAILS

When applying templates to nodes selected by "namespace::*", a out-of-bounds read is performed. Later, this value is used during unlinking of nodes, leading to a WRITE error in xmlUnlinkNode().

VERSION

xsltproc: libxml 20706, libxslt 10126 and libexslt 815
Chromium: 18.0.1025.151 (Dev 130497 Linux) Ubuntu 10.04
Chromium+ASan: 21.0.1180.49 (147161)

REPRODUCTION CASE

<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="1.0" >
<xsl:template match="*">
	<xsl:for-each select="namespace::*">
		<xsl:apply-templates/>
	</xsl:for-each>
</xsl:template>
</xsl:stylesheet>

ADDITIONAL INFORMATION

Valgrind + xsltproc:

==5547== Invalid read of size 4
==5547==    at 0x40E8C03: xsltApplyTemplates (transform.c:4837)
==5547==    by 0x40E5FA6: xsltApplySequenceConstructor (transform.c:2595)
==5547==    by 0x40E6A4C: xsltForEach (transform.c:5628)
==5547==    by 0x40E5FA6: xsltApplySequenceConstructor (transform.c:2595)
==5547==    by 0x40E75E1: xsltApplyXSLTTemplate (transform.c:3044)
==5547==    by 0x40E7E41: xsltProcessOneNode (transform.c:2045)
==5547==    by 0x40E83E9: xsltProcessOneNode (transform.c:1875)
==5547==    by 0x40EB8D9: xsltApplyStylesheetInternal (transform.c:6049)
==5547==    by 0x8049E11: xsltProcess (xsltproc.c:404)
==5547==    by 0x804A866: main (xsltproc.c:867)
==5547==  Address 0x43f90fc is 0 bytes after a block of size 4 alloc'd
==5547==    at 0x4024F20: malloc (vg_replace_malloc.c:236)
==5547==    by 0x41A85FC: xmlStrndup (xmlstring.c:45)
==5547==    by 0x41A86DF: xmlStrdup (xmlstring.c:71)
==5547==    by 0x417EB2B: xmlXPathNodeSetDupNs (xpath.c:3388)
==5547==    by 0x418072F: xmlXPathNodeSetAddNs (xpath.c:3578)
==5547==    by 0x418EC7D: xmlXPathNodeCollectAndTest (xpath.c:12421)
==5547==    by 0x418C3FB: xmlXPathCompOpEval (xpath.c:13375)
==5547==    by 0x418C681: xmlXPathCompOpEval (xpath.c:13862)
==5547==    by 0x418EE11: xmlXPathRunEval (xpath.c:14432)
==5547==    by 0x418F438: xmlXPathCompiledEvalInternal (xpath.c:14792)
==5547==    by 0x418F655: xmlXPathCompiledEval (xpath.c:14855)
==5547==    by 0x40E68E1: xsltForEach (transform.c:5531)
[...]
==5547== Invalid read of size 4
==5547==    at 0x4150901: xmlUnlinkNode (tree.c:3783)
==5547==    by 0x40E8BEC: xsltApplyTemplates (transform.c:4898)
==5547==    by 0x40E5FA6: xsltApplySequenceConstructor (transform.c:2595)
==5547==    by 0x40E6A4C: xsltForEach (transform.c:5628)
==5547==    by 0x40E5FA6: xsltApplySequenceConstructor (transform.c:2595)
==5547==    by 0x40E75E1: xsltApplyXSLTTemplate (transform.c:3044)
==5547==    by 0x40E7E41: xsltProcessOneNode (transform.c:2045)
==5547==    by 0x40E83E9: xsltProcessOneNode (transform.c:1875)
==5547==    by 0x40EB8D9: xsltApplyStylesheetInternal (transform.c:6049)
==5547==    by 0x8049E11: xsltProcess (xsltproc.c:404)
==5547==    by 0x804A866: main (xsltproc.c:867)
==5547==  Address 0x43f9110 is not stack'd, malloc'd or (recently) free'd
==5547== 
==5547== Invalid write of size 4
==5547==    at 0x4150904: xmlUnlinkNode (tree.c:3783)
==5547==    by 0x40E8BEC: xsltApplyTemplates (transform.c:4898)
==5547==    by 0x40E5FA6: xsltApplySequenceConstructor (transform.c:2595)
==5547==    by 0x40E6A4C: xsltForEach (transform.c:5628)
==5547==    by 0x40E5FA6: xsltApplySequenceConstructor (transform.c:2595)
==5547==    by 0x40E75E1: xsltApplyXSLTTemplate (transform.c:3044)
==5547==    by 0x40E7E41: xsltProcessOneNode (transform.c:2045)
==5547==    by 0x40E83E9: xsltProcessOneNode (transform.c:1875)
==5547==    by 0x40EB8D9: xsltApplyStylesheetInternal (transform.c:6049)
==5547==    by 0x8049E11: xsltProcess (xsltproc.c:404)
==5547==    by 0x804A866: main (xsltproc.c:867)
==5547==  Address 0x50 is not stack'd, malloc'd or (recently) free'd
==5547== 
==5547== Process terminating with default action of signal 11 (SIGSEGV)
==5547==  Access not within mapped region at address 0x50
==5547==    at 0x4150904: xmlUnlinkNode (tree.c:3783)
==5547==    by 0x40E8BEC: xsltApplyTemplates (transform.c:4898)
==5547==    by 0x40E5FA6: xsltApplySequenceConstructor (transform.c:2595)
==5547==    by 0x40E6A4C: xsltForEach (transform.c:5628)
==5547==    by 0x40E5FA6: xsltApplySequenceConstructor (transform.c:2595)
==5547==    by 0x40E75E1: xsltApplyXSLTTemplate (transform.c:3044)
==5547==    by 0x40E7E41: xsltProcessOneNode (transform.c:2045)
==5547==    by 0x40E83E9: xsltProcessOneNode (transform.c:1875)
==5547==    by 0x40EB8D9: xsltApplyStylesheetInternal (transform.c:6049)
==5547==    by 0x8049E11: xsltProcess (xsltproc.c:404)
==5547==    by 0x804A866: main (xsltproc.c:867)

Asan + Chromium:

==2107== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f88d1258a88 at pc 0x7f88edfe963c bp 0x7fff8b4607d0 sp 0x7fff8b4607c8
READ of size 4 at 0x7f88d1258a88 thread T0

    #0  0000000008a9763c <xsltApplyTemplates+0xdec>:
 8a9763c:	e8 ef fc 80 00       	callq  92a7330 <__asan_report_load8>
    #1  xsltApplySequenceConstructor+0xdde
    #2  xsltForEach+0xb61
    #3  xsltApplySequenceConstructor+0xdde
    #4  xsltApplyXSLTTemplate+0xeab
    #5  xsltProcessOneNode+0x156f
    #6  xsltProcessOneNode+0xc87
    #7  xsltApplyStylesheetInternal+0x9d4
[...]
0x7f88d1258a88 is located 4 bytes to the right of 4-byte region [0x7f88d1258a80,0x7f88d1258a84)
 
asan-symbols.txt
5.0 KB View Download
Summary: Heap-buffer-overflow in xsltApplyTemplates
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=83226001

Uploader: jschuh@chromium.org

Crash Type: Heap-buffer-overflow READ 4
Crash Address: 0x7f544606ed88
Crash State:
  - crash stack -
  xsltApplyTemplates
  xsltApplySequenceConstructor
  xsltForEach
  

Minimized Testcase (0.41 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94KGD8W2094yk5g_XEprgUzBFLozALO60abObNq38PZ1qNPORy5dMrre7AXO5lkLnyyNaDXkKTQnEe9JqENpaVdzvQQRnzuAW89F6_mwNxX6k4RpmFtvgsrWrodiaI9EkPbQ9lXapGH3DTGJN6hkAlOl0Jfn8WrhcyLltsnDLY2H6miJmE
Cc: veill...@gmail.com
Labels: -Pri-0 -Area-Undefined Pri-1 Area-Internals SecSeverity-Medium SecImpacts-Stable SecImpacts-Beta OS-All Mstone-20 Stability-AddressSanitizer Stability-Valgrind
Status: Available
This READ error leads to tree corruption and invalid WRITE access (as seen in the Valgrind trace). Severity = Medium ?
Labels: -SecSeverity-Medium SecSeverity-High
Labels: -Mstone-20 Mstone-21 reward-topanel
Owner: cevans@chromium.org
Status: Assigned
Labels: Security-CodeYellow
Please do read Mark's email titled "Code Yellow: Security Bug Backlog" on chrome-team mailing list.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 3 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=149930

------------------------------------------------------------------------
r149930 | cevans@chromium.org | 2012-08-03T21:44:45.924877Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/libxml/src/include/libxml/tree.h?r1=149930&r2=149929&pathrev=149930
   M http://src.chromium.org/viewvc/chrome/trunk/src/third_party/libxml/README.chromium?r1=149930&r2=149929&pathrev=149930

Fix namespace vs. node type issue in a generic way.

BUG= 138673 
Review URL: https://chromiumcodereview.appspot.com/10824157
------------------------------------------------------------------------
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: FixUnreleased
Ok, this is "fixed". Daniel -- the fix is both clever and foul at the same time, so you probably want to fix it properly rather than taking it up! The reason I've done it this way in Chromium is that it's defensive against other instances of the same problem:

- Grabbing a node, cast to generic node type.
- Accessing node->children without checking the node type is suitable.

Particularly, a namespace node type is not suitable because node->children corresponds to a different pointer that is definitely not a list of children -- boom! :)

The correct fix is to audit uses of node->children to make sure node->type has always been checked first.

My hack is to make the "namespace node" structure always have a NULL children field, if it should ever have an inappropriate lookup of node->children after a forced cast to a generic node. Ugly but should be effective at catching _every_ instance of this bug.
Project Member

Comment 9 by ClusterFuzz, Aug 4 2012

ClusterFuzz has detected this issue as fixed in range 149909:149937.

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

Uploader: jschuh@chromium.org

Crash Type: Heap-buffer-overflow READ 4
Crash Address: 0x7f544606ed88
Crash State:
  - crash stack -
  xsltApplyTemplates
  xsltApplySequenceConstructor
  xsltForEach
  
Fixed: https://cluster-fuzz.appspot.com/revisions?range=149909:149937

Minimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv94KGD8W2094yk5g_XEprgUzBFLozALO60abObNq38PZ1qNPORy5dMrre7AXO5lkLnyyNaDXkKTQnEe9JqENpaVdzvQQRnzuAW89F6_mwNxX6k4RpmFtvgsrWrodiaI9EkPbQ9lXapGH3DTGJN6hkAlOl0Jfn8WrhcyLltsnDLY2H6miJmE

If you suspect that the result above is incorrect, try re-doing that job on the testcase report page.
Fix looks broken to me, it change the node size for no good reason. The bug is in
libxslt, the fix is there
http://git.gnome.org/browse/libxslt/commit/?id=937ba2a3eb42d288f53c8adc211bd1122869f0bf

and to not fail on xmlUnlinkNode for namespace node which is also a bug
http://git.gnome.org/browse/libxml2/commit/?id=6ca24a39d0eb7fd7378a5bc8be3286bf745a36ba

Adding a string named children in a namespace node is really not a proper fix for this issue,

Daniel


Thank you Daniel. Yeah, I acknowledged the foulness of the fix above.

Thanks for the proper fix. Do you think there are any other places where node->children is looked at without first checking for XML_NAMESPACE_DECL ?
Well, yes that's something to double check. We already had the problem of
not checking for the type somewhere in libxslt compilation, so doing
a generic pass over the code is a good idea. Added to my TODO for
2.9.0 ...

Daniel
Labels: -reward-topanel reward-1000 reward-unpaid
@nicolas.gregoire: nice bug find (because others have certainly fuzzed this area too).
Happy to reward you a $1000 Chromium Security Reward :D
Thanks!
Project Member

Comment 15 by bugdroid1@chromium.org, Aug 24 2012

Labels: -Merge-Approved merge-merged-1180
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=153287

------------------------------------------------------------------------
r153287 | cevans@chromium.org | 2012-08-24T21:13:21.521707Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1180/src/third_party/libxml/README.chromium?r1=153287&r2=153286&pathrev=153287
   M http://src.chromium.org/viewvc/chrome/branches/1180/src/third_party/libxml/src/include/libxml/tree.h?r1=153287&r2=153286&pathrev=153287

Merge 149930 - Fix namespace vs. node type issue in a generic way.

BUG= 138673 
Review URL: https://chromiumcodereview.appspot.com/10824157

TBR=cevans@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10876070
------------------------------------------------------------------------
Labels: CVE-2012-2871
Just a followup for record, I added various extra note type checks
following this issue:

libxslt
http://git.gnome.org/browse/libxslt/commit/?id=1564b30e994602a95863d9716be83612580a2fed
libexslt:
http://git.gnome.org/browse/libxslt/commit/?id=24653072221e76d2f1f06aa71225229b532f8946

Daniel
Thanks Daniel for these patches!

By the way, the Git and Chromium versions of libxslt are more and more desynchronized. Is there a plan to release a stable version of libxslt, including fixes for all the recent tickets (both here and on the Gnome bug-tracker) ?
Hopefully i will be able to make a libxslt release this week ... idem
for libxml2 !!!

Daniel
Labels: -reward-unpaid
Paid as part of a $1000 batch.

Comment 21 by veill...@gmail.com, Sep 12 2012

BTW the new libxml2-2.9.0 and libxslt-1.1.27 releases are out ...

Daniel
Project Member

Comment 22 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.
Status: Fixed
Project Member

Comment 24 by bugdroid1@chromium.org, Jan 18 2013

Labels: Restrict-View-EditIssue
Restrict-View-EditIssue is preferred since it allows anyone who can edit an issue (committers and contributors) to view the bug.
Project Member

Comment 25 by bugdroid1@chromium.org, Jan 18 2013

Restrict-View-EditIssue is preferred since it allows anyone who can edit an issue (committers and contributors) to view the bug.
Project Member

Comment 26 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Type-Security -Area-Internals -SecSeverity-High -SecImpacts-Stable -SecImpacts-Beta -Mstone-21 -Stability-AddressSanitizer -Stability-Valgrind Security-Impact-Stable Security-Impact-Beta Performance-Valgrind M-21 Cr-Internals Security-Severity-High Type-Bug-Security Performance-Memory-AddressSanitizer
Project Member

Comment 27 by bugdroid1@chromium.org, Mar 14 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Labels: -Restrict-View-SecurityNotify -Restrict-View-EditIssue
Project Member

Comment 29 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Severity-High Security_Severity-High
Project Member

Comment 30 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 31 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member

Comment 32 by bugdroid1@chromium.org, Apr 1 2013

Labels: -Performance-Memory-AddressSanitizer Stability-Memory-AddressSanitizer
Project Member

Comment 33 by bugdroid1@chromium.org, Apr 1 2013

Labels: -Performance-Valgrind Stability-Valgrind
Project Member

Comment 34 by sheriffbot@chromium.org, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 35 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 36 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
Labels: CVE_description-submitted

Sign in to add a comment