New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 676623 link

Starred by 2 users

Issue metadata

Status: Fixed
Last visit > 30 days ago
Closed: Feb 2017
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Sign in to add a comment

Security: libxslt generation of text nodes integer overflow

Reported by, Dec 22 2016

Issue description


With a xslt file which generates big text nodes it is possible to trigger an integer overflow in xsltAddTextString in transform.c of the libxslt library. The function handles the memory management if new data are added to a text node. The vulnerability exists because there is no check for overflow while calculating a new size of a buffer to hold the data of a text node. The issue can be exploited to trigger an out of bounds write.

A simple fix would limit the max size of a text node to a sane value.
A mitigation would it be to use a size_checked_realloc like in libxml in libxslt.

Chrome Version: 57.0.2953.0 + beta
Operating System: Ubuntu 16.04.1 LTS

Please include a demonstration of the security bug, such as an attached
HTML or binary file that reproduces the bug when loaded in Chrome. PLEASE
make the file as small as possible and remove any content not required to
demonstrate the bug.

Type of crash: [tab, browser, etc.]
Crash State: [see link above: stack trace, registers, exception record]
Client ID (if relevant): [see link above]


Comment 1 by, Dec 22 2016

Type of crash: tab
Crash State:

==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fd7a76a2000 at pc 0x564695670354 bp 0x7ffd5c6a4020 sp 0x7ffd5c6a37d0
WRITE of size 5614304 at 0x7fd7a76a2000 thread T0 (chrome)

    #0 0x564695670353 in __asan_memcpy ??:?
    #1 0x7fd93b807573 in xsltAddTextString ./out/Debug/../../third_party/libxslt/libxslt/transform.c:837
    #2 0x7fd93b806cd9 in xsltCopyTextString ./out/Debug/../../third_party/libxslt/libxslt/transform.c:929
    #3 0x7fd93b81f988 in xsltValueOf ./out/Debug/../../third_party/libxslt/libxslt/transform.c:4540
    #4 0x7fd93b80e26d in xsltApplySequenceConstructor ./out/Debug/../../third_party/libxslt/libxslt/transform.c:2766
    #5 0x7fd93b80c4e0 in xsltApplyXSLTTemplate ./out/Debug/../../third_party/libxslt/libxslt/transform.c:3204
    #6 0x7fd93b82101a in xsltCallTemplate ./out/Debug/../../third_party/libxslt/libxslt/transform.c:4780
    #7 0x7fd93b80e26d in xsltApplySequenceConstructor ./out/Debug/../../third_party/libxslt/libxslt/transform.c:2766
    #8 0x7fd93b825857 in xsltIf ./out/Debug/../../third_party/libxslt/libxslt/transform.c:5390
    #9 0x7fd93b80e26d in xsltApplySequenceConstructor ./out/Debug/../../third_party/libxslt/libxslt/transform.c:2766
    #10 0x7fd93b80c4e0 in xsltApplyXSLTTemplate ./out/Debug/../../third_party/libxslt/libxslt/transform.c:3204

0x7fd7a76a2000 is located 6144 bytes to the left of 5616152-byte region [0x7fd7a76a3800,0x7fd7a7bfea18)

Comment 2 by, Dec 22 2016

sorry for splitted report
2.3 KB View Download

Comment 3 Deleted

Project Member

Comment 4 by ClusterFuzz, Dec 22 2016

ClusterFuzz is analyzing your testcase. Developers can follow the progress at
Components: Blink>XML
Labels: Stability-Memory-AddressSanitizer Security_Severity-Medium Security_Impact-Stable OS-All Pri-1
Status: Assigned (was: Unconfirmed)
Hi Dominic,

Feel free to adjust labels, or reassign as appropriate.

Project Member

Comment 6 by, Dec 23 2016

Labels: M-56
Project Member

Comment 7 by ClusterFuzz, Dec 28 2016

ClusterFuzz is analyzing your testcase. Developers can follow the progress at
Labels: -Security_Severity-Medium Security_Severity-High
Project Member

Comment 9 by ClusterFuzz, Dec 29 2016

ClusterFuzz is analyzing your testcase. Developers can follow the progress at
Project Member

Comment 10 by, Jan 5 2017

dominicc: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit - Your friendly Sheriffbot
Status: Started (was: Assigned)
hofusec, can I CC you on the upstream libxslt bug?

Comment 17 by, Jan 12 2017

Nick Wellnhofer contributed a simpler, better patch upstream at see patch 2 for posterity.
Note: Per upstream request, we need to ping the upstream bug when we land something.
Status: Fixed (was: Started)
This should be fixed.
Project Member

Comment 22 by, Feb 1 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 23 by, Feb 3 2017

Labels: Merge-Request-57
Project Member

Comment 24 by, Feb 3 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit - Your friendly Sheriffbot
Please merge your change to M57 branch 2987 before 5:00 PM Pt, Monday (02/06/) so we can pick it up for next week Beta release. Thank you.
Project Member

Comment 26 by, Feb 6 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit - Your friendly Sheriffbot
Please merge your change to M57 branch 2987 before 5:00 PM PT, Tuesday (02/07/17) so we can pick it up for next Beta release. Thank you.
Labels: reward-topanel
Labels: Merge-Merged
I'm not sure why a bot did not comment here, but I merged this yesterday:

Merged to 57.0.2987.33 (as 3b4c1d91...).

If someone could please sanity check me on that :)
Labels: -Merge-Approved-57 merge-merged-2987
Per comment #29, this is already merged to M57 branch 2987 and I can see the merge here:
Thanks for the CC folks.  Tracking this at Apple with <rdar://problem/30398908>.

Labels: -reward-topanel reward-unpaid reward-3000
Nice one!  The panel decided to award $3,000 for this report.  A member of our finance team will be in touch shortly to arrange payment.

*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an established charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.

Labels: -reward-unpaid reward-inprocess
Labels: -Hotlist-Merge-Approved
Has a CVE-ID been assigned to this issue?  Apple wants to mention the CVE-ID in an upcoming security bulletin.

Andrew, did it get a CVE?
Labels: CVE-2017-5029
Yep, it's CVE-2017-5029. Thanks for the ping.

Comment 39 by, Mar 2 2017

Cool, thanks for the fast response!
Labels: Release-0-M57
Labels: CVE-2017-5033
Wait, two CVEs for a single bug? I've pinged Andrew to ensure that this is correct.
Labels: -CVE-2017-5033
Good spot - overly zealous automation :-(
Labels: -Restrict-View-SecurityNotify allpublic
Removing view restrictions given #44

Comment 46 Deleted

In the patch I see

+        if (len >= INT_MAX - ctxt->lasttuse) {
+            xsltTransformError(ctxt, NULL, target,
+                "xsltCopyText: text allocation failed\n");
+            return(NULL);
+        }
+        minSize = ctxt->lasttuse + len + 1;

So shouldn't the condition be

    if (len >= INT_MAX - ctxt->lasttuse - 1)

Or, moreover, maybe ctxt->lasttuse == INT_MAX, so a better condition could be
    if (ctxt->lasttuse == INT_MAX || len >= INT_MAX - ctxt->lasttuse - 1)
Yeah, that needs to account for the + 1. I have filed  Issue 716967 .
See the other issue but ddkilzer points out that the existing code is OK because of the >= check, and as you point out using a > check has the added complication of lasttuse being INT_MAX that >= does not have.
The only reason to change it now would be if it makes the code clearer (when someone looks at it in the future).

There is no residual security issue based on my analysis of the current code.

Labels: CVE_description-submitted

Sign in to add a comment