[webkit-reviews] review granted: [Bug 31106] Sanitize web fonts using the OTS library : [Attachment 42808] transcode_webfonts_by_ots_v10

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted David Levin <levin at chromium.org>'s
request for review:
Bug 31106: Sanitize web fonts using the OTS library
https://bugs.webkit.org/show_bug.cgi?id=31106

Attachment 42808: transcode_webfonts_by_ots_v10
https://bugs.webkit.org/attachment.cgi?id=42808&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
 54	// name table replacement and/or padding for glyf table.

"glyph" not "glyf"

 36 #include "opentype-sanitiser.h"

and

 9	   and attempts to validate and sanitise them. We hope this reduces the
attack surface


sanitizer instead of sanitiser.

I just read through the entire patch for the first time and it otherwise looks
OK, those typos can be corrected on landing, but this cannot be cq+'d as is.

As far as I can tell this change is non-harmful to WebKit and helps Chromium
feel more secure.  I am certain there are real, exploitable bugs in system
font-parsers on the various OSes WebKit supports.

Adding a sanitization step allows WebKit to trade any known system font parser
bugs for possible WebKit or OpenTypeSanitizer bugs which can be contained in a
sandbox instead of being exploit-your-machine bugs.  To the best of my
knowledge, WebKit has historically done the same for CG and Skia graphics
libraries.  I see this as a similar approach.

I've not spoke with any of the other Chromium folks in person about this, but
from a WebKit perspective, this seems like a good change to make.

I think other ports are going to want to adopt this type of sanitization. 
Whether that means we eventually need to move this "OTS" code into WebKit or
not, I'm not sure.  We don't include libxslt in WebKit, and I see this as
similar.

Obviously we should not commit this with objection remaining, but I'll mark
this with r+ representing my support of this patch.