552308 - Possible integer overflows in WOFF

Status: NEW

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

Description

[Daniel Veditz [:dveditz]]

Fixing bug 552216 will likely focus attention on WOFF, we need to check for other similar bugs there. The immediate cause of that bug was that we read "orig" from the file (potentially evil data) and then did
       orig = (orig + 3) & ~3;
Which might overflow if orig was within 3 of rolling over.

Elsewhere in the file we have
   #define LONGALIGN(x) (((x) + 3) & ~3)

There are eight uses that might have the same problem
http://mxr.mozilla.org/mozilla-central/search?string=LONGALIGN

The use in calcChecksum looks like it would just lead to an immediate error or a bogus checksum. Not sure about the other uses (do we even call woffEncode()?).

We should check various mallocs, too.
 tableOrder = (tableOrderRec *) malloc(numOrigTables * sizeof(tableOrderRec));
on line 215 looks OK since numOrigTables is uint16, but maybe we should assert sizeof(numOrigTables) == 2 just to protect against future changes. Ditto line 238 (and 245) where numTables is used. Ah, looks like all this encoding stuff is #ifndef WOFF_MOZILLA_CLIENT

I'd be happy if Sid and Brandon could go over the rest of the file before we ship 3.6.2 though.

Comment 1

[Jonathan Kew [:jfkthame]]

Yes, it'd be good to check these; I'll try to go over it tomorrow, but other eyes would of course be very welcome.

We do not call woffEncode at any time; we build with the WOFF_MOZILLA_CLIENT #define, which excludes this and a number of other functions from compilation altogether.  (It'd still be nice to find and fix any errors in them, but they are not part of the Firefox build.)

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Any progress here? I agree with Dan that this area of code is likely to get deeper attention from BlackHats in the near future.

Comment 3

[Jonathan Kew [:jfkthame]]

I took another pass through the code and found a couple of cases in the encoding routines (NOT part of the Firefox build) where there could be a possibility of overflow leading to invalid fonts being created. I'm intending to put up a patch for this, to improve its quality as a sample implementation, though it's less urgent as we don't use that code.

It'd be good if someone else could check the decoding functions (which we actually use), as I'm probably too close to this code to be a good reviewer of it.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #3)
> It'd be good if someone else could check the decoding functions (which we
> actually use), as I'm probably too close to this code to be a good reviewer of
> it.

Should this be assigned to somebody else, then?  (Sid or Brandon, per comment 0, perhaps?)

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: add a number of overflow checks in woff encoding functions

I've looked at the uses of LONGALIGN and other calculations of buffer sizes, offsets, etc in the woff.c code, and added a bunch of overflow checks. Typically this means adding statements like "if (X + Y < X) { FAIL; }" before actually adding Y to X so that the code just bails out if the calculation is going to overflow. No valid font should come anywhere close to the limits of 32-bit arithmetic, so I don't see any reason to do more than return an INVALID FONT error code if this happens.

All the places I've added these checks are in the woff encoding routines, which are not part of the Mozilla build (#ifndef WOFF_MOZILLA_CLIENT) and so they do not indicate potential vulnerabilities in Firefox. Nevertheless, I think we should apply the patch to our tree as the woff code here also serves as a reference implementation for both encoding and decoding functions.

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to comment #4)
> (In reply to comment #3)
> > It'd be good if someone else could check the decoding functions (which we
> > actually use), as I'm probably too close to this code to be a good reviewer of
> > it.
> 
> Should this be assigned to somebody else, then?  (Sid or Brandon, per comment
> 0, perhaps?)

I'd be happy to see someone take that on.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 435560 [details] [diff] [review]
add a number of overflow checks in woff encoding functions

r=dveditz

Comment 8

[Jonathan Kew [:jfkthame]]

Pushed additional checks in encoding routines (NPOTB):
http://hg.mozilla.org/mozilla-central/rev/69eb050f2c0a

Leaving this bug open as it would still be good to have an additional audit of the decoding routines that we actually use in the Firefox build.

Comment 9

[Ian Melven :imelven]

https://bugzilla.mozilla.org/show_bug.cgi?id=527276 brought in the ots Sanitiser for OpenType project. i tested with WOFF fonts based on OpenType and TrueType fonts and both of them went through ots' ProcessWOFF function

to me it seems like this bug now becomes equivalent to an audit of the ots code, which is probably a good idea, but maybe a little less pressing than auditing our own woff code ? (we could also try fuzzing the ots code directly as well with a little bit of a wrapper around it, perhaps ?)

Comment 10

[Ian Melven :imelven]

additionally for our fuzzing pleasure (and as a good sign someone else has already done this fuzzing, ots ships with a script called test_malicious_fonts.sh which calls a wrapper (code for wrapper is at http://code.google.com/p/ots/source/browse/trunk/test/validator-checker.cc)

Comment 11

[Josh Aas]

Since this is now just about an audit does this need to remain hidden?

Comment 12

[Ian Melven :imelven]

That makes sense to me, opening it up. Thanks Josh !

I reset assignee to nobody as well.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information