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