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

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.

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.

Comment on attachment 8610703 [details] [diff] [review] Be a bit more careful with overflow checking in XHR sec-approval=dveditz

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.

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.

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?

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

(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

> might it be good to file a new bug to write some tests for this sort of overflow Yeah, good idea. Filed bug 1170413.

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/e08cbbf120f1 https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/e08cbbf120f1 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/1e9c67a73182 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/1e9c67a73182