Closed Bug 1168207 (CVE-2015-2739) Opened 5 years ago Closed 5 years ago

Memory safety problem in ArrayBufferBuilder::append

Categories

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

38 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed
firefox-esr31 39+ fixed
firefox-esr38 39+ fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: q1, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-intoverflow, regression, sec-high, Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows; rv:***) Gecko/20100101 Firefox/**.*
Build ID: 20150305021524

Steps to reproduce:

ArrayBufferBuilder::append (in dom\base\nsXMLHttpRequest.cpp) can overwrite memory it does not own if aDataLen is very large. For example, assume that mLength=2, aDataLen=0xffffffff, and mCapacity=2. In this case, the LHS on line 4019 overflows, giving the value 1. Control then skips to line 4047 (assume a release build), which copies aDataLen bytes from aNewData all over Firefox's address space. Presumably this eventually will cause an access-violation crash on 32-bit platforms, but perhaps not before some other thread uses the written data to do something undesirable. On 64-bit platforms, execution might continue for a long time, depending upon how address space is laid out.

I don't know, however, whether ArrayBufferBuilder::append is exposed to external content in such a way that this defect can be exploited. I suppose that a server could send back a defective XMLHttpRequest containing a very large length, though perhaps other code checks for this problem.

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) {
4020:     uint32_t newcap;
4021:     // Double while under aMaxGrowth or if not specified.
4022:     if (!aMaxGrowth || mCapacity < aMaxGrowth) {
4023:       newcap = mCapacity * 2;
4024:     } else {
4025:       newcap = mCapacity + aMaxGrowth;
4026:     }
4027: 
4028:     // But make sure there's always enough to satisfy our request.
4029:     if (newcap < mLength + aDataLen) {
4030:       newcap = mLength + aDataLen;
4031:     }
4032: 
4033:     // Did we overflow?
4034:     if (newcap < mCapacity) {
4035:       return false;
4036:     }
4037: 
4038:     if (!setCapacity(newcap)) {
4039:       return false;
4040:     }
4041:   }
4042: 
4043:   // Assert that the region isn't overlapping so we can memcpy.
4044:   MOZ_ASSERT(!areOverlappingRegions(aNewData, aDataLen, mDataPtr + mLength,
4045:                                     aDataLen));
4046: 
4047:   memcpy(mDataPtr + mLength, aNewData, aDataLen);
4048:   mLength += aDataLen;
4049: 
4050:   return true;
4051: }
Component: Untriaged → DOM
Product: Firefox → Core
Flags: needinfo?(dveditz)
So in practice this code is only reached from nsXMLHttpRequest::StreamReaderFunc which is a callback passed to ReadSegments in nsXMLHttpRequest::OnDataAvailable.  The aDataLen value is then the size of the necko-internal data buffer involved.

A bigger problem is that mLength is in fact under hostile control: it's just how much data there is.  Simply doing a gzip-bomb kind of thing would work.  In a 32-bit process this would fail to allocate the necessary 4 gigabytes of memory, but in a 64-bit one we could in fact end up with overflow on line 4019 there.

We should just use CheckedUint32 for the arithmetic here, I'd think.
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8610703 [details] [diff] [review]
Be a bit more careful with overflow checking in XHR

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Probably not that easily, esp. for 32-bit systems.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  The patch itself does, since it's clearly about overflow checking.

Which older supported branches are affected by this flaw?  Anything newer than Firefox 24.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  I think this should be simple to backport.

How likely is this patch to cause regressions; how much testing does it need?  I doubt there is much regression risk here.
Attachment #8610703 - Flags: sec-approval?
Attachment #8610703 - Flags: review?(amarchesini) → review+
Comment on attachment 8610703 [details] [diff] [review]
Be a bit more careful with overflow checking in XHR

sec-approval=dveditz
Attachment #8610703 - Flags: sec-approval? → sec-approval+
Comment on attachment 8610703 [details] [diff] [review]
Be a bit more careful with overflow checking in XHR

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec:high.
User impact if declined: Potential for for overwriting random data in our address space if a server sends just the wrong response.
Fix Landed on Version: 41
Risk to taking this patch (and alternatives if risky): Pretty low risk unless I
  screwed up the arithmetic.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: Bug 866431 
[User impact if declined]:  Potential for overwriting random data in our address space if a server sends just the wrong response.
[Describe test coverage new/current, TreeHerder]: Lots of XHR tests, but probably none for the overflow scenario here...
[Risks and why]: Low risk, I think, as long as I got the arithmetic right.
[String/UUID change made/needed]: None.
Attachment #8610703 - Flags: approval-mozilla-esr38?
Attachment #8610703 - Flags: approval-mozilla-esr31?
Attachment #8610703 - Flags: approval-mozilla-beta?
Attachment #8610703 - Flags: approval-mozilla-aurora?
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8610703 [details] [diff] [review]
Be a bit more careful with overflow checking in XHR

Approved for uplift to beta, esr31, and esr38.  sec-high, fixes a regression. 

Also for aurora but we should wait until tomorrow (Tuesday) to land this on aurora, after updates are re-enabled.
Attachment #8610703 - Flags: approval-mozilla-esr38?
Attachment #8610703 - Flags: approval-mozilla-esr38+
Attachment #8610703 - Flags: approval-mozilla-esr31?
Attachment #8610703 - Flags: approval-mozilla-esr31+
Attachment #8610703 - Flags: approval-mozilla-beta?
Attachment #8610703 - Flags: approval-mozilla-beta+
Attachment #8610703 - Flags: approval-mozilla-aurora?
Attachment #8610703 - Flags: approval-mozilla-aurora+
bz, might it be good to file a new bug to write some tests for this sort of overflow situation  -- if they don't exist already?
Flags: needinfo?(bzbarsky)
This doesn't seem to apply to esr31 (nsXMLHttpRequest.cpp doesn't even exist there).
(In reply to Wes Kocher (:KWierso) from comment #11)
> This doesn't seem to apply to esr31 (nsXMLHttpRequest.cpp doesn't even exist
> there).

nsXMLHttpRequest is definitely on ESR31. It is just in a different location, content/base/src/nsXMLHttpRequest.cpp
Flags: sec-bounty?
(In reply to Andrew McCreight [:mccr8] from comment #13)
> (In reply to Wes Kocher (:KWierso) from comment #11)
> > This doesn't seem to apply to esr31 (nsXMLHttpRequest.cpp doesn't even exist
> > there).
> 
> nsXMLHttpRequest is definitely on ESR31. It is just in a different location,
> content/base/src/nsXMLHttpRequest.cpp

Thanks for the pointer. 

https://hg.mozilla.org/releases/mozilla-esr31/rev/6d8096018db6
Blocks: 1170413
> might it be good to file a new bug to write some tests for this sort of overflow 

Yeah, good idea.  Filed bug 1170413.
Flags: needinfo?(bzbarsky)
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+]
Alias: CVE-2015-2739
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.