Closed Bug 764296 (CVE-2012-1962) Opened 12 years ago Closed 12 years ago

memory corruption of strings

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
firefox13 --- wontfix
firefox14 + verified
firefox15 + verified
firefox16 + verified
firefox-esr10 14+ fixed

People

(Reporter: wkeese, Assigned: luke)

References

()

Details

(Keywords: regression, sec-critical, Whiteboard: [js:p1:fx16][advisory-tracking+][qa?] fixed by bug 765297)

Attachments

(2 files)

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"
Based on http://bugs.dojotoolkit.org/ticket/15496 (but the info in this ticket is sufficient to reproduce and fix)
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
Component: Untriaged → Tabbed Browser
Keywords: regression
QA Contact: untriaged → tabbed.browser
Component: Tabbed Browser → DOM
Product: Firefox → Core
QA Contact: tabbed.browser → general
Version: 14 Branch → Trunk
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
Assignee: nobody → general
Blocks: 609440
Status: UNCONFIRMED → NEW
Component: DOM → JavaScript Engine
Ever confirmed: true
OS: Mac OS X → All
QA Contact: general → general
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
No longer blocks: 609440
> 8. Repeat Step 5, 6, 7 ,8 until fail to move tab
I tried repeat step 8 max 10 times
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).
>After more 10 tabs, I wasn't able to display 'NeweditorLayout10'
seems dojo bug?
(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.
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)
Blocks: 675132, 609440
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.
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
Ah, it seems to only reproduce on 32-bit.
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.
No longer blocks: 609440, 675132
Group: core-security
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.
(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 :)
Attached file 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.
CC'ing Jesse for fuzzer material in comment 16.
Attachment #633579 - Attachment is patch: false
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.
Assignee: general → luke
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [js:p1:fx16] → [js:p1:fx16] fixed by bug 765297
Attached patch fixSplinter Review
[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.)
Attachment #635967 - Flags: review+
Attachment #635967 - Flags: approval-mozilla-esr10?
Attachment #635967 - Flags: approval-mozilla-beta?
Attachment #635967 - Flags: approval-mozilla-aurora?
(Carrying over njn's review since the rebasing was purely syntactic.)
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.
Attachment #635967 - Flags: approval-mozilla-esr10?
Attachment #635967 - Flags: approval-mozilla-esr10+
Attachment #635967 - Flags: approval-mozilla-beta?
Attachment #635967 - Flags: approval-mozilla-beta+
Attachment #635967 - Flags: approval-mozilla-aurora?
Attachment #635967 - Flags: approval-mozilla-aurora+
Whiteboard: [js:p1:fx16] fixed by bug 765297 → [js:p1:fx16][advisory-tracking+] fixed by bug 765297
Alias: CVE-2012-1962
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?
Status: RESOLVED → VERIFIED
Alex, can you please add a follow-up comment here about status-firefox-esr10:fixed? This bug is still reproducible for me on ESR.
Whiteboard: [js:p1:fx16][advisory-tracking+] fixed by bug 765297 → [js:p1:fx16][advisory-tracking+][qa?] fixed by bug 765297
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).
I confirmed the fix locally and pushed with over-the-shoulder review from Terrence:
https://hg.mozilla.org/releases/mozilla-esr10/rev/269362063f74
Group: core-security
Blocks: 776542
You need to log in before you can comment on or make changes to this bug.