1170809 - (CVE-2015-2740) Overflow in nsXMLHttpRequest::AppendToResponseText causes memory-safety bug

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[q1]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: crash4.patch

Comment 2

[q1]

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.

Comment 3

[q1]

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.

Comment 4

[(no longer active)]

q1, can you please file comment 2 and comment 3 separately?  Thanks!

(Also, I'm curious to know how you're finding these.  :-)

Comment 5

[q1]

[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 6

[q1]

Oops, I already reported the bug in comment 2 as https://bugzilla.mozilla.org/show_bug.cgi?id=1168207 . I've spawned https://bugzilla.mozilla.org/show_bug.cgi?id=1171166 for comment 3.

Comment 7

[Andrea Marchesini [:baku]]

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?

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 9

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

Comment 10

[(no longer active)]

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

Comment 11

[q1]

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

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

This will probably miss 39 beta 4, but we could still get it into beta 5 if that seems best.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 14

[(no longer active)]

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

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8614658 [details] [diff] [review]
crash4.patch

Many approvals given.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfdca3f63b48

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bae7437861e7

Comment 18

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10597487&repo=mozilla-inbound

Comment 19

[Ryan VanderMeulen [:RyanVM]]

And Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/0c9c9cb75c06

Comment 20

[Andrea Marchesini [:baku]]

Attachment: crash4.patch

-  if (neededCapacity.isValid()) {
+  if (!neededCapacity.isValid()) {
     return NS_ERROR_OUT_OF_MEMORY;
   }

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e51efbbf1d6

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b53ec0c065e

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Andrea Marchesini [:baku]]

Ehsan, any idea why?

Comment 25

[(no longer active)]

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?

Comment 26

[Andrea Marchesini [:baku]]

Attachment: crash4.patch

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.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 29

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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 :(

Comment 33

[Andrea Marchesini [:baku]]

Attachment: crash4.patch

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ac18409d64

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=11004069&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/577434cfbe6f

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Ironically, it successfully built on ASAN at least.

Comment 37

[Andrea Marchesini [:baku]]

Attachment: crash4.patch

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e9a6613f8ed

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/71dbe6bca798
https://hg.mozilla.org/releases/mozilla-release/rev/049a3b900417
https://hg.mozilla.org/releases/mozilla-esr38/rev/14f588171a9d
https://hg.mozilla.org/releases/mozilla-esr31/rev/7707a37fe477

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e9a6613f8ed

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2f8b845e5fa3
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/d3e432c4546a
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/d3e432c4546a
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/6a6840e6c51e
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/6a6840e6c51e

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/281ed9edce03