Bug 1171603 (CVE-2015-4487)

Overflow nsTSubstring::ReplacePrep causes memory-safety bugs in string library

RESOLVED FIXED in Firefox 40, Firefox OS master

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: q1, Assigned: baku)

Tracking

({csectype-intoverflow, sec-moderate})

38 Branch
mozilla42
csectype-intoverflow, sec-moderate
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox39 wontfix, firefox40 fixed, firefox41 fixed, firefox42 fixed, firefox-esr31 wontfix, firefox-esr38 wontfix, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 wontfix, b2g-v2.2r wontfix, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main40+])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305021524

Steps to reproduce:

nsTSubstring::ReplacePrep (C:\docs\dev\firefox\source\38.0.1\xpcom\string\nsTSubstring.h) can cause memory to be written that the destination string object does not own. This bug could permit an attacker to write portions of Firefox's address space, potentially allowing her to execute code of her choice.

The bug is in line 1003:

 998:  NS_WARN_UNUSED_RESULT bool ReplacePrep(index_type aCutStart,
 999:                                         size_type aCutLength,
1000:                                         size_type aNewLength)
1001:  {
1002:    aCutLength = XPCOM_MIN(aCutLength, mLength - aCutStart);
1003:    uint32_t newTotalLen = mLength - aCutLength + aNewLength;
1004:    if (aCutStart == mLength && Capacity() > newTotalLen) {
1005:      mFlags &= ~F_VOIDED;
1006:      mData[newTotalLen] = char_type(0);
1007:      mLength = newTotalLen;
1008:      return true;
1009:    }
1010:    return ReplacePrepInternal(aCutStart, aCutLength, aNewLength, newTotalLen);
1011:  }

If aCutStart > mLength, aCutLength becomes a very large uint32_t. The same thing happens if aCutLength is very large, or if aNewLength is very large. In all of these cases, newTotalLen can overflow, resulting in a call to ReplacePrepInternal with a too-small newTotalLen.


The simplest case is where aNewLength is very large.

Using aNewLength == 0xfffffffe and mLength == 10 for the existing string, imagine a call to Append (&s, 0xfffffffe). (The same kind of thing works on AppendASCII, etc.). Append lives at nsTSubstring.h line 531:

531:  void Append(const char_type* aData, size_type aLength = size_type(-1))
532:  {
533:    Replace(mLength, 0, aData, aLength);
534:  }
 
The call to Replace resolves to the function at nsTSubstring.cpp line 502 with arguments (10, 0, &s, 0xfffffffe):

502: void
503: nsTSubstring_CharT::Replace(index_type aCutStart, size_type aCutLength,
504:                             const char_type* aData, size_type aLength)
505: {
506:   if (!Replace(aCutStart, aCutLength, aData, aLength,
507:                mozilla::fallible)) {
508:     AllocFailed(Length() - aCutLength + 1);
509:   }
510: }

which then chains to the function at nsTSubstring.cpp line 512 with arguments (10, 0, &s, 0xfffffffe, mozilla::fallible)):

512: bool
513: nsTSubstring_CharT::Replace(index_type aCutStart, size_type aCutLength,
514:                             const char_type* aData, size_type aLength,
515:                             const fallible_t& aFallible)
516: {
517:   // unfortunately, some callers pass null :-(
518:   if (!aData) {
519:     aLength = 0;
520:   } else {
521:     if (aLength == size_type(-1)) {
522:       aLength = char_traits::length(aData);
523:     }
524: 
525:     if (IsDependentOn(aData, aData + aLength)) {
526:       nsTAutoString_CharT temp(aData, aLength);
527:       return Replace(aCutStart, aCutLength, temp, aFallible);
528:     }
529:   }
530: 
531:   aCutStart = XPCOM_MIN(aCutStart, Length());
532: 
533:   bool ok = ReplacePrep(aCutStart, aCutLength, aLength);
534:   if (!ok) {
535:     return false;
536:   }
537: 
538:   if (aLength > 0) {
539:     char_traits::copy(mData + aCutStart, aData, aLength);
540:   }
541: 
542:   return true;
543: }

Control passes to line 525, which returns false because aData does not overlap mData. Line 531 then computes 10 for aCutStart, so line 533 calls ReplacePrep (10, 0, 0xfffffffe). That function:

 998:  NS_WARN_UNUSED_RESULT bool ReplacePrep(index_type aCutStart,
 999:                                         size_type aCutLength,
1000:                                         size_type aNewLength)
1001:  {
1002:    aCutLength = XPCOM_MIN(aCutLength, mLength - aCutStart);
1003:    uint32_t newTotalLen = mLength - aCutLength + aNewLength;
1004:    if (aCutStart == mLength && Capacity() > newTotalLen) {
1005:      mFlags &= ~F_VOIDED;
1006:      mData[newTotalLen] = char_type(0);
1007:      mLength = newTotalLen;
1008:      return true;
1009:    }
1010:    return ReplacePrepInternal(aCutStart, aCutLength, aNewLength, newTotalLen);
1011:  }

computes aCutLength == 0 on line 1002, and newTotalLen == 10 - 0 + 0xfffffffe == 8 on line 1003. The test on line 1004 passes, line 1006 spuriously writes a 0 to mData [8], and control returns to the caller (nsTSubstring_CharT::Replace) at line 534. Control then eventually passes to line 539:

539:     char_traits::copy(mData + aCutStart, aData, aLength);

which copies 0xfffffffe bytes from aData into the unallocated region beginning at mData + 10.
Flags: sec-bounty?

Updated

4 years ago
Component: Untriaged → String
Flags: needinfo?(dbaron)
Product: Firefox → Core
IIRC, we analyzed these methods for integer overflow a year or two ago.
> If aCutStart > mLength

I'm pretty sure that can't occur.  I buried this in some large Grid patches:
+    MOZ_ASSERT(aCutStart <= mLength);
and it looks green on Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbe6413ac829

There is also an implicit invariant that string lengths (mLength) are < 2G:
http://mxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.cpp#44

I suspect that we have very few calls to Append[ASCII] that pass a length
that isn't a string length.

Have you found any instances where aNewLength in ReplacePrep isn't a string length?
Flags: needinfo?(q1)
(Reporter)

Comment 3

4 years ago
(In reply to Mats Palmgren (:mats) from comment #2)
> > If aCutStart > mLength
> 
> I'm pretty sure that can't occur.  I buried this in some large Grid patches:
> +    MOZ_ASSERT(aCutStart <= mLength);
> and it looks green on Try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbe6413ac829

I am not familiar with this test. Can you explain it?

> There is also an implicit invariant that string lengths (mLength) are < 2G:
> http://mxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.
> cpp#44

I'll look at that. Thanks.
 
> I suspect that we have very few calls to Append[ASCII] that pass a length
> that isn't a string length.
>
> Have you found any instances where aNewLength in ReplacePrep isn't a string
> length?

If you mean instances where the length passed into Append[AScII] is not implicitly specified by a literal, and where the body and length of a ns*String are not passed in, yes, there are quite a few. Here's a nonexhaustive list:

StringBuilder::ToString in (38.0.1\dom\base\FragmentOrElement.cpp line 2224)
nsFeedSniffer::AppendSegmentToString (38.0.1\browser\components\feeds\nsFeedSniffer.cpp line 343)
AppendSegmentToString (38.0.1\docshell\base\nsDocShell.cpp line 10823)
nsContentUtils::GetPseudoAttributeValue (38.0.1\dom\base\nsContentUtils.cpp line 1131; this one _appears_ safe)
nsPlainTextSerializer::AddToLine (38.0.1\dom\base\nsPlainTextSerializer.cpp line 1249)
nsXMLHttpRequest::StreamReaderFunc (38.0.1\dom\base\nsXMLHttpRequest.cpp line 1843)
DictionaryBase::AppendJSONToString (38.0.1\dom\bindings\BindingUtils.cpp line 1734)
TextDecoder::Decode (38.0.1\dom\encoding\TextDecoder.cpp line 77)
nsJSONWriter::Write (38.0.1\dom\json\nsJSON.cpp line 305 & 311)
nsJSONWriter::FlushBuffer() (38.0.1\dom\json\nsJSON.cpp line 328)
AppendSegmentToString (38.0.1\dom\security\nsCSPContext.cpp line 1210)
nsBase64Encoder::Write (38.0.1\netwerk\base\nsBase64Encoder.cpp line 27)
CopyRawHeader (\38.0.1\netwerk\mime\nsMIMEHeaderParamImpl.cpp line 1082)


Even if all these usages currently are safe (which I don't know), the string library should protect itself from bad arguments, so that someone doesn't inadvertently add a security bug.
Flags: needinfo?(q1)
(In reply to q1 from comment #3)
> (In reply to Mats Palmgren (:mats) from comment #2)
> > > If aCutStart > mLength
> > 
> > I'm pretty sure that can't occur.  I buried this in some large Grid patches:
> > +    MOZ_ASSERT(aCutStart <= mLength);
> > and it looks green on Try:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbe6413ac829
> 
> I am not familiar with this test. Can you explain it?

I added the above assertion to the code and then ran all our
regression test suites with that change and the tests passed.

ReplacePrep is an internal method so I think that assertion
is good enough to cover any mistake with 'aCutStart'.

> Even if all these usages currently are safe (which I don't know), the string
> library should protect itself from bad arguments, so that someone doesn't
> inadvertently add a security bug.

Sure.  Using CheckedInt for calculating 'newTotalLen' and returning
false if it overflows seems reasonable.  I wonder why we didn't
do that when we looked at this earlier though.
Keywords: csectype-intoverflow
(Assignee)

Comment 5

4 years ago
Created attachment 8625450 [details] [diff] [review]
crash7.patch
Attachment #8625450 - Flags: review?(mats)
(Assignee)

Updated

4 years ago
Assignee: nobody → amarchesini
Comment on attachment 8625450 [details] [diff] [review]
crash7.patch

Looks good to me.  You should get r+ from a module peer though (I'm not).
Attachment #8625450 - Flags: review?(mats) → feedback+
(Assignee)

Comment 7

4 years ago
Comment on attachment 8625450 [details] [diff] [review]
crash7.patch

ehsan, move this patch if your review queue is too crazy.
Attachment #8625450 - Flags: review?(ehsan)

Updated

3 years ago
Attachment #8625450 - Flags: review?(ehsan) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Needs a sec rating and/or sec-approval before landing.
Keywords: checkin-needed
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(amarchesini)
(Assignee)

Comment 9

3 years ago
Created attachment 8628244 [details] [diff] [review]
crash7.patch

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

Not easy at all.

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

The problem is well described in the bug.

Which older supported branches are affected by this flaw?

all.

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

It should be easy to port this patch everywhere in case it doesn't apply.

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

I don't see how it can cause regressions.
Flags: needinfo?(amarchesini)
Attachment #8628244 - Flags: sec-approval?
(Assignee)

Updated

3 years ago
Attachment #8625450 - Attachment is obsolete: true
Do you have a suggestion as to the security rating? Can we actually trigger this bug?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 11

3 years ago
I don't think it's possible to trigger this bug. Or at least we need to play with a string with length close to 2^32.
I would say sec-moderate or less.
Flags: needinfo?(amarchesini)
Keywords: sec-moderate
Comment on attachment 8628244 [details] [diff] [review]
crash7.patch

Clearing sec-approval?. As a bug that isn't a sec-high or sec-critical, it doesn't need sec-approval+ to be checked in.
Attachment #8628244 - Flags: sec-approval?
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/97c7e83acb6b
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-v2.5: --- → affected
status-b2g-master: --- → affected
status-firefox39: --- → wontfix
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox-esr31: --- → wontfix
status-firefox-esr38: --- → affected
Flags: needinfo?(dbaron)
Keywords: checkin-needed
Backed out for Windows Werror bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=11315031&repo=mozilla-inbound
Flags: needinfo?(amarchesini)
(Assignee)

Comment 15

3 years ago
Created attachment 8629539 [details] [diff] [review]
crash7.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a500fb43689
Attachment #8628244 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9d8209f754df
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Please nominate this for aurora/beta/esr38/b2g37/b2g34 approval when you get a chance.
status-b2g-master: affected → fixed
Flags: needinfo?(amarchesini)
(Assignee)

Comment 19

3 years ago
Comment on attachment 8629539 [details] [diff] [review]
crash7.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this is old code.
User impact if declined: possibly a crash
Testing completed: none
Risk to taking this patch (and alternatives if risky): none 
String or UUID changes made by this patch: none
Flags: needinfo?(amarchesini)
Attachment #8629539 - Flags: approval-mozilla-esr38?
Attachment #8629539 - Flags: approval-mozilla-b2g37?
Attachment #8629539 - Flags: approval-mozilla-b2g34?
Attachment #8629539 - Flags: approval-mozilla-aurora?
Attachment #8629539 - Flags: approval-mozilla-beta?
Comment on attachment 8629539 [details] [diff] [review]
crash7.patch

Fix a crash, taking it for aurora & beta.
However, not for esr 38 as it is just a sec moderate and it does not seem to be a critical issue from the user perspective.
Attachment #8629539 - Flags: approval-mozilla-esr38?
Attachment #8629539 - Flags: approval-mozilla-esr38-
Attachment #8629539 - Flags: approval-mozilla-beta?
Attachment #8629539 - Flags: approval-mozilla-beta+
Attachment #8629539 - Flags: approval-mozilla-aurora?
Attachment #8629539 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5d348e83333b
status-b2g-v2.5: affected → fixed
status-firefox41: affected → fixed
Target Milestone: --- → mozilla42

Comment 23

3 years ago
Comment on attachment 8629539 [details] [diff] [review]
crash7.patch

Seems very edge and not critical. Also NI new FxOS RM Mahi if he has different opinion.
Flags: needinfo?(mpotharaju)
Attachment #8629539 - Flags: approval-mozilla-b2g37?
Attachment #8629539 - Flags: approval-mozilla-b2g37-
Attachment #8629539 - Flags: approval-mozilla-b2g34?
Attachment #8629539 - Flags: approval-mozilla-b2g34-
status-b2g-v2.1: affected → wontfix
status-b2g-v2.1S: affected → wontfix
status-b2g-v2.2: affected → wontfix
status-b2g-v2.2r: --- → wontfix
status-firefox-esr38: affected → wontfix
Agree with Josh. Thanks for adding me to the loop.
Flags: sec-bounty? → sec-bounty+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main40+]
Alias: CVE-2015-4487
Flags: needinfo?(mpotharaju)
Group: core-security
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.