Bugzilla@Mozilla – Bug 552216
WOFF heap corruption due to integer overflow
Last modified: 2010-04-14 23:24:22 PDT
Summon comment box
Created attachment 432357 [details] Proof of concept Evgeny L. from Vulndisco reported a crash due to a buffer overflow in our WOFF parser. I am able to reproduce the crash, but the stack I get doesn't appear to be in font parser code. I'll try it in a debug build shortly to see if it looks any different. Here's the crash report where it looks like we're calling null: 0bc35855-ee77-4c61-b469-c7c482100313 To reproduce, run the attached python script ff1.py and load http://localhost:8080/1.html
More useful link to crash report: http://crash-stats.mozilla.com/report/index/0bc35855-ee77-4c61-b469-c7c482100313
On windows I see bp-37919069-18f3-4289-aa5d-33b6a2100313 which makes a little more sense -- at least the crash address looks like it came from the bogus server data.
Also slightly different bp-4119d51b-c798-4c23-82f4-7aa652100313
we should try and squeeze this in to fx 3.6.2. We need to confirm but it's possible this is the zeroday reported by Evgeny and Secunia at http://secunia.com/advisories/38608/
Switching the blocking request to 1.9.2, woff is not supported in 1.9.1 (Fx 3.5).
I wonder what is causing a few crash reports with the same signature as in comment 2 to show up for fx3.5.8 nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) | nsInputStreamPump::OnStateStop() http://crash-stats.mozilla.com/report/index/b1ab0b18-0e4a-4545-8be4-382692100301 http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=nsCOMPtr_base%3A%3Aassign_assuming_AddRef%28nsISupports*%29%20|%20nsInputStreamPump%3A%3AOnStateStop%28%29&date=&range_value=8&range_unit=weeks&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=nsCOMPtr_base%3A%3Aassign_assuming_AddRef%28nsISupports*%29%20|%20nsInputStreamPump%3A%3AOnStateStop%28%29 there are also some 3.5.8 and 3.5.3 reports on the signature in comment 3 nsStreamListenerTee::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=nsStreamListenerTee%3A%3AOnStopRequest%28nsIRequest*%2C%20nsISupports*%2C%20unsigned%20int%29&date=&range_value=2&range_unit=weeks&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=nsStreamListenerTee%3A%3AOnStopRequest%28nsIRequest*%2C%20nsISupports*%2C%20unsigned%20int%29
Note that none of these crash signatures are closely associated with the woff-decoder bug (even though on 3.6, they may be caused by it). The bug is such that we end up using zlib to decompress into a too-small buffer; we therefore overwrite some other chunk of memory with the decomressed data; and subsequently crash when another part of the code makes use of its now-damaged data. So similar crash signatures could occur as a result of more or less any memory-stomping bug. So all this tells us is that there must be something in 3.5.x that can trash memory in a vaguely similar way. It's certainly not the woff code in that case, as it isn't even present in the 1.9.1 tree. Maybe there's some other compressed-format decoder such as jpeg or png that can be fooled in a similar way? If we had a proof-of-concept that triggers the crashes on 3.5.x we could probably locate the problem there.
Do we need to allow orig length anywhere near 4G-3? Even given machines that can handle it that's a ridiculous font size -- no reasonable font will be anything close to that by several orders of magnitude and it allows an unreasonable font to suck up huge amounts of your resources. On OS X 10.5 the biggest font I have is 华文细黑.ttf at 17.4 Mb, followed by AppleGothic at 15. Half of 'em are under 1Mb.
CC'ing the reporter
(In reply to comment #5) > we should try and squeeze this in to fx 3.6.2. > > We need to confirm but it's possible this is the zeroday reported by Evgeny and > Secunia at http://secunia.com/advisories/38608/ Exactly, this is the bug i've found.
(In reply to comment #9) > Do we need to allow orig length anywhere near 4G-3? Well, of course that's a ridiculous size; I'm not sure how we would pick an appropriate smaller limit, though. And if orig length of an individual table is that large, the file as a whole will fail sanityCheck a moment later as the total size will be excessive.
Comment on attachment 432363 [details] [review] check for potential arithmetic overflow This patch is OK but I'd prefer a patch that just made offs, orig and comp all uint64_t. That's a lot more robust.
Created attachment 432405 [details] [review] use 64-bit arithmetic to avoid risk of overflow
Pushed: http://hg.mozilla.org/mozilla-central/rev/aaf4b4f00941 plus Windows bustage fix: http://hg.mozilla.org/mozilla-central/rev/30267b65a901
Created attachment 432408 [details] [review] patch for 1.9.2 branch, includes followup windows bustage fix
Argh, additional bustage fix: http://hg.mozilla.org/mozilla-central/rev/ba99b3a279cf
Created attachment 432409 [details] [review] patch for 1.9.2 branch, includes followups
Created attachment 432419 [details] crashtest.diff Here's a crashtest. Please land after the fix has shipped on the 1.9.2 branch.
Comment on attachment 432409 [details] [review] patch for 1.9.2 branch, includes followups a1922=beltzner, please land on mozilla1.9.2 immediately and follow up here with changeset. Evgeny, thanks for confirming that this is the bug.
(marking blocking - also, is this now FIXED on trunk?)
Pushed to 1.9.2 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/827a6883442f
Does this apply to 1.9.1 branch as well? Might not be good to ship this fix on 1.9.2 while leaving 1.9.1 vulnerable, if so.
1.9.1 is unaffected (see flags above).
Since this is a security fix, shouldn't the patch be super-reviewed, as per our review policy (http://www.mozilla.org/hacking/reviewers.html)?
I verified the problem and the fix with the attached PoC using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.2pre) Gecko/20100315 Namoroka/3.6.2pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100315 Namoroka/3.6.2pre (.NET CLR 3.5.30729).
Comment on attachment 432409 [details] [review] patch for 1.9.2 branch, includes followups sr=dveditz (for reed). this patch had many many eyes on it before it landed (including mine) but if a formal marker makes people happy then here it is.
*** Bug 554189 has been marked as a duplicate of this bug. ***
(In reply to comment #27) > (From update of attachment 432409 [details] [review]) > sr=dveditz (for reed). this patch had many many eyes on it before it landed > (including mine) but if a formal marker makes people happy then here it is. So many that they all failed to notice the obvious flaw below before it landed (fixed in comment 17)? - if (tableTotal > 0xffffffffU - orig) { + tableTotal += orig; + if (tableTotal > 0xffffffffU) { return eWOFF_invalid; } tableTotal += orig;
It could be patched much more easy without inc of a bit's: uint32_t tableTotal = 0; //... uint32_t orig = READ32BE(dirEntry->origLen); //... #define MAX(x,y) ((x)>(y)?(x):(y)) if ( tableTotal + orig < MAX(tableTotal,orig) ) { return eWOFF_invalid; } else { tableTotal += orig; }
Any remaining issues suggested in comment 7 and 8 are now being tracked in Bug 555139 - (CVE-2010-1122)
verified FIXED on build: Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.1) Gecko/20100330 Namoroka/3.6.1 Fennec/1.0.1