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 2 users
Status: Fixed
Last visit 17 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 Back to list

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, 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
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, 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, 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
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
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.
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.

Sign in to add a comment