Bug 1170809 (CVE-2015-2740)

Overflow in nsXMLHttpRequest::AppendToResponseText causes memory-safety bug

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: q1, Assigned: baku)

Tracking

({csectype-intoverflow, sec-critical})

38 Branch
mozilla41
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox38 wontfix, firefox39+ fixed, firefox38.0.5 wontfix, firefox40+ fixed, firefox41+ fixed, firefox-esr3139+ fixed, firefox-esr3839+ fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+])

Attachments

(1 attachment, 4 obsolete attachments)

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

Steps to reproduce:

nsXMLHttpRequest::AppendToResponseText (38.0.1\dom\base\nsXMLHttpRequest.cpp) can write to memory that it does not own:

665: nsresult
666: nsXMLHttpRequest::AppendToResponseText(const char * aSrcBuffer,
667:                                       uint32_t aSrcBufferLen)
668: {
669:   NS_ENSURE_STATE(mDecoder);
670:
671:   int32_t destBufferLen;
672:   nsresult rv = mDecoder->GetMaxLength(aSrcBuffer, aSrcBufferLen,
673:                                        &destBufferLen);
674:   NS_ENSURE_SUCCESS(rv, rv);
675:
676:   if (!mResponseText.SetCapacity(mResponseText.Length() + destBufferLen, fallible)) {
677:     return NS_ERROR_OUT_OF_MEMORY;
678:   }
679:
680:   char16_t* destBuffer = mResponseText.BeginWriting() + mResponseText.Length();
681:
682:   int32_t totalChars = mResponseText.Length();
683:
684:   // This code here is basically a copy of a similar thing in
685:   // nsScanner::Append(const char* aBuffer, uint32_t aLen).
686:   int32_t srclen = (int32_t)aSrcBufferLen;
687:   int32_t destlen = (int32_t)destBufferLen;
688:   rv = mDecoder->Convert(aSrcBuffer,
689:                          &srclen,
690:                          destBuffer,
691:                          &destlen);
692:   ...

The bug is in line 676. If mResponseText.Length() + destBufferLen overflows, line 676 will SetCapacity to a value smaller than mResponseText's current length. The remaining code will then write data beginning at the _end_ of the buffer (mResponseText.BeginWriting() + mResponseText.Length()).

Since the response text from an XMLHttpRequest is under external control, this bug could be leveraged to corrupt Firefox's address space, possibly causing execution of attacker-chosen code.

Presumably this problem can occur only under 64-bit OSes.
Assignee: nobody → amarchesini
Status: UNCONFIRMED → NEW
Ever confirmed: true
Posted patch crash4.patch (obsolete) — Splinter Review
Attachment #8614658 - Flags: review?(ehsan)
There is a similar bug in ArrayBufferBuilder::append in the same module:

4013: bool
4014: ArrayBufferBuilder::append(const uint8_t *aNewData, uint32_t aDataLen,
4015:                            uint32_t aMaxGrowth)
4016: {
4017:   MOZ_ASSERT(!mMapPtr);
4018:
4019:   if (mLength + aDataLen > mCapacity) {
        ...
4041:   }
        ...
4047:   memcpy(mDataPtr + mLength, aNewData, aDataLen);
        ...

Line 4019 can overflow, skipping the code in that if block that ordinarily would expand the buffer to accomodate aDataLen. Then line 4047 copies the new data past the end of the existing buffer.

This code seems to be exposed to external content via the call in nsXMLHttpRequest::StreamReaderFunc.
There is a similar bug in nsXMLHttpRequest::SendAsBinary in the same module:

2361: void
2362: nsXMLHttpRequest::SendAsBinary(const nsAString &aBody,
2363:                                ErrorResult& aRv)
2364: {
2365:   char *data = static_cast<char*>(NS_Alloc(aBody.Length() + 1));
2366:   if (!data) {
2367:     aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
2368:     return;
2369:   }
        ... // use data

If the addition in line 2365 overflows, data will point to a zero-length buffer, which the remainder of the function will then overwrite.
q1, can you please file comment 2 and comment 3 separately?  Thanks!

(Also, I'm curious to know how you're finding these.  :-)
Attachment #8614658 - Flags: review?(ehsan) → review+
Component: Untriaged → DOM
Product: Firefox → Core
[blockquote]q1, can you please file comment 2 and comment 3 separately?  Thanks![/blockquote]
Sure. I am curious about the policy on multiple bugs in one module or subtree, though, as someone else asked me to file one bug report for all bugs in a single subtree: https://bugzilla.mozilla.org/show_bug.cgi?id=1169326#c2 .

[blockquote](Also, I'm curious to know how you're finding these.  :-)[/blockquote]
That'd be telling, but the answer is banal: code review.

Also, what's the procedure to blockquote text?
Comment on attachment 8614658 [details] [diff] [review]
crash4.patch

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

I don't think it's so easy.

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

Yes.

Which older supported branches are affected by this flaw?

All the branches.

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

No big risks.

How likely is this patch to cause regressions; how much testing does it need?
Attachment #8614658 - Flags: sec-approval?
Attachment #8614658 - Flags: approval-mozilla-esr38?
Attachment #8614658 - Flags: approval-mozilla-esr31?
Attachment #8614658 - Flags: approval-mozilla-beta?
Attachment #8614658 - Flags: approval-mozilla-aurora?
Keywords: checkin-needed
This needs a security rating before it can get sec-approval (and doesn't need sec-approval for certain security ratings). Security rating also determines back porting to ESR branches as well.

This sounds like at least a sec-high. Do we have a test case that actually reproduces the crash?
Flags: needinfo?(q1)
I have not tried to create a test case. I found this bug (and virtually all the others I've reported) by code review.
(In reply to q1 from comment #5)
> [blockquote]q1, can you please file comment 2 and comment 3 separately? 
> Thanks![/blockquote]
> Sure. I am curious about the policy on multiple bugs in one module or
> subtree, though, as someone else asked me to file one bug report for all
> bugs in a single subtree:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1169326#c2 .

Yeah, there is no hard rule.  But a general rule of thumb is that if you file something for one issue, and comment on the same bug for other issues, there is a chance that people may miss them, which was mostly my point.  :-)

> [blockquote](Also, I'm curious to know how you're finding these. 
> :-)[/blockquote]
> That'd be telling, but the answer is banal: code review.

Hah, nice work!

> Also, what's the procedure to blockquote text?

You can click on the "[reply]" link next to the comment number.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> (In reply to q1 from comment #5)
> > [blockquote]q1, can you please file comment 2 and comment 3 separately? 
> > Thanks![/blockquote]
> > Sure. I am curious about the policy on multiple bugs in one module or
> > subtree, though, as someone else asked me to file one bug report for all
> > bugs in a single subtree:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1169326#c2 .
> 
> Yeah, there is no hard rule.  But a general rule of thumb is that if you
> file something for one issue, and comment on the same bug for other issues,
> there is a chance that people may miss them, which was mostly my point.  :-)

Good point. Also I suppose it makes tracking easier to file one report per bug, so I'll do that from now on.

> > Also, what's the procedure to blockquote text?
> 
> You can click on the "[reply]" link next to the comment number.

It's so simple, I missed it. It's interesting that it's implemented like old-style email quoting, by parsing greater-than signs.
This will probably miss 39 beta 4, but we could still get it into beta 5 if that seems best.
Ehsan, can you suggest a security rating so I can give this sec-approval to go in? What is the implication of hitting this code path?
Flags: needinfo?(ehsan)
(In reply to Al Billings [:abillings] from comment #13)
> Ehsan, can you suggest a security rating so I can give this sec-approval to
> go in? What is the implication of hitting this code path?

This bug can cause us to write past the end of a heap allocated buffer, and it's especially dangerous since it's in XHR code.  At least in theory, it would make it possible for example for the attacker to poison vtable pointers with an address to an attacker controlled table, which can lead to arbitrary code execution.  So I think this is sec-critical.
Flags: needinfo?(ehsan)
Comment on attachment 8614658 [details] [diff] [review]
crash4.patch

Many approvals given.
Attachment #8614658 - Flags: sec-approval?
Attachment #8614658 - Flags: sec-approval+
Attachment #8614658 - Flags: approval-mozilla-esr38?
Attachment #8614658 - Flags: approval-mozilla-esr38+
Attachment #8614658 - Flags: approval-mozilla-esr31?
Attachment #8614658 - Flags: approval-mozilla-esr31+
Attachment #8614658 - Flags: approval-mozilla-beta?
Attachment #8614658 - Flags: approval-mozilla-beta+
Attachment #8614658 - Flags: approval-mozilla-aurora?
Attachment #8614658 - Flags: approval-mozilla-aurora+
Flags: sec-bounty?
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10597487&repo=mozilla-inbound
Flags: needinfo?(amarchesini)
Posted patch crash4.patch (obsolete) — Splinter Review
-  if (neededCapacity.isValid()) {
+  if (!neededCapacity.isValid()) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
Attachment #8614658 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Backed out for making ASAN opt builds perma-timeout. AFAICT, looks like the timeouts were during the checktests. No other platforms were affected.
https://treeherder.mozilla.org/logviewer.html#?job_id=10607298&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/22c7bbe185f5
https://hg.mozilla.org/releases/mozilla-aurora/rev/a5826a94fc54
Flags: needinfo?(amarchesini)
Flags: needinfo?(q1)
Ehsan, any idea why?
Flags: needinfo?(amarchesini) → needinfo?(ehsan)
Hmm, my guess would be that we're triggering one of those new NS_ERROR_OUT_OF_MEMORY error return paths now somehow.  Perhaps try adding MOZ_CRASHes there and push to try?
Flags: needinfo?(ehsan)
Posted patch crash4.patch (obsolete) — Splinter Review
Read previous comments.
It took forever to find a patch that makes asan clang compiler happy. The original patch was making the compiler time out.
Attachment #8617973 - Attachment is obsolete: true
Attachment #8624742 - Flags: sec-approval?
I don't think this needs to get security-approval again, but I'm wondering if Ehsan should take another look before we push it again? We're up against the wall timing-wise, so this needs to land Really Soon now if the intent is for it to make Fx39.
Flags: needinfo?(ehsan)
Comment on attachment 8624742 [details] [diff] [review]
crash4.patch

You don't need to set sec-approval? again after it has been given but, as Ryan says, we're shipping soon. This bounced and the last beta is done. I really doubt this is making 39.
Attachment #8624742 - Flags: sec-approval?
While this is critical I think this can wait for 40. I'd like us to feel more confident about the fix and have time to follow through on test failurees.  If you disagree strongly then please let me know, and we can still get this into the RC build on Monday.
BTW, this is further-complicated by the fact that it already landed and bounced once, so intrepid folks paying attention to our commits might already be aware of this issue :\
Comment on attachment 8624742 [details] [diff] [review]
crash4.patch

This makes sense to me, though it's sad asan failed on the obvious thing.  :(

That said, shouldn't 'size' be a uint32_t as opposed to int32_t?  Or better yet mResponseText::size_type?  Otherwise your assignment is undefined behavior when the RHS doesn't fit in int32_t.
Andrea, if you can address comment 31 soon, I can help get this landed and merged around ASAP this weekend. I think we should assume that this is essentially out in the open :(
Flags: needinfo?(ehsan) → needinfo?(amarchesini)
Posted patch crash4.patch (obsolete) — Splinter Review
Attachment #8624742 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Ironically, it successfully built on ASAN at least.
Posted patch crash4.patchSplinter Review
Attachment #8625135 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/5e9a6613f8ed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+]
Alias: CVE-2015-2740
Flags: sec-bounty? → sec-bounty+
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.