Closed Bug 1451908 (CVE-2018-5177) Opened 2 years ago Closed 2 years ago

undefined behavior results in negative allocation size

Categories

(Core :: XSLT, defect)

61 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 + verified
firefox61 + verified

People

(Reporter: guyinbara, Assigned: erahm)

Details

(Keywords: csectype-undefined, sec-moderate, Whiteboard: [adv-main60+][post-critsmash-triage])

Crash Data

Attachments

(4 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

I've audited the the xslt format-number function implementation and found a corner case the results in a negative bufsize allocation (which is sign extended and casted to size_t on a call to new array[]).

the bug lies in the file:
mozilla-central/source/dom/xslt/xslt/txFormatNumberFunctionCall.cpp+273

the buggy code is:
value = fabs(value) * multiplier;

    // Prefix
    nsAutoString res(prefix);

    int bufsize;
    if (value > 1)
        bufsize = (int)log10(value) + 30;
    else
        bufsize = 1 + 30;

    char* buf = new char[bufsize];

what happens is that the the value is checked earlier in the function to not be INF/-INF/NaN but is later multiplied by a possible multiplier that can cause it to turn into INF.
when that happens, log10(INF) returns INF which when casted to int which is undefined (will result on x86 as a large negative number) and later on will be used as a size for allocation.

steps to repro:
  browse to: https://www.w3schools.com/xml/tryxslt.asp?xmlfile=cdcatalog&xsltfile=cdcatalog_ex2
  insert into the xsl table the text supplied in the attached file


Actual results:

(int)log10(value) is undefined when value is INF/-INF/NaN
the resulting value is used afterwards as an allocation initialization.



Expected results:

there should've been a check for invalid double values after the multiplication (or drop the multiplication altogether).

i'd recommend to drop the multiplication or add another test after multiplication.
Andrew, who is maintaining XSLT these days? :)
Group: firefox-core-security → core-security
Component: Untriaged → XSLT
Flags: needinfo?(overholt)
Product: Firefox → Core
I get the following OOM crash on nightly: bp-53fa3bcc-5216-469e-93f9-bef9c0180406
Group: core-security → dom-core-security
Status: UNCONFIRMED → NEW
Crash Signature: [@ OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | txFormatNumberFunctionCall::evaluate ]
Ever confirmed: true
Flags: sec-bounty?
I've explained thoroughly the bug cause. My fear is that a cast from inf to int will result in zero on some architectures that will cause the allocation to pass zero as the size and then call PRdtoa with size-1 which will be -1 casted to size_t which is a huge number causing the function to overflow the buffer
(In reply to Johann Hofmann [:johannh] from comment #1)
> Andrew, who is maintaining XSLT these days? :)

Eric and Peter know things :)
Flags: needinfo?(peterv)
Flags: needinfo?(overholt)
Flags: needinfo?(erahm)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
(In reply to guyio from comment #3)
> I've explained thoroughly the bug cause. My fear is that a cast from inf to
> int will result in zero on some architectures that will cause the allocation
> to pass zero as the size and then call PRdtoa with size-1 which will be -1
> casted to size_t which is a huge number causing the function to overflow the
> buffer

Yes, casting infinity to an int is UB. It's probably currently okay on all of our tier-1 platforms, but this is certainly worth fixing.
Flags: needinfo?(erahm)
Comment on attachment 8968619 [details] [diff] [review]
Check for infinite value in txFormatNumberFunctionCall

Review of attachment 8968619 [details] [diff] [review]:
-----------------------------------------------------------------

Peter, this is the simplest change. We just check for infinity again once we have the multiplier.
Comment on attachment 8968619 [details] [diff] [review]
Check for infinite value in txFormatNumberFunctionCall

Review of attachment 8968619 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Could you add an automated testcase?
Attachment #8968619 - Flags: review?(peterv) → review+
hope this file could be of use to an automated testcase
(In reply to Peter Van der Beken [:peterv] from comment #8)
> Comment on attachment 8968619 [details] [diff] [review]
> Check for infinite value in txFormatNumberFunctionCall
> 
> Review of attachment 8968619 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! Could you add an automated testcase?

Will do, ni to actually do it.

(In reply to guyio from comment #9)
> Created attachment 8969479 [details]
> automated html testcase
> 
> hope this file could be of use to an automated testcase

Thanks!
Flags: needinfo?(peterv) → needinfo?(erahm)
Attached patch Add crashtestSplinter Review
Attachment #8970661 - Flags: review?(peterv)
Comment on attachment 8968619 [details] [diff] [review]
Check for infinite value in txFormatNumberFunctionCall

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It just looks like we're fixing a formatting edge case, but you can also tell we're possibly looking at overflow.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No. Crashtest is attached separately and will land when the bug is public.

Which older supported branches are affected by this flaw?

All.

If not all supported branches, which bug introduced the flaw?

N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The patch should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?

Low risk, we're just early returning with the proper formatting. I confirmed locally that the crashtest passes with the patch applied.
Flags: needinfo?(erahm)
Attachment #8968619 - Flags: sec-approval?
Although this doesn't seem exploitable with our current compilers, it would be nice to get this into ESR-60 because we don't know what people might be using on that branch a year down the line.
Comment on attachment 8968619 [details] [diff] [review]
Check for infinite value in txFormatNumberFunctionCall

sec-approval+.

Can we get a beta branch patch made and nominated?
Flags: needinfo?(erahm)
Attachment #8968619 - Flags: sec-approval? → sec-approval+
Comment on attachment 8968619 [details] [diff] [review]
Check for infinite value in txFormatNumberFunctionCall

Approval Request Comment
[Feature/Bug causing the regression]: XSLT number formatting
[User impact if declined]: OOMs, UB may cause sec issues in the future
[Is this code covered by automated tests?]: I don't believe so.
[Has the fix been verified in Nightly?]: Currently on inbound, crashtest confirmed to fix the issue locally.
[Needs manual test from QE? If yes, steps to reproduce]: Run the attached crashtest.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Small change to return early in a number formatting method.
[String changes made/needed]: N/A
Flags: needinfo?(erahm)
Attachment #8968619 - Flags: approval-mozilla-beta?
Attachment #8968619 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Attachment #8970661 - Flags: review?(peterv) → review+
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main60+]
Alias: CVE-2018-5177
Flags: qe-verify+
Whiteboard: [adv-main60+] → [adv-main60+][post-critsmash-triage]
I have successfully reproduced the issue on Fx 59.0b14 using the attached crashtest.

This issue is verified fixed as the browser no longer crashes on Firefox 60.0b16 and Firefox 61.0a1 using Windows 10 64bit, macOS 10.11 and Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.