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
Created attachment 42480 [details] transcode_webfonts_by_ots_v1
[+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+.
How is this chromium only? Would this not be useful for other ports?
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.
(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.
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.
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 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?
Created attachment 42542 [details] transcode_webfonts_by_ots_v2
Created attachment 42543 [details] transcode_webfonts_by_ots_v3
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 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!
Created attachment 42610 [details] transcode_webfonts_by_ots_v4
Created attachment 42611 [details] transcode_webfonts_by_ots_v5
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 on attachment 42611 [details] transcode_webfonts_by_ots_v5 LGTM. (I am not a WebKit reviewer. You also need a real review.)
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?
(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 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.
Created attachment 42691 [details] transcode_webfonts_by_ots_v6
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.
(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.
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.
(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.
(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 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.
Created attachment 42738 [details] transcode_webfonts_by_ots_v7
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 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*>("
Created attachment 42788 [details] transcode_webfonts_by_ots_v8
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.
Created attachment 42807 [details] transcode_webfonts_by_ots_v9
Created attachment 42808 [details] transcode_webfonts_by_ots_v10
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 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.
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.
> 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 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.
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.
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.
(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
Yes, the v1 sanitiser will kill complex text and kerning. Probably other things too.
@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.
> 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.
(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
(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 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.
> "glyph" not "glyf" glyf isn't a typo: it's the name of the table: http://www.microsoft.com/typography/otspec/glyf.htm
Created attachment 44018 [details] transcode_webfonts_by_ots_v11
Created attachment 44019 [details] transcode_webfonts_by_ots_v12
(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 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 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 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
Created attachment 44200 [details] transcode_webfonts_by_ots_v13
(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 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
(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 on attachment 44200 [details] transcode_webfonts_by_ots_v13 Ok. Let's try again.
Comment on attachment 44200 [details] transcode_webfonts_by_ots_v13 Clearing flags on attachment: 44200 Committed r51623: <http://trac.webkit.org/changeset/51623>
All reviewed patches have been landed. Closing bug.
I believe the bogus inspector failure was just bug 30098.