As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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 | Splinter 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 | Splinter 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 | Splinter 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 | Splinter Review

Description User image 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 User image 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 User image 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 User image Daniel Veditz [:dveditz] 2010-03-13 14:36:19 PST
Also slightly different bp-4119d51b-c798-4c23-82f4-7aa652100313
Comment 5 User image 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 User image 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 User image 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 User image 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 User image Daniel Veditz [:dveditz] 2010-03-13 21:46:49 PST
CC'ing the reporter
Comment 11 User image 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 User image 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 User image Robert O'Callahan (:roc) (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 User image 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 User image 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 User image 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 User image Jonathan Kew (:jfkthame) 2010-03-14 05:58:36 PDT
Argh, additional bustage fix:
http://hg.mozilla.org/mozilla-central/rev/ba99b3a279cf
Comment 18 User image 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 User image 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 User image 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 User image Mike Beltzner [:beltzner, not reading bugmail] 2010-03-14 07:51:17 PDT
(marking blocking - also, is this now FIXED on trunk?)
Comment 22 User image 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 User image Robert Kaiser 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 User image Jonathan Kew (:jfkthame) 2010-03-14 09:07:07 PDT
1.9.1 is unaffected (see flags above).
Comment 25 User image 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 User image 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 User image 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 User image Daniel Veditz [:dveditz] 2010-03-22 16:13:45 PDT
*** Bug 554189 has been marked as a duplicate of this bug. ***
Comment 30 User image 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 User image 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 User image 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 User image 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.