Closed Bug 552216 (CVE-2010-1028) Opened 14 years ago Closed 14 years ago

WOFF heap corruption due to integer overflow

Categories

(Core :: Layout: Text and Fonts, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4
Tracking Status
blocking1.9.2 --- .2+
status1.9.2 --- .2-fixed
status1.9.1 --- unaffected

People

(Reporter: bsterne, Assigned: jfkthame)

References

Details

(Keywords: crash, testcase, verified1.9.2, Whiteboard: [sg:critical?])

Attachments

(2 files, 2 obsolete files)

Attached file 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
OS: Mac OS X → All
Whiteboard: [sg:critical?]
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.
Assignee: nobody → jfkthame
Attachment #432363 - Flags: review?(jdaggett)
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/
blocking1.9.1: --- → ?
Switching the blocking request to 1.9.2, woff is not supported in 1.9.1 (Fx 3.5).
blocking1.9.1: ? → ---
blocking1.9.2: --- → ?
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.
Summary: Investigate reported WOFF parser buffer overflow → WOFF heap corruption due to integer overflow
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] [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.
Attachment #432363 - Flags: review?(jdaggett) → review+
Attachment #432363 - Attachment is obsolete: true
Attachment #432408 - Flags: approval1.9.2.3?
Attachment #432408 - Flags: approval1.9.2.2?
Attachment #432408 - Attachment is obsolete: true
Attachment #432409 - Flags: approval1.9.2.3?
Attachment #432409 - Flags: approval1.9.2.2?
Attachment #432408 - Flags: approval1.9.2.3?
Attachment #432408 - Flags: approval1.9.2.2?
Attached patch crashtest.diffSplinter Review
Here's a crashtest.  Please land after the fix has shipped on the 1.9.2 branch.
Flags: in-testsuite?
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.
Attachment #432409 - Flags: approval1.9.2.3?
Attachment #432409 - Flags: approval1.9.2.2?
Attachment #432409 - Flags: approval1.9.2.2+
(marking blocking - also, is this now FIXED on trunk?)
blocking1.9.2: ? → .2+
Pushed to 1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/827a6883442f
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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)?
Keywords: crash, testcase
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).
Keywords: verified1.9.2
Alias: CVE-2010-1028
Attachment #432405 - Flags: superreview+
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.
Attachment #432409 - Flags: superreview+
Group: core-security
Target Milestone: --- → mozilla1.9.3a4
(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;
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
You need to log in before you can comment on or make changes to this bug.