764296 - (CVE-2012-1962) memory corruption of strings

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill Keese]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.54 Safari/536.5

Steps to reproduce:

1. Load http://bill.dojotoolkit.org/1.7/ffMemoryCorruption.html.   (I'll also attach the test case, but it requires dojo to run, so the URL is simpler.)

2. Click the "add another tab" button repeatedly until the tabs overflow and a down arrow button appears on the upper right of the TabContainer.

3. Repeatedly click the down arrow button to display a menu of tabs, and click choices on the menu to navigate between tabs.




Actual results:

Eventually the menu tab navigation will stop working (usually after two or three times). 

In console (firebug or builtin web console) do this to see the root of the problem:

  contentTabs.getChildren().map(function(f){ return f.id; }).join(", ")

It shows corruption in the ids:

[17:15:15.289] "acceptProposedCo, kanji-characters-etc-here (bugzilla won't let me input them) \x00\x00\x00\x00\x00\x00₀A\x00\x00\x00\x00 ...


Note: occurs on both mac and windows, from version 8 to 14 and possibly earlier versions too.



Expected results:

The id's shouldn't change, they should be plain english strings:

[17:27:56.750] "NeweditorLayout1, NeweditorLayout2, NeweditorLayout3"

Comment 1

[Bill Keese]

Based on http://bugs.dojotoolkit.org/ticket/15496 (but the info in this ticket is sufficient to reproduce and fix)

Comment 2

[Loic]

I think there is a regression indeed.
I tried with mozregression and the corruption of element ID doesn't appear in previous versions of FF.

m-c:
good: 2011-08-03
bad: 2011-08-04
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3735fb1cd5ef&tochange=be4b064f1159

m-i:
good: 2011-08-02
bad: 2011-08-03
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=37373e42a698&tochange=50a3378c5940

Comment 3

[Alice0775 White]

I can reproduce in Firefox4.0.1, It is not easy, but can reproduce.

Step to Reproduce:

1. Open http://bill.dojotoolkit.org/1.7/ffMemoryCorruption.html
2. Open 10 tabs bu clicking "Add another tab button"
3. Click drop down and select #1
4. Click drop down and select #10
5. Click drop down and select #1
6. Click dropdown arrow quickly 10-20 times
7. Click drop down and select #10
8. Repeat Step 5, 6, 7 ,8 until fail to move tab

Actual Result:
Execute the following code
contentTabs.getChildren().map(function(f){ return f.id; }).join(", ")

Output:
NeweditorLayout1, NeweditorLayout2, NeweditorLayout3, NeweditorLayout4, NeweditorLayout5, NeweditorLayout6, NeweditorLayout7, NeweditorLayout8, NeweditorLayout9, <dijitReset di

Expected Results:
NeweditorLayout1, NeweditorLayout2, NeweditorLayout3, NeweditorLayout4, NeweditorLayout5, NeweditorLayout6, NeweditorLayout7, NeweditorLayout8, NeweditorLayout9, NeweditorLayout10

(m-c)
Last Good:
http://hg.mozilla.org/mozilla-central/rev/06a91c14588d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20110104 Firefox/4.0b9pre ID:20110104125846
First Bad:
http://hg.mozilla.org/mozilla-central/rev/b2275b45a33d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20110101 Firefox/4.0b9pre ID:20110101030401
Pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=06a91c14588d&tochange=969691cfe40e


(TM)
Last Good:
http://hg.mozilla.org/tracemonkey/rev/df86d5999fef
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101214 Firefox/4.0b8pre ID:20101215214744
First Bad:
http://hg.mozilla.org/tracemonkey/rev/41ff7f6d02d9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101216 Firefox/4.0b8pre ID:20101216133703
Pushlog:
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=df86d5999fef&tochange=41ff7f6d02d9

Suspected: Bug 609440

Comment 4

[Alice0775 White]

In addition to comment #3
I tested
javascript.options.tracejit.chrome=false
javascript.options.tracejit.content=false
and
javascript.options.tracejit.chrome=true
javascript.options.tracejit.content=true

Comment 5

[Alice0775 White]

> 8. Repeat Step 5, 6, 7 ,8 until fail to move tab
I tried repeat step 8 max 10 times

Comment 6

[Loic]

If you don't open more 9 tabs, you should find my regression range as in comment #2.
After more 10 tabs, I wasn't able to display 'NeweditorLayout10', I saw only sometimes '<dijitReset di' and '<dijitReset di' with Asian symbols (corrupt I guess).

Comment 7

[Alice0775 White]

>After more 10 tabs, I wasn't able to display 'NeweditorLayout10'
seems dojo bug?

Comment 8

[Loic]

(In reply to Alice0775 White from comment #7)
> >After more 10 tabs, I wasn't able to display 'NeweditorLayout10'
> seems dojo bug?

Maybe but as I wasn't totally sure, I just tried the regression with <= 9 tabs.

Comment 9

[Alice0775 White]

OK I test 9tabs opened.

Step to Reproduce:

1. Open http://bill.dojotoolkit.org/1.7/ffMemoryCorruption.html
2. Open 9 tabs by clicking "Add another tab button"
3. Click drop down and select #1
4. Click drop down and select #9
5. repeat step3-4 max 5 times

Actual Result:
Execute the following code
contentTabs.getChildren().map(function(f){ return f.id; }).join(", ")

Output:
NeweditorLayout1, NeweditorLayout2, NeweditorLayout3, dijitMenuItemFoc, [object HTMLTabl, tabStripButtonFo, [object HTMLTabl, , 菠 菰 萐 萠???

Expected Results:
NeweditorLayout1, NeweditorLayout2, NeweditorLayout3, NeweditorLayout4, NeweditorLayout5, NeweditorLayout6, NeweditorLayout7, NeweditorLayout8, NeweditorLayout9

(cached m-i)
Last Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/cfea4859f458
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110802 Firefox/8.0a1 ID:20110802070839
First Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8a54bd8aa5c8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110802 Firefox/8.0a1 ID:20110802102645
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cfea4859f458&tochange=8a54bd8aa5c8

In local build:
Last Good:8bff20b3f8db
First Bad:0884753e359c
Regressed by:
0884753e359c	Luke Wagner — Bug 675132 - When flattening strings, round up after incrementing length (for null char) to avoid wasting memory when jemalloc rounds up the allocation size (r=njn)

Comment 10

[Luke Wagner [:luke]]

I can repro easily on aurora, but not nightly (debug or opt).  Alice can you repro on Nightly?  If not, perhaps we could bisect on the fixing cset?

My suspicion with bug 675132 is that it just perturbs memory allocation patterns in a way that exposes a bug in some other place; the patch itself is pretty simple.  Still, I'll look into it.

Comment 11

[Alice0775 White]

I can still reproduce with clean profile in latest Nightly nightly 
http://hg.mozilla.org/mozilla-central/rev/3f408698a03f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120614030534

Comment 12

[Luke Wagner [:luke]]

Ah, it seems to only reproduce on 32-bit.

Comment 13

[Luke Wagner [:luke]]

Ok, figured it out: JSDependentString::undepend is wrong to turn a dependent string into a fixed string.  The reason is that if you turn a dependent string D1, which depends on B, into a flat string, then there may some other dependent string D2, which depends on D1 and whose chars point into the buffer owned by B.  When D1 is no longer dependent on B, it no longer roots B, so B and its chars may be free which leaves D2 with a dangling pointer.

This is a pretty hard case to hit since we rarely call undepend.  I was right that bug 675132 just happened to tickle malloc in the right way and did not cause the bug.  I think the real cause is bug 578189, when dependent strings started pointing directly at the chars of their base instead of storing an offset.

Comment 14

[Bill Keese]

Hi Luke, glad you tracked it down!  I figured it was something with used memory being freed (or something like that).   This brings back memories to my C and C++ days.

Comment 15

[Luke Wagner [:luke]]

(In reply to Bill Keese from comment #14)
Hi Bill, thanks for the great test-case; it would have been 10x harder without it!  (To wit, this isn't an ordinary C++ user-after-free bug, but a bug in the GC logic :)

Comment 16

[Luke Wagner [:luke]]

Attachment: shell test

Unfortunately, there isn't a trivial fix.  When a dependent string gets undepended, there isn't an efficient way to find all strings which depend on it.  So, instead, I added a new "JSUndependedString" string subtype which is a JSFixedString that also has a base.  The patch is relatively straightforward, but I'd like to let this bake on trunk for a while and then land on aurora/beta right before the next release.  To land on trunk, I created bug 765297 as a cover.

Comment 17

[Luke Wagner [:luke]]

CC'ing Jesse for fuzzer material in comment 16.

Comment 18

[David Mandelin [:dmandelin]]

Luke, I think you told me this goes back to Fx4, so I marked affected for esr10. Correct me if I'm wrong.

Marking sec-critical because comment 0 says it overwrites memory it shouldn't.

Comment 19

[Luke Wagner [:luke]]

Attachment: fix

[Approval Request Comment]
User impact if declined: security vulnerability; may crash (though rare)
Fix Landed on Version: FF16
Risk to taking this patch (and alternatives if risky): low but not very low

To restate comment 16, a fix has been on trunk for a couple of days (under bug 765297).  Also, what is the best day to land these? (Note: the bug is also on release.)

Comment 20

[Luke Wagner [:luke]]

(Carrying over njn's review since the rebasing was purely syntactic.)

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 635967 [details] [diff] [review]
fix

[Triage Comment]
Please land on branches ASAP, since this is an sg:crit with non-zero risk.

Comment 22

[Luke Wagner [:luke]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b5800cf2020
https://hg.mozilla.org/releases/mozilla-beta/rev/2a1099b9f468
https://hg.mozilla.org/releases/mozilla-esr10/rev/8b5d51c4cdac

Comment 23

[u279076]

Reproduced with Firefox 13.0.1.

Unreproducible with:
 * Firefox 14.0b12
 * Firefox 15.0a2 2012-07-11
 * Firefox 16.0a1 2012-07-11

Still reproducible with:
 * Firefox 10.0.6esrpre 2012-07-11

When it fails in ESR I get the following errors:
--
Tried to register widget with id==dijitMenuItemFoc_stcMi but that id is already registered @ http://bill.dojotoolkit.org/1.7/dijit/registry.js:31
Tried to register widget with id==contentTabs_menu but that id is already registered @ http://bill.dojotoolkit.org/1.7/dijit/registry.js:31
--
The ids appear in plain-text normally though, I don't see a problem there.

Perhaps we need a follow-up fix for the ESR branch?

Comment 24

[u279076]

Alex, can you please add a follow-up comment here about status-firefox-esr10:fixed? This bug is still reproducible for me on ESR.

Comment 25

[Luke Wagner [:luke]]

Wow, it looks like mozilla-esr10 has a subtle different from all the other releases that didn't show up as a conflict in the patch rebase.  Fortunately, the fix is trivial (just doing what the already-landed patch does in 1 more place).

Comment 26

[Luke Wagner [:luke]]

I confirmed the fix locally and pushed with over-the-shoulder review from Terrence:
https://hg.mozilla.org/releases/mozilla-esr10/rev/269362063f74