440230 - (CVE-2008-2785) Firefox CSSValue Array Memory Corruption Vulnerability

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: zipped PoC (multiple files) from ZDI

ZDI submitted the following advisory to us via mail to [e-mail]

-- ABSTRACT ------------------------------------------------------------

TippingPoint has identified a vulnerability affecting the following 
products:

    Mozilla Firefox 2.0.x
    Mozilla Firefox 3.0.x

-- VULNERABILITY DETAILS -----------------------------------------------

This vulnerability allows remote attackers to execute arbitrary code on
vulnerable installations of Mozilla Firefox. User interaction is
required to exploit this vulnerability in that the target must visit a
malicious page.

The specific flaw exists in the browser's handling reference counters to
the nsCSSValue:Array class. Creating more then 65,535 references will
overflow a 16-bit reference counter and therefore result in an erroneous
free() while the object still exists. Properly manipulated this can
result in arbitrary code execution under the context of the current
user.

Most reference counters are implemented as unsigned longs and can not be
feasible overflowed with too many reference. However, the
nsCSSValue::Array class uses a 16-bit wide value,
layout/style/nsCSSValue.h (lines 321-335):

void AddRef() {
  ++mRefCnt;
  NS_LOG_ADDREF(this, mRefCnt, "nsCSSValue::Array", sizeof(*this));
}
void Release() {
  --mRefCnt;
  NS_LOG_RELEASE(this, mRefCnt, "nsCSSValue::Array");
  if (mRefCnt == 0)
    delete this;
}

private:
  PRUint16 mRefCnt;
  PRUint16 mCount;

An attacker can create an arbitrary number of references to this object
by creating a style definition with counter-increment, counter-reset,
cursor or text-shadow attribute. Setting one of those attributes to an
arbitrary HTML object creates a reference to this object.

Creating more than 65535 HTML objects with reference to this style
definition will overflow the reference counter, which will lead to a
freeing of the object whilst references to it still exist.


-- CREDIT --------------------------------------------------------------

This vulnerability was discovered by:
    * Anonymous

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possible patch

I think we should do this here, and elsewhere in the tree.

The nsStyleStruct.{h,cpp} change shouldn't be needed on 1.9 or 1.8.  I haven't yet looked to see if additional changes should be needed there.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Note that I haven't tested this against the attached ZIP and don't plan to.  I don't have Java installed.  If somebody else wants to try it, you're welcome to.)

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd be interested in other people's opinions on whether they agree with this approach (including those already cc:ed).


Also, should I hold off on checking this in to trunk?  (If so, somebody else may need to land it for me.)

Comment 4

[Mike Schroepfer]

Note that for me on mac the testcase hung the browser for ~5minutes until I force-quit it.  Is that how long it takes on windows as well?

Comment 5

[Brendan Eich [:brendan]]

Are the PR_UINT32_MAX checks necessary? Possibly on 64-bit systems, but should we use a uint64 mRefCnt there to avoid having to worry?

It's not possible on 32-bit systems to add 2^32-1 refs from JS, of course.

/be

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> It's not possible on 32-bit systems to add 2^32-1 refs from JS, of course.

...assuming that we have no reference leak bugs.

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to comment #4)
> Note that for me on mac the testcase hung the browser for ~5minutes until I
> force-quit it.  Is that how long it takes on windows as well?

Yes, it took ages while it filled up memory and played games trying to force GC -- very brute-force. A motivated attacker might be able to make this more efficient and workable.

I haven't yet reproduced the claimed "launch calc.exe" PoC, but I did get shut down by Windows DEP which indicates a serious problem.

(In reply to comment #2)
> (Note that I haven't tested this against the attached ZIP and don't plan to.
> I don't have Java installed.

There are actually two testcases, one uses Flash and the other Java.

Comment 8

[Martijn Wargers (dead)]

Attachment: testcase

This is a simple html testcase that triggers the crash, without all the memory filling stuff.
This crashes after a gc, which happens after 20 seconds or so.

Comment 9

[Mike Schroepfer]

(In reply to comment #7)
> (In reply to comment #4)
> > Note that for me on mac the testcase hung the browser for ~5minutes until I
> > force-quit it.  Is that how long it takes on windows as well?
> 
> Yes, it took ages while it filled up memory and played games trying to force GC
> -- very brute-force. A motivated attacker might be able to make this more
> efficient and workable.
> 
> I haven't yet reproduced the claimed "launch calc.exe" PoC, but I did get shut
> down by Windows DEP which indicates a serious problem.
> 

If we are shut down by DEP doesn't that mean we are safe from the exploit?

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Only on systems with DEP available, so probably not Linux and OS X, and almost certainly not people with pre-x86-64 machines.  I guess you can turn DEP off, too, for some or all applications -- not sure what the defaults are.

Comment 11

[Brandon Sterne (:bsterne)]

(In reply to comment #8)
> This is a simple html testcase that triggers the crash, without all the memory
> filling stuff.
> This crashes after a gc, which happens after 20 seconds or so.

This didn't crash for me on Linux.  It definitely causes a hang, but I left the page open for ~5min before killing the browser.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008061015 Firefox/3.0

Comment 12

[Mike Schroepfer]

I got a crash on Mac only after leaving the tab open for quite a while and browsing to other sites in other tabs.  Just letting it sit didn't do it for me.

Comment 13

[Mike Schroepfer]

And david we good to land this on trunk?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I started an email discussion about how we should proceed.

I'd also note that we probably ought to wrap these if-conditions in NS_UNLIKELY.

Comment 15

[Ben Bucksch (:BenB)]

If you miss the AddRef, like the patch intentionally does, it's not a leak, but you still possibly access a wild pointer, no?

Comment 16

[Ben Bucksch (:BenB)]

Ah, no, nevermind. If we hit that case, no releases will work anymore for that object, it will indeed never be freed and leaked.

Comment 17

[chris hofmann]

I got this stack trace out of using the test case on winXP

http://crash-stats.mozilla.com/report/index/74cff7fa-3f32-11dd-a7e4-001cc45a2ce4

0  	mozcrt19.dll  	arena_dalloc_small  	 jemalloc.c:4094
1 	mozcrt19.dll 	arena_dalloc 	jemalloc.c:4196
2 	mozcrt19.dll 	free 	jemalloc.c:6009
3 	xul.dll 	nsThebesRegion::FreeRects 	mozilla/gfx/src/thebes/nsThebesRegion.cpp:183
4 	xul.dll 	ConvertNativeRegionToAppRegion 	mozilla/view/src/nsViewManager.cpp:402
5 	xul.dll 	nsViewManager::Refresh 	mozilla/view/src/nsViewManager.cpp:436
6 	xul.dll 	xul.dll@0x2fc068 	
7 	xul.dll 	HandleEvent 	mozilla/view/src/nsView.cpp:168
8 	xul.dll 	nsWindow::DispatchEvent 	mozilla/widget/src/windows/nsWindow.cpp:985
9 	xul.dll 	nsWindow::DispatchWindowEvent 	mozilla/widget/src/windows/nsWindow.cpp:1010
10 	xul.dll 	xul.dll@0x2ec341 	
11 	xul.dll 	nsWindow::ProcessMessage 	mozilla/widget/src/windows/nsWindow.cpp:4288
12 	xul.dll 	nsWindow::WindowProc 	mozilla/widget/src/windows/nsWindow.cpp:1200
13 	user32.dll 	InternalCallWinProc 	
14 	user32.dll 	UserCallWinProcCheckWow 	
15 	user32.dll 	DispatchClientMessage 	
16 	user32.dll 	__fnDWORD 	
17 	ntdll.dll 	KiUserCallbackDispatcher 	
18 	xul.dll 	nsScriptSecurityManager::GetObjectPrincipal 	
19 	user32.dll 	DispatchMessageW 	
20 	xul.dll 	nsAppShell::ProcessNextNativeEvent 	mozilla/widget/src/windows/nsAppShell.cpp:148
21 	xul.dll 	nsBaseAppShell::OnProcessNextEvent 	mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:278
22 	xul.dll 	nsThread::ProcessNextEvent 	mozilla/xpcom/threads/nsThread.cpp:497
23 	xul.dll 	nsBaseAppShell::Run 	mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:170
24 	nspr4.dll 	PR_GetEnv 	
25 	firefox.exe 	wmain 	mozilla/toolkit/xre/nsWindowsWMain.cpp:87
26 	firefox.exe 	firefox.exe@0x217f 	
27 	kernel32.dll 	BaseProcessStart 	

It turns out that Firefox 3.0 Crash Reports [@ arena_dalloc_small ] has been bouncing around the #1-3 top crash reports for the last few days since release, but this stack trace looks fairly unique among all the reports with that stack signature.  

Is there a chance that we will fix other crashes with this fix?  

Could there be other ways to get to exploitable crashes around area_dalloc_small that we need to think about?

search down for "0x5f  	153  	chofmann looking at bugs" to see my crash report among the other arena_dalloc_small reports in 

http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&range_unit=days&version=Firefox%3A3.0&signature=arena_dalloc_small&range_value=3

Comment 18

[Johnny Stenback (:jst)]

W/o looking at this in extreme detail I would *not* expect this to fix any crashes that people are seeing unless they're being directly exploited by someone aware of this problem.

Comment 19

[chris hofmann]

I also got a stack trace out of a Mac crash.

0  	XUL  	nsCSSValue::DoReset  	 mozilla/layout/style/nsCSSStyleRule.cpp:326
1 	XUL 	nsCSSValue::DoReset 	mozilla/layout/style/nsCSSStyleRule.cpp:261
2 	XUL 	nsCSSCompressedDataBlock::Destroy 	mozilla/layout/style/nsCSSDataBlock.cpp:532
3 	XUL 	nsCSSDeclaration::~nsCSSDeclaration 	mozilla/layout/style/nsCSSDataBlock.cpp:109
4 	XUL 	CSSStyleRuleImpl::~CSSStyleRuleImpl 	mozilla/layout/style/nsCSSStyleRule.cpp:240
5 	XUL 	nsCSSRule::Release 	mozilla/layout/style/nsCSSPseudoElements.cpp:64
6 	XUL 	nsAttrValue::EnsureEmptyMiscContainer 	mozilla/content/base/src/nsAtomListUtils.cpp:982
7 	XUL 	nsAttrValue::Reset 	mozilla/content/base/src/nsAtomListUtils.cpp:149
8 	XUL 	nsAttrAndChildArray::Clear 	mozilla/content/base/src/nsAtomListUtils.cpp:177
9 	XUL 	nsAttrAndChildArray::~nsAttrAndChildArray 	mozilla/content/base/src/nsAtomListUtils.cpp:133
10 	XUL 	nsGenericElement::~nsGenericElement 	mozilla/content/base/src/nsGenericElement.cpp:1191
11 	XUL 	nsHTMLBodyElement::~nsHTMLBodyElement 	mozilla/content/html/content/src/nsHTMLBRElement.cpp:57
12 	XUL 	nsNodeUtils::LastRelease 	mozilla/content/base/src/nsNodeUtils.cpp:245
13 	XUL 	nsGenericElement::Release 	mozilla/content/base/src/nsGenericElement.cpp:3571
14 	XUL 	nsXPCOMCycleCollectionParticipant::Unroot 	nsCycleCollectionParticipant.cpp:74
15 	XUL 	nsCycleCollector::CollectWhite 	mozilla/xpcom/base/nsCycleCollector.cpp:1676
16 	XUL 	nsCycleCollector::FinishCollection 	mozilla/xpcom/base/nsCycleCollector.cpp:2434
17 	XUL 	nsCycleCollector_finishCollection 	mozilla/xpcom/base/nsCycleCollector.cpp:2916
18 	XUL 	XPCCycleCollectGCCallback 	mozilla/js/src/xpconnect/src/nsXPConnect.cpp:453
19 	libmozjs.dylib 	js_GC 	mozilla/js/src/jsgc.c:3526
20 	libmozjs.dylib 	JS_GC 	mozilla/js/src/jslong.c:2469
21 	XUL 	nsXPConnect::Collect 	mozilla/js/src/xpconnect/src/nsXPConnect.cpp:529
22 	XUL 	nsCycleCollector::Collect 	mozilla/xpcom/base/nsCycleCollector.cpp:2250
23 	XUL 	nsCycleCollector_collect 	mozilla/xpcom/base/nsCycleCollector.cpp:2898
24 	XUL 	nsJSContext::CC 	mozilla/dom/src/base/nsJSEnvironment.cpp:3346
25 	XUL 	nsUserActivityObserver::Observe 	mozilla/dom/src/base/nsJSEnvironment.cpp:270
26 	XUL 	nsObserverList::NotifyObservers 	mozilla/xpcom/ds/nsObserverList.cpp:128
27 	XUL 	nsObserverService::NotifyObservers 	mozilla/xpcom/ds/nsObserverService.cpp:181
28 	XUL 	nsUITimerCallback::Notify 	mozilla/content/events/src/nsEventStateManager.cpp:208
29 	XUL 	nsTimerImpl::Fire 	mozilla/xpcom/threads/nsTimerImpl.cpp:403
30 	XUL 	nsTimerEvent::Run 	mozilla/xpcom/threads/nsTimerImpl.cpp:490
31 	XUL 	nsThread::ProcessNextEvent 	mozilla/xpcom/threads/nsThread.cpp:510
32 	XUL 	NS_ProcessPendingEvents_P 	nsThreadUtils.cpp:180
33 	XUL 	nsBaseAppShell::NativeEventCallback 	mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:121
34 	XUL 	nsAppShell::ProcessGeckoEvents 	mozilla/widget/src/cocoa/nsAppShell.mm:299
35 	CoreFoundation 	CoreFoundation@0x7260d 	
36 	CoreFoundation 	CoreFoundation@0x72cf7 	
37 	HIToolbox 	HIToolbox@0x2fda3 	
38 	HIToolbox 	HIToolbox@0x2fbbc 	
39 	HIToolbox 	HIToolbox@0x2fa30 	
40 	AppKit 	AppKit@0x40504 	
41 	AppKit 	AppKit@0x3fdb7 	
42 	AppKit 	AppKit@0x38df2 	
43 	XUL 	nsAppShell::Run 	mozilla/widget/src/cocoa/nsAppShell.mm:588
44 	XUL 	nsAppStartup::Run 	mozilla/toolkit/components/startup/src/nsAppStartup.cpp:181
45 	XUL 	XRE_main 	mozilla/toolkit/xre/nsAppRunner.cpp:3170
46 	firefox-bin 	main 	mozilla/browser/app/nsBrowserApp.cpp:158
47 	firefox-bin 	start 	crt.c:272
48 	firefox-bin 	start 	
49 		@0x0 	

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

roc/jst: dbaron's afk until the 29th, and I'm not involved in this email discussion that was started; where are we on landing this patch? has it been able to bake on mozilla-central?

Comment 21

[Johnny Stenback (:jst)]

This hasn't landed yet and we're waiting for more work in this area (by dmandelin) to attempt to identify other areas with the same problem before publishing this change. I'll be doing further work on this issue as well.

Comment 22

[David Mandelin [:dmandelin]]

Attachment: mRefCnt and mRefCount member variables report

This is a list of member variables named mRefCnt and mRefCount prepared using Dehydra on a Linux trunk build. The name and source location of each member is given. They are grouped by type: showing bits and S/U for signed/unsigned. In parens are given any typedefs or wrapper classes around the mValue member.

The notable stuff is near the top. The nsCSSValue::Array::mRefCnt being 16-bit unsigned is shown. There is also an Assertion::mRefCnt, which is 16-bit signed. Then there is a group of 14 variables that are 32-bit signed or unsigned that are not defined using nsrefcnt, but through PRInt32 or PRUInt32 directly, and 3 more that are nsrefcnt, but not using any wrapper. So those 17 might merit additional attention.

I'm going to search for more stuff by looking for increments inside functions with likely names and maybe broadening the name search a bit.

Comment 23

[David Mandelin [:dmandelin]]

Attachment: Class fields assigned in refcounting methods

This is a list of class fields assigned in "refcounting methods" and the expressions used to assign them. A "refcounting method" was defined as any method with a name in:

  AddRef,Release,AddReference,ReleaseReference,Hold,Drop,incr,decr

I didn't spot any bitmasking operations other than the known one in the cycle counting decr. This check did turn up a few other ref count members. I checked them out and none are 16-bit. Let me know if you want the source listing of where they are defined.

Comment 24

[David Mandelin [:dmandelin]]

Attachment: Scripts used to generate my results

Comment 25

[Johnny Stenback (:jst)]

Attachment: Minimal fix to make us safe on 32-bit systems.

This is a minimal patch to make us safe against this problem on 32-bit systems. It's dbaron's 16-bit refcount fix, and the same fix applied to the other 16-bit refcount that dmandelin found. Once this is in we can worry about making our reference counting safe on 64-bit systems IMO, in a separate bug.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 327032 [details] [diff] [review]
Minimal fix to make us safe on 32-bit systems.

A good start.

However, I agree with dbaron that we really should do this for all refcounts, because a bug is still triggerable if someone finds a refcount leak. We don't find too many of those these days, but still...

Comment 27

[Johnny Stenback (:jst)]

Agreed, but I don't think we need to solve that before landing the fix for this issue, and possibly opening this bug up. And I think when we fix that kind of problem *anywhere* we need to fix them across the board (as best as we can).

Comment 28

[Mike Schroepfer]

nice work folks (go static analysis for the coverage win!).  We should land this on trunk, then on both branches in quick succession.  Before doing so I'd like to understand our 2.0.0.x and 3.0.1 schedules to make sure we can push all updates fast.  

Comment 29

[Al Billings [:abillings - ex-MoCo]]

 Martijn's testcase doesn't do anything on branch for me or Sam. Running the PoC case on a server, I simply get the "Warning: Unresponsive script" dialog every 20 seconds or so and nothing else happens. If I tell the script to continue, the process just repeats for another 20 seconds or so. If I stop the script, nothing happens either. 

This was with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.16pre) Gecko/2008062703 BonEcho/2.0.0.16pre.

I'm going to double-check this on Windows XP.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Behavior is the same on XP (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.16pre) Gecko/2008062703 BonEcho/2.0.0.16pre). Martijn's testcase does not visibly do anything. Running the PoC, I get a unresponsive script prompt (and Firefox is hung until I get it) but can stop the script at that point (or continue it until the next time).

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 327032 [details] [diff] [review]
Minimal fix to make us safe on 32-bit systems.

a=beltzner, are we landing this under a cover bug? Either way, we should hold off until we're ready to roll builds.

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 327032 [details] [diff] [review]
Minimal fix to make us safe on 32-bit systems.

This looks like it applies to the 1.8 branch, and a quick MXR shows only these two int16 mRefCnt variables. Requesting approval unless Johnny says we need a different patch.

Comment 33

[Al Billings [:abillings - ex-MoCo]]

I set dom.max_script_run_time to 10,000 in about:config and tried the PoC again
on Firefox 2.0.0.14. While the script happily uses my resources, nothing else
ever happens. All it does is hang my browser. If I leave the default values for
dom.max_script_run_time in place, I get a prompt to stop the script.

Comment 34

[Samuel Sidler (old account; do not CC)]

Comment on attachment 327032 [details] [diff] [review]
Minimal fix to make us safe on 32-bit systems.

Approved for 1.8.1.16. a=ss for release-drivers.

Please land on the MOZILLA_1_8_BRANCH when you land on 1.9.

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we have a cover bug under which we're going to land this? jst, are you planning on landing this tonight or tomorrow morning?

Comment 36

[Johnny Stenback (:jst)]

I can land whenever, but there's no cover bug. If others think we need one let me know, otherwise I can either just check in referring to this closed bug, or I can check in w/o a bug reference in the commit message.

Comment 37

[Johnny Stenback (:jst)]

Fixed on trunk and branches. Bug 443059 filed as a followup.

Comment 38

[Al Billings [:abillings - ex-MoCo]]

As I mention in comment 33, I see the same behavior in 2.0.0.14 and 2.0.0.16 with this.

Comment 39

[Josh Bressers]

This is also CVE-2008-2785

Comment 40

[Daniel Veditz [:dveditz]]

(In reply to comment #33)
> I set dom.max_script_run_time to 10,000 in about:config and tried the PoC
> again on Firefox 2.0.0.14. While the script happily uses my resources,
> nothing else ever happens. All it does is hang my browser.

When it works it alerts a url-escaped string, then after a pause alerts "Done", and when you click "OK" firefox disappears and calc.exe runs. It does this within the bounds of the current max script time. If it's going to work you shouldn't hit the unresponsive script dialog, or maybe once if you've got a slow or loaded machine.

I never got the PoC to work on 2.0.0.x but by code inspection it has the same problem. That probably means the PoC is sensitive to memory layout which unfortunately also means the PoC not working is not necessarily proof that we fixed it.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I landed the remainder of my original patch as http://hg.mozilla.org/mozilla-central/rev/447d12ffe764 since it seemed better in than out.

Comment 42

[Alexander Sack]

Comment on attachment 327032 [details] [diff] [review]
Minimal fix to make us safe on 32-bit systems.

a=asac for 1.8.0