Closed Bug 1171166 Opened 7 years ago Closed 7 years ago

Overflow in nsXMLHttpRequest::SendAsBinary causes memory-safety bug


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

38 Branch
Not set



Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- unaffected
firefox-esr31 --- wontfix
firefox-esr38 --- wontfix


(Reporter: q1, Assigned: baku)


(Keywords: csectype-intoverflow)


(1 file)

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

Steps to reproduce:

[ asked me to file this bug separately from :]

nsXMLHttpRequest::SendAsBinary (38.0.1\dom\base\nsXMLHttpRequest.cpp) can overflow, causing a memory-safety bug:

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.
Flags: sec-bounty?
Note that this requires that aBody has 2^32-1 characters, if I'm reading this correctly.
Component: Untriaged → DOM
Product: Firefox → Core
Andrea, could you investigate this since you've fixed so many other of these bugs? Thanks.
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch patch.patchSplinter Review
Attachment #8624308 - Flags: review?(continuation)
Ever confirmed: true
It looks like this code was removed in Firefox 39, in bug 853162.
We only call this variant of nsXMLHttpRequest::SendAsBinary() from two places: from DOM bindings, and from 
the other variant of SendAsBinary.

The string for DOM bindings is generated via ConvertJSValueToString(). This string comes from a JS string, and the max length of a JS string in Firefox is (1 << 28) - 1, as defined in MaxStringLength in jsfriendapi.h.

The other variant of SendAsBinary() is not called anywhere in Firefox that we ship, and is not exposed to content, but I guess could be called by an addon or something. I'll look into that a bit.
Well, I guess if the string is coming in via XPIDL, then it will also have a max length of (1 << 28) - 1, so it should be okay, too.  I think we don't actually need to fix this, particularly because it is only on ESR31 and ESR38. I'll unhide the bug in a day or two if there are no objections.
Attachment #8624308 - Flags: review?(continuation)
thanks for debugging it. Feel free to close this bug.
Flags: needinfo?(continuation)
Group: core-security
Closed: 7 years ago
Flags: needinfo?(continuation)
Resolution: --- → WONTFIX
In summary: this overflow can't actually happen, because the string comes from JS, and JS strings are never long enough to cause the overflow.
Flags: sec-bounty? → sec-bounty-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.