Bug 764296 (CVE-2012-1962)

memory corruption of strings

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Bill Keese, Assigned: luke)

Tracking

({regression, sec-critical})

Trunk
x86
All
regression, sec-critical
Points:
---

Firefox Tracking Flags

(firefox13 wontfix, firefox14+ verified, firefox15+ verified, firefox16+ verified, firefox-esr1014+ fixed)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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"
(Reporter)

Updated

5 years ago
(Reporter)

Comment 1

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

Comment 2

5 years ago
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

Updated

5 years ago
Component: Tabbed Browser → DOM
Product: Firefox → Core
QA Contact: tabbed.browser → general
Version: 14 Branch → Trunk

Comment 3

5 years ago
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

Comment 4

5 years ago
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

Comment 5

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

Comment 6

5 years ago
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

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

Comment 8

5 years ago
(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

5 years ago
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
(Assignee)

Comment 10

5 years ago
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

5 years ago
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
(Assignee)

Comment 12

5 years ago
Ah, it seems to only reproduce on 32-bit.
(Assignee)

Comment 13

5 years ago
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
(Reporter)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
(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 :)
(Assignee)

Comment 16

5 years ago
Created attachment 633579 [details]
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.
(Assignee)

Comment 17

5 years ago
CC'ing Jesse for fuzzer material in comment 16.
(Assignee)

Updated

5 years ago
Attachment #633579 - Attachment is patch: false
(Assignee)

Updated

5 years ago
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
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.
status-firefox-esr10: --- → affected
status-firefox16: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox14: ? → +
tracking-firefox15: ? → +
tracking-firefox16: --- → +
Keywords: sec-critical
Whiteboard: [js:p1:fx16]
(Assignee)

Updated

5 years ago
status-firefox16: affected → fixed
Assignee: general → luke
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox13: affected → wontfix
tracking-firefox-esr10: ? → 14+
Resolution: --- → FIXED
Whiteboard: [js:p1:fx16] → [js:p1:fx16] fixed by bug 765297
(Assignee)

Comment 19

5 years ago
Created attachment 635967 [details] [diff] [review]
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.)
Attachment #635967 - Flags: review+
Attachment #635967 - Flags: approval-mozilla-esr10?
Attachment #635967 - Flags: approval-mozilla-beta?
Attachment #635967 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 20

5 years ago
(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+
(Assignee)

Comment 22

5 years ago
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
status-firefox-esr10: affected → fixed
status-firefox14: affected → fixed
status-firefox15: affected → fixed
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-firefox-esr10: fixed → affected
status-firefox14: fixed → verified
status-firefox15: fixed → verified
status-firefox16: fixed → verified
Status: RESOLVED → VERIFIED

Updated

5 years ago
status-firefox-esr10: affected → fixed
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
(Assignee)

Comment 25

5 years ago
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).
(Assignee)

Comment 26

5 years ago
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

Updated

5 years ago
Blocks: 776542
You need to log in before you can comment on or make changes to this bug.