Bug 31106 - Sanitize web fonts using the OTS library
Summary: Sanitize web fonts using the OTS library
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-04 00:56 PST by Yusuke Sato
Modified: 2009-12-03 15:10 PST (History)
14 users (show)

See Also:


Attachments
transcode_webfonts_by_ots_v1 (5.93 KB, patch)
2009-11-04 06:12 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v2 (9.76 KB, patch)
2009-11-04 20:58 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v3 (10.95 KB, patch)
2009-11-04 21:05 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v4 (10.95 KB, patch)
2009-11-05 17:23 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v5 (12.20 KB, patch)
2009-11-05 17:25 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v6 (12.27 KB, patch)
2009-11-07 01:39 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v7 (12.68 KB, patch)
2009-11-09 00:58 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v8 (11.85 KB, patch)
2009-11-09 13:06 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v9 (11.85 KB, patch)
2009-11-09 15:11 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v10 (16.45 KB, patch)
2009-11-09 15:13 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v11 (5.37 KB, patch)
2009-11-30 06:28 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v12 (16.51 KB, patch)
2009-11-30 06:30 PST, Yusuke Sato
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
transcode_webfonts_by_ots_v13 (16.51 KB, patch)
2009-12-02 18:28 PST, Yusuke Sato
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Sato 2009-11-04 00:56:15 PST
On Chromium, we have to transcode a web font before handing it over to rendering libraries such as FreeType2 and t2embed.dll, for security reasons.

Chromium bug: http://crbug.com/17818
Comment 1 Yusuke Sato 2009-11-04 06:12:20 PST
Created attachment 42480 [details]
transcode_webfonts_by_ots_v1
Comment 2 Yusuke Sato 2009-11-04 06:18:11 PST
[+cc: levin@chromium.org, agl@chromium.org, jshin@chromium.org]

David, Adam,
Could you please review this change?

This change depends on a patch on Chromium side which is under review (http://codereview.chromium.org/363001). So please do not cq+ for a while even if it's r+.
Comment 3 Sam Weinig 2009-11-04 07:27:04 PST
How is this chromium only?  Would this not be useful for other ports?
Comment 4 Yusuke Sato 2009-11-04 08:07:01 PST
Yes, it should be useful for other ports. However, since the transcoder is kind of experimental, I would like to test it with Chromium's dev channel first.
Comment 5 Evan Martin 2009-11-04 08:21:25 PST
(In reply to comment #3)
> How is this chromium only?  Would this not be useful for other ports?

I haven't been following this since the initial exploratory patch, but as I recall the transcoding involves stripping important font information like hinting tables.  So even post-experiment this code may be a regression on other ports that already offer fonts.
Comment 6 Adam Langley 2009-11-04 08:25:19 PST
There's no reason why hinting code cannot be sanitised and passed through, but it's not in V1. The current thinking is that it's a lot of code and @font-face is mostly used for large, heading test rather than for the main body.
Comment 7 Yusuke Sato 2009-11-04 09:17:50 PST
Actually the transcoder now supports font hinting in glyf and CFF tables, but it still does not support glyph substitution tables. As a result of the lack of the GSUB support, web browsers that use the transcoder can't handle web fonts for some complex scripts.

> this code may be a regression on other ports

So it is correct at present.
Comment 8 Eric Seidel (no email) 2009-11-04 10:14:10 PST
Comment on attachment 42480 [details]
transcode_webfonts_by_ots_v1

Hum... Maybe eventually we would want to wrap this in some sort of platform abstraction and have the OpenTypeSanitizer be a back-end only used by chromium at the moment.  All others would just have a pass-through filter for now?
Comment 9 Yusuke Sato 2009-11-04 20:58:14 PST
Created attachment 42542 [details]
transcode_webfonts_by_ots_v2
Comment 10 Yusuke Sato 2009-11-04 21:05:37 PST
Created attachment 42543 [details]
transcode_webfonts_by_ots_v3
Comment 11 Yusuke Sato 2009-11-04 21:09:14 PST
I've created a new patch that introduces platform/graphics/opentype/OpenTypeSanitiser.{cpp,h} class. Can you please take another look?

(and please ignore the v2 patch. I forgot to add ChangeLog to v2.)
Comment 12 Adam Langley 2009-11-05 11:25:13 PST
Comment on attachment 42543 [details]
transcode_webfonts_by_ots_v3

LGTM. (I am not a WebKit reviewer. You need a real review also.)



> +        handle web fonts in a secure manner

This ChangeLog entry should be more descriptive:

Add support for OpenType Sanitiser (OTS). This is experimental code that is Chromium only for the moment. It parses OpenType files (from @font-face) and attempts to validate and sanitise them. We hope this reduces the attack surface of the system font libraries.

> +    // This is the largest web font size which we'll try to transcode.
> +    static const size_t maxWebFontSize = 30 * 1024 * 1024;  // 30 MB

This is pretty huge, but looking around it does seem that some fonts are nearly this large!
Comment 13 Yusuke Sato 2009-11-05 17:23:10 PST
Created attachment 42610 [details]
transcode_webfonts_by_ots_v4
Comment 14 Yusuke Sato 2009-11-05 17:25:29 PST
Created attachment 42611 [details]
transcode_webfonts_by_ots_v5
Comment 15 Yusuke Sato 2009-11-05 17:31:02 PST
Uploaded v5 patch (please ignore v4. it's accidentially uploaded, sorry.).

- Revised WebCore/ChangeLog as agl suggested.
- Added a patch for WebKit/chromium/DEPS following Yaar's comment in http://codereview.chromium.org/363001 .

The code is not changed.
Comment 16 Adam Langley 2009-11-05 17:40:37 PST
Comment on attachment 42611 [details]
transcode_webfonts_by_ots_v5

LGTM.

(I am not a WebKit reviewer. You also need a real review.)
Comment 17 Dave Hyatt 2009-11-06 12:24:12 PST
Comment on attachment 42611 [details]
transcode_webfonts_by_ots_v5

Minor quibble, but you should use American English spelling rather than British English spelling, e.g., we don't call Colors Colours. :)

This means changing sanitise to sanitize, and OpenTypeSanitiser to OpenTypeSanitizer.

Can you explain more the motivation of this patch?  Have you run into specific attacks/exploits?  How do other browsers like Firefox fare?
Comment 18 Adam Barth 2009-11-06 13:23:05 PST
(In reply to comment #17)
> Can you explain more the motivation of this patch?  Have you run into specific
> attacks/exploits?  How do other browsers like Firefox fare?

These questions are difficult to answer publicly.  We can either talk off-bug (e.g., in email) or we can create a security-sensitive bug.
Comment 19 Sam Weinig 2009-11-06 14:47:50 PST
Comment on attachment 42611 [details]
transcode_webfonts_by_ots_v5

This is a feature, and there should be queried off an ENABLE(), not a PLATFORM().  There is nothing platform specific about this.
Comment 20 Yusuke Sato 2009-11-07 01:39:23 PST
Created attachment 42691 [details]
transcode_webfonts_by_ots_v6
Comment 21 Yusuke Sato 2009-11-07 01:44:01 PST
Thanks for the review! Uploaded v6 patch. Changes are as follows:

- Fixed class and method names: s to z.
- Removed #if PLATFORM()s. Use #if ENABLE(OPENTYPE_SANITIZER) instead.
Comment 22 Sam Weinig 2009-11-07 17:21:52 PST
(In reply to comment #18)
> (In reply to comment #17)
> > Can you explain more the motivation of this patch?  Have you run into specific
> > attacks/exploits?  How do other browsers like Firefox fare?
> 
> These questions are difficult to answer publicly.  We can either talk off-bug
> (e.g., in email) or we can create a security-sensitive bug.

Adam, please email webkit-security with the details if you don't feel comfortable discussing them here. Thanks.
Comment 23 John Daggett 2009-11-08 05:21:16 PST
One small nit, if you're basically reconstructing the font, you really should remove the DSIG table, as it will no longer be valid for the reconstructed font.
Comment 24 Yusuke Sato 2009-11-08 05:33:14 PST
(In reply to comment #23)
Yes, the sanitizer always drops DSIG table from a reconstructed font. It recalculates  checksums for each table as well. Thanks.
Comment 25 Yusuke Sato 2009-11-08 16:57:35 PST
(In reply to comment #22)
I believe there was a similar discussion about half year ago. Can you please check the bug 25245 which is marked as security-sensitive?
https://bugs.webkit.org/show_bug.cgi?id=25245
Comment 26 David Levin 2009-11-08 22:27:25 PST
Comment on attachment 42691 [details]
transcode_webfonts_by_ots_v6

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2009-11-07  Yusuke Sato  <yusukes@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        handle web fonts in a secure manner
> +        https://bugs.webkit.org/show_bug.cgi?id=31106
> +
> +        Add support for OpenType sanitiser (OTS). This is experimental code that is
> +        Chromium only for the moment.

It isn't for chromium only anymore.


> diff --git a/WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp b/WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp

>  #include "FontPlatformData.h"
>  #include "NotImplemented.h"
> +#if ENABLE(OPENTYPE_SANITIZER)
> +#include "OpenTypeSanitizer.h"
> +#endif

The "if enable" should be in the header and then the include should be be done with no "if enable".

>  #include "SharedBuffer.h"

>  
>  #if PLATFORM(WIN_OS)
> @@ -245,6 +248,14 @@ FontCustomPlatformData* createFontCustomPlatformData(SharedBuffer* buffer)
>  {
>      ASSERT_ARG(buffer, buffer);
>  
> +#if ENABLE(OPENTYPE_SANITIZER)
> +    OpenTypeSanitizer sanitizer(buffer);
> +    PassRefPtr<SharedBuffer> transcodeBuffer = sanitizer.sanitize();

Use RefPtr not PassRefPtr in functons.

> +    if (!transcodeBuffer)
> +        return 0;  // validation failed.

One space before end of line comments.

> +    buffer = transcodeBuffer.get();
> +#endif

> diff --git a/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp b/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp

> +#if ENABLE(OPENTYPE_SANITIZER)
> +    OpenTypeSanitizer sanitizer(buffer);
> +    PassRefPtr<SharedBuffer> transcodeBuffer = sanitizer.sanitize();

Use RefPtr not PassRefPtr in functons.

> +    if (!transcodeBuffer)
> +        return 0;  // validation failed.

One space before end of line comments.

> +    buffer = transcodeBuffer.get();
> +#endif

> diff --git a/WebCore/platform/graphics/opentype/OpenTypeSanitizer.cpp b/WebCore/platform/graphics/opentype/OpenTypeSanitizer.cpp
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright (c) 2009, Google Inc. All rights reserved.

Use a capital c
no comma after the year

"Copyright (C) 2009 Google Inc. All rights reserved."


Add "#if ENABLE(OPENTYPE_SANITIZER)" in the file right before "#include "config.h"". Put the endif if at end of the file "#endif // ENABLE(OPENTYPE_SANITIZER)"

> +#include "config.h"

> +namespace WebCore {
> +
> +PassRefPtr<SharedBuffer> OpenTypeSanitizer::sanitize()
> +{
> +    if (!m_buffer)
> +        return 0;
> +
> +#if PLATFORM(CHROMIUM)

I know you did this if PLATFORM at Eric's suggest, but I think it turned out in a form that no one would use.

So I would just get rid of *all* "if PLATFORM(CHROMIUM)" in this file. If others want to use it in the future, it can be adjusted then to meet their needs.


> +    // This is the largest web font size which we'll try to transcode.
> +    static const size_t maxWebFontSize = 30 * 1024 * 1024;  // 30 MB

One space before end of line comments.

> +    if (m_buffer->size() > maxWebFontSize)
> +        return 0;
> +
> +    // A transcoded font is usually smaller than an original font.
> +    // However, it can be slightly bigger than the original one due to
> +    // name table replacement and/or padding for glyf table.

I've typically seen glyph instead of glyf but I did see glyf in one place on the web.

> +    static const size_t padLen = 20 * 1024;  // 20kB

One space before end of line comments.

> +
> +    unsigned char* transcodeRawBuffer = new unsigned char[m_buffer->size() + padLen];

Use OwnArrayPtr.

> +    ots::MemoryStream output(transcodeRawBuffer, m_buffer->size() + padLen);
> +    if (!ots::Process(&output, (const uint8_t *) m_buffer->data(), m_buffer->size())) {

Use c++ style cast. (reinterpret_cast). instead of "(const uint8_t *)".

> +        delete[] transcodeRawBuffer;

This goes away when you use OwnArrayPtr (and then the "if" will have one line so the {} will go away on the if clause.)

> +        return 0;
> +    }

> +    const size_t transcodeLen = output.Tell();
> +    return SharedBuffer::create(transcodeRawBuffer, transcodeLen);

I think this leaks but once you switch to OwnArrayPtr, it won't.

> diff --git a/WebCore/platform/graphics/opentype/OpenTypeSanitizer.h b/WebCore/platform/graphics/opentype/OpenTypeSanitizer.h
> + * Copyright (c) 2009, Google Inc. All rights reserved.

Use a capital c
no comma after the year

> +#ifndef OpenTypeSanitizer_h
> +#define OpenTypeSanitizer_h

Add the if enable here.
Comment 27 Yusuke Sato 2009-11-09 00:58:59 PST
Created attachment 42738 [details]
transcode_webfonts_by_ots_v7
Comment 28 Yusuke Sato 2009-11-09 01:08:44 PST
Thanks for the review. Uploaded v7 patch which addresses all the comments except this one:

> > +    // name table replacement and/or padding for glyf table.
> 
> I've typically seen glyph instead of glyf but I did see glyf in one place on
> the web.

OpenType fonts can have two glyph tables, "CFF" and "glyf", and the comment refers to the latter.
Please let me leave the comment as is to make it less ambiguous.

Thanks,
Yusuke
Comment 29 David Levin 2009-11-09 08:18:29 PST
Comment on attachment 42738 [details]
transcode_webfonts_by_ots_v7

> diff --git a/WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp b/WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp

Why did the the include move to the header file?

Just put it here as you like in your previous patch.


> diff --git a/WebCore/platform/graphics/chromium/FontCustomPlatformData.h b/WebCore/platform/graphics/chromium/FontCustomPlatformData.h

>  #include "FontRenderingMode.h"
>  #include <wtf/Noncopyable.h>
>  
> +#if ENABLE(OPENTYPE_SANITIZER)

Now that you have the ENABLE guards in the header file, you don't need them around the include.

> +#include "OpenTypeSanitizer.h"

I don't understand why this include is here (instead of being in the cpp file as in the previous patch).

> diff --git a/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp b/WebCore/platform/graphics/mac/FontCustomPlatformData.cpp
> diff --git a/WebCore/platform/graphics/mac/FontCustomPlatformData.h b/WebCore/platform/graphics/mac/FontCustomPlatformData.h

Same comments about the #include file for these two files as well.

> diff --git a/WebCore/platform/graphics/opentype/OpenTypeSanitizer.cpp b/WebCore/platform/graphics/opentype/OpenTypeSanitizer.cpp

> +
> +#include "OwnArrayPtr.h"

Include wtf files like this:

#include <wtf/OwnArrayPtr.h>

and put it after the other includes.

> +#include "SharedBuffer.h"
> +#include "opentype-sanitiser.h"
> +#include "ots-memory-stream.h"

> +    if (!ots::Process(&output, reinterpret_cast<const uint8_t *>(m_buffer->data()), m_buffer->size()))

No space after the unit8_t. "reinterpret_cast<const uint8_t*>("
Comment 30 Yusuke Sato 2009-11-09 13:06:21 PST
Created attachment 42788 [details]
transcode_webfonts_by_ots_v8
Comment 31 Yusuke Sato 2009-11-09 13:10:26 PST
Uploaded v8 patch.

(In reply to comment #29)
> Why did the the include move to the header file?
As I told you offline, I misunderstood your review comment, sorry.
Comment 32 Yusuke Sato 2009-11-09 15:11:43 PST
Created attachment 42807 [details]
transcode_webfonts_by_ots_v9
Comment 33 Yusuke Sato 2009-11-09 15:13:14 PST
Created attachment 42808 [details]
transcode_webfonts_by_ots_v10
Comment 34 Yusuke Sato 2009-11-09 15:15:51 PST
WebCore.xcodeproj was missing from the previous patch. Added the file.
Other files are not changed.

David, sorry for bothering you.

(In reply to comment #33)
> Created an attachment (id=42808) [details]
> transcode_webfonts_by_ots_v10
Comment 35 David Levin 2009-11-09 15:16:30 PST
Comment on attachment 42808 [details]
transcode_webfonts_by_ots_v10

Looks good.

It would be good to check other build files (for example other files in WebCore that list HTMLDataListElement.h ) and see if you need to change them.
Comment 36 Sam Weinig 2009-11-09 15:36:44 PST
I would rather this not get committed until we have more time to discus this.  If it is in fact a good idea to have this sanitizer, then I believe a copy should live in the webkit tree (just as image decoders live in the tree).

That said, I am not sure it is good idea. What makes one parser (the sanitizer) less prone to security bugs then the actual font parser?  Won't this increase the attack surface for a certain class of bug?

Let's not push this through until there has been more discussion on this.
Comment 37 Adam Langley 2009-11-09 16:04:13 PST
> What makes one parser (the sanitizer) less prone to security bugs then the actual font parser?

The reasons behind using a sanitiser:
  * Should bugs in popular parsers be found or known, we can render them irrelevant with the sanitiser. Since we don't control the system font parser, the time to patch may be unbounded and it's almost certainly a lot longer than the patch time of Chrome. For Safari on OS X this isn't an issue, since Apple controls both. But for any WebKit browser on Windows, this is a concern.
  * The system font parser may not be open source. In this case, security folks can review the sanitiser, but not the system font parser. The sanitiser is also a lot smaller than a full parser/renderer, thus it's easier to review and to see where attacker-controlled values are going.

So, you're correct that adding the sanitiser introduces the possibility of exploiting a bug in the sanitiser itself. However, given the two points above, I believe it's a worthwhile tradeoff.

> If it is in fact a good idea to have this sanitizer, then I believe a copy
> should live in the webkit tree (just as image decoders live in the tree).

At this point, I believe that's premature. But, if people feel strongly, I will concede as I don't feel that strongly.
Comment 38 David Levin 2009-11-09 16:46:36 PST
Comment on attachment 42808 [details]
transcode_webfonts_by_ots_v10

The patch still looks fine but I'll switch this back to r? pending Sam's request being answered so that it isn't accidentally committed.
Comment 39 John Daggett 2009-11-09 17:27:00 PST
I'm curious why OpenType layout (e.g. GPOS/GSUB) and AAT (e.g. morx) tables are omitted from the sanitizer.  My experience fixing font bugs in Firefox makes me think that these are actually more susceptible to attack then many of the base level TrueType tables, since the complexity of these tables easily hides underlying bugs.

One other thing to note here is that Webkit code currently uses the t2embed library for loading ttf fonts.  There have been known problems with this library in the past:

KB 961371 Vulnerabilities in the Embedded OpenType Font Engine could allow remote code execution
http://support.microsoft.com/kb/961371

If you're implementing a sanitizer seems like you really should be skipping calls to t2embed and instead using the low-level font loading API's, as is done for CFF fonts currently.
Comment 40 Yusuke Sato 2009-11-09 18:10:19 PST
Yes, the OTS library currently does not support GPOS/GSUB/morx tables. However, "does not support" means that OTS does not parse these tables, does not put them on a reconstructed font. As a result, attackers are not able to abuse these tables.
http://code.google.com/p/ots/wiki/DesignDoc

Though we might add parsers for these tables in the future if needed, it's unlikely for the first release.
Comment 41 John Daggett 2009-11-09 18:35:37 PST
(In reply to comment #40)
> Yes, the OTS library currently does not support GPOS/GSUB/morx tables. However,
> "does not support" means that OTS does not parse these tables, does not put
> them on a reconstructed font. As a result, attackers are not able to abuse
> these tables.

This means that fonts for any language that requires shaping (Arabic, Hindi, etc.) will effectively be neutered by the sanitize process.  Also looks like this effectively disables kerning, a recently added feature:

https://bugs.webkit.org/show_bug.cgi?id=6136
Comment 42 Adam Langley 2009-11-09 18:53:37 PST
Yes, the v1 sanitiser will kill complex text and kerning. Probably other things too.
Comment 43 Ian Fette 2009-11-10 23:53:15 PST
@Sam - Any update? http://www.microsoft.com/technet/security/Bulletin/MS09-065.mspx was made public today, so hopefully some of the reasoning behind this work is clearer.
Comment 44 Yusuke Sato 2009-11-13 19:24:36 PST
> This change depends on a patch on Chromium side which is under review
> (http://codereview.chromium.org/363001). So please do not cq+ for a while even
> if it's r+.

The Chromium patch has been landed. Now the WebKit change is cq+ safe.
Comment 45 Jungshik Shin 2009-11-18 10:23:16 PST
(In reply to comment #42)
> Yes, the v1 sanitiser will kill complex text and kerning. Probably other things
> too.

Actually, kerning is not an issue, yet because none of webkit ports supports kerning (and ligatures) for "simple" scripts (e.g. Latin), yet while Firefox 3.x does. 

Anyway, we're tracking them at http://crbug.com/27131 and http://crbug.com/27132
Comment 46 Jungshik Shin 2009-11-18 10:30:33 PST
(In reply to comment #45)
> (In reply to comment #42)
> > Yes, the v1 sanitiser will kill complex text and kerning. Probably other things
> > too.
> 
> Actually, kerning is not an issue, yet because none of webkit ports supports
> kerning (and ligatures) for "simple" scripts (e.g. Latin), yet while Firefox
> 3.x does. 

Clarification: When I wrote 'not an issue, yet', I meant 'not an issue, yet for Chromium or other webkit ports'. 

Because Firefox already supports kerning, for OTS to be used by Firefox soon, kern and other related tables have to be sanitized and passed through. Contributions are welcome :-)  ( http://code.google.com/p/ots/ )

For complex scripts, gpos,gsub and other tables must be sanitized even for webkit.
Comment 47 Eric Seidel (no email) 2009-11-29 13:37:24 PST
Comment on attachment 42808 [details]
transcode_webfonts_by_ots_v10

 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.
Comment 48 Adam Langley 2009-11-29 15:41:16 PST
> "glyph" not "glyf"

glyf isn't a typo: it's the name of the table:
http://www.microsoft.com/typography/otspec/glyf.htm
Comment 49 Yusuke Sato 2009-11-30 06:28:45 PST
Created attachment 44018 [details]
transcode_webfonts_by_ots_v11
Comment 50 Yusuke Sato 2009-11-30 06:30:45 PST
Created attachment 44019 [details]
transcode_webfonts_by_ots_v12
Comment 51 Yusuke Sato 2009-11-30 06:32:39 PST
(In reply to comment #50)
> Created an attachment (id=44019) [details]
> transcode_webfonts_by_ots_v12

Attached a new patch (v12) which can be applied to the latest tree.
Replaced sanitiser with sanitizer as well. Thanks, Eric.
Comment 52 Eric Seidel (no email) 2009-11-30 09:55:30 PST
Comment on attachment 44019 [details]
transcode_webfonts_by_ots_v12

Now that I understand that "glyf" is a special name, I guess I would have provided a url in the comment like AGL did in the comment above, but that's really not a big issue.

I still think this is a good change to make.  Lets leave this for at least a day for Sam to make any final comments on before committing.
Comment 53 Eric Seidel (no email) 2009-12-02 12:01:40 PST
Comment on attachment 44019 [details]
transcode_webfonts_by_ots_v12

Not having heard from Sam, I'm going to commit this.  We an always roll it out if there are further objections.
Comment 54 WebKit Commit Bot 2009-12-02 14:51:37 PST
Comment on attachment 44019 [details]
transcode_webfonts_by_ots_v12

Rejecting patch 44019 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel', '--force']" exit_code: 1
Last 500 characters of output:
cts to file WebCore/WebCore.xcodeproj/project.pbxproj.rej
patching file WebCore/platform/graphics/chromium/FontCustomPlatformData.cpp
patching file WebCore/platform/graphics/mac/FontCustomPlatformData.cpp
patching file WebCore/platform/graphics/opentype/OpenTypeSanitizer.cpp
patching file WebCore/platform/graphics/opentype/OpenTypeSanitizer.h
patching file WebKit/chromium/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/chromium/DEPS
patching file WebKit/chromium/features.gypi
Comment 55 Yusuke Sato 2009-12-02 18:28:44 PST
Created attachment 44200 [details]
transcode_webfonts_by_ots_v13
Comment 56 Yusuke Sato 2009-12-02 18:30:57 PST
(In reply to comment #54)
> (From update of attachment 44019 [details])
> Rejecting patch 44019 from commit-queue.
> 
> Failed to run
> "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply',
> '--reviewer', 'Eric Seidel', '--force']" exit_code: 1
> Last 500 characters of output:
> cts to file WebCore/WebCore.xcodeproj/project.pbxproj.rej

Hmm...
Uploaded v13 which should resolve the project.pbxproj conflict.
Comment 57 WebKit Commit Bot 2009-12-02 20:11:29 PST
Comment on attachment 44200 [details]
transcode_webfonts_by_ots_v13

Rejecting patch 44200 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11703 test cases.
inspector/elements-img-tooltip.html -> failed

Exiting early after 1 failures. 9385 tests run.
275.25s total testing time

9384 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
6 test cases (<1%) had stderr output
Comment 58 Yusuke Sato 2009-12-02 21:46:03 PST
(In reply to comment #57)
> (From update of attachment 44200 [details])
> Rejecting patch 44200 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari',
> '--quiet', '--exit-after-n-failures=1']" exit_code: 1
> Running build-dumprendertree
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Testing 11703 test cases.
> inspector/elements-img-tooltip.html -> failed

I've run-webkit-tests locally again, and confirmed that all test cases succeed:

yusukes-macpro:WebKit yusukes$ ./WebKitTools/Scripts/run-webkit-tests 
...
all 11703 test cases succeeded

Changing cq- to cq?. Thanks.
Comment 59 Adam Barth 2009-12-02 22:02:51 PST
Comment on attachment 44200 [details]
transcode_webfonts_by_ots_v13

Ok.  Let's try again.
Comment 60 WebKit Commit Bot 2009-12-02 22:14:31 PST
Comment on attachment 44200 [details]
transcode_webfonts_by_ots_v13

Clearing flags on attachment: 44200

Committed r51623: <http://trac.webkit.org/changeset/51623>
Comment 61 WebKit Commit Bot 2009-12-02 22:14:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 62 Eric Seidel (no email) 2009-12-03 15:10:01 PST
I believe the bogus inspector failure was just bug 30098.