552216 - (CVE-2010-1028) WOFF heap corruption due to integer overflow

Status: RESOLVED FIXED

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

Description

[Brandon Sterne (:bsterne)]

Attachment: 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)]

More useful link to crash report:
http://crash-stats.mozilla.com/report/index/0bc35855-ee77-4c61-b469-c7c482100313

Comment 2

[Daniel Veditz [:dveditz]]

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]]

Also slightly different bp-4119d51b-c798-4c23-82f4-7aa652100313

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: check for potential arithmetic overflow

Yes, it's a flaw in the woff decoder; by specifying the uncompressed length of a table as 0xfffffffd, they cause arithmetic overflow (to zero) when we round up to a multiple of 4, so then we miscalculate the total buffer size needed, and end up overwriting some "random" other memory when we decompress.

To protect against this, the patch simply checks that we can round up the original length without overflow.

Comment 5

[chris hofmann]

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]]

Switching the blocking request to 1.9.2, woff is not supported in 1.9.1 (Fx 3.5).

Comment 7

[chris hofmann]

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

Comment 8

[Jonathan Kew [:jfkthame]]

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]]

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]]

CC'ing the reporter

Comment 11

[Evgeny L.]

(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]]

(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) (email my personal email if necessary)]

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]]

Attachment: use 64-bit arithmetic to avoid risk of overflow

Comment 15

[Jonathan Kew [:jfkthame]]

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]]

Attachment: patch for 1.9.2 branch, includes followup windows bustage fix

Comment 17

[Jonathan Kew [:jfkthame]]

Argh, additional bustage fix:
http://hg.mozilla.org/mozilla-central/rev/ba99b3a279cf

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: patch for 1.9.2 branch, includes followups

Comment 19

[Mats Palmgren (inactive)]

Attachment: 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]]

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]]

(marking blocking - also, is this now FIXED on trunk?)

Comment 22

[Jonathan Kew [:jfkthame]]

Pushed to 1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/827a6883442f

Comment 23

[Robert Kaiser]

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]]

1.9.1 is unaffected (see flags above).

Comment 25

[Reed Loden [:reed]]

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 - ex-MoCo]]

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]]

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 30

[Daniel C]

(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]

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]

Any remaining issues suggested in comment 7 and 8 are now being tracked in  Bug 555139 -  (CVE-2010-1122)

Comment 33

[Aakash Desai [:aakashd]]

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

Comment 34

[Daniel Veditz [:dveditz]]

Sorry the working exploit code was hidden so long. It's easy to find hidden bugs for FIXED security issues and publish those (we have a process), but much harder to find redacted attachments and separate the needles out of the haystack of redacted spam.

Comment 35

[Andrew McCreight [:mccr8]]

Some interesting historical context from a recent comment:
(In reply to Jonathan Kew [:jfkthame] from bug 1980462 comment #3)

(FWIW, the code that was involved in bug 552216 is no longer around; WOFF decoding is long-since replaced by the OTS library, which has had pretty extensive testing/fuzzing over the years.)