Last Comment Bug 552216 - (CVE-2010-1028) WOFF heap corruption due to integer overflow
(CVE-2010-1028)
: WOFF heap corruption due to integer overflow
Status: RESOLVED FIXED
[sg:critical?]
: crash, testcase, verified1.9.2
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla1.9.3a4
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
: 554189 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-13 13:41 PST by Brandon Sterne (:bsterne)
Modified: 2010-04-14 23:24 PDT (History)
26 users (show)
mats: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2+
.2-fixed
unaffected


Attachments
check for potential arithmetic overflow (924 bytes, patch)
2010-03-13 15:18 PST, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Review
use 64-bit arithmetic to avoid risk of overflow (1.62 KB, patch)
2010-03-14 04:57 PDT, Jonathan Kew (:jfkthame)
roc: review+
dveditz: superreview+
Details | Diff | Review
patch for 1.9.2 branch, includes followup windows bustage fix (2.39 KB, patch)
2010-03-14 05:48 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Review
patch for 1.9.2 branch, includes followups (2.53 KB, patch)
2010-03-14 06:03 PDT, Jonathan Kew (:jfkthame)
dveditz: superreview+
mbeltzner: approval1.9.2.2+
Details | Diff | Review

Description Brandon Sterne (:bsterne) 2010-03-13 13:41:54 PST
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
Comment 1 Brandon Sterne (:bsterne) 2010-03-13 13:45:14 PST
More useful link to crash report:
http://crash-stats.mozilla.com/report/index/0bc35855-ee77-4c61-b469-c7c482100313
Comment 2 Daniel Veditz [:dveditz] 2010-03-13 14:29:59 PST
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.
Comment 3 Daniel Veditz [:dveditz] 2010-03-13 14:36:19 PST
Also slightly different bp-4119d51b-c798-4c23-82f4-7aa652100313
Comment 5 chris hofmann 2010-03-13 15:31:25 PST
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/
Comment 6 Jonathan Kew (:jfkthame) 2010-03-13 15:38:25 PST
Switching the blocking request to 1.9.2, woff is not supported in 1.9.1 (Fx 3.5).
Comment 8 Jonathan Kew (:jfkthame) 2010-03-13 16:11:00 PST
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.
Comment 9 Daniel Veditz [:dveditz] 2010-03-13 21:45:43 PST
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.
Comment 10 Daniel Veditz [:dveditz] 2010-03-13 21:46:49 PST
CC'ing the reporter
Comment 11 Evgeny L. 2010-03-13 22:18:45 PST
(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.
Comment 12 Jonathan Kew (:jfkthame) 2010-03-14 00:54:18 PST
(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 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-14 03:14:50 PDT
Comment on attachment 432363 [details] [diff] [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.
Comment 14 Jonathan Kew (:jfkthame) 2010-03-14 04:57:25 PDT
Created attachment 432405 [details] [diff] [review]
use 64-bit arithmetic to avoid risk of overflow
Comment 15 Jonathan Kew (:jfkthame) 2010-03-14 05:27:26 PDT
Pushed:
 http://hg.mozilla.org/mozilla-central/rev/aaf4b4f00941
plus Windows bustage fix:
 http://hg.mozilla.org/mozilla-central/rev/30267b65a901
Comment 16 Jonathan Kew (:jfkthame) 2010-03-14 05:48:50 PDT
Created attachment 432408 [details] [diff] [review]
patch for 1.9.2 branch, includes followup windows bustage fix
Comment 17 Jonathan Kew (:jfkthame) 2010-03-14 05:58:36 PDT
Argh, additional bustage fix:
http://hg.mozilla.org/mozilla-central/rev/ba99b3a279cf
Comment 18 Jonathan Kew (:jfkthame) 2010-03-14 06:03:36 PDT
Created attachment 432409 [details] [diff] [review]
patch for 1.9.2 branch, includes followups
Comment 19 Mats Palmgren (:mats) 2010-03-14 07:13:12 PDT
Created attachment 432419 [details] [diff]
crashtest.diff

Here's a crashtest.  Please land after the fix has shipped on the 1.9.2 branch.
Comment 20 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-14 07:49:06 PDT
Comment on attachment 432409 [details] [diff] [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.
Comment 21 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-14 07:51:17 PDT
(marking blocking - also, is this now FIXED on trunk?)
Comment 22 Jonathan Kew (:jfkthame) 2010-03-14 08:01:14 PDT
Pushed to 1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/827a6883442f
Comment 23 Robert Kaiser (not working on stability any more) 2010-03-14 09:05:01 PDT
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.
Comment 24 Jonathan Kew (:jfkthame) 2010-03-14 09:07:07 PDT
1.9.1 is unaffected (see flags above).
Comment 25 Reed Loden [:reed] (use needinfo?) 2010-03-14 16:16:43 PDT
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)?
Comment 26 Al Billings [:abillings] 2010-03-15 13:24:45 PDT
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 27 Daniel Veditz [:dveditz] 2010-03-20 09:47:20 PDT
Comment on attachment 432409 [details] [diff] [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.
Comment 29 Daniel Veditz [:dveditz] 2010-03-22 16:13:45 PDT
*** Bug 554189 has been marked as a duplicate of this bug. ***
Comment 30 Daniel Cater 2010-03-23 06:09:13 PDT
(In reply to comment #27)
> (From update of attachment 432409 [details] [diff] [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;
Comment 31 inikakinache 2010-03-23 11:20:35 PDT
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;
}
Comment 32 chris hofmann 2010-03-27 08:54:16 PDT
Any remaining issues suggested in comment 7 and 8 are now being tracked in  Bug 555139 -  (CVE-2010-1122)
Comment 33 Aakash Desai [:aakashd] 2010-03-31 16:06:08 PDT
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

Note You need to log in before you can comment on or make changes to this bug.