RESOLVED FIXED 25239
Add a settings entry to en/disable web font support
https://bugs.webkit.org/show_bug.cgi?id=25239
Summary Add a settings entry to en/disable web font support
Jungshik Shin
Reported 2009-04-16 11:39:57 PDT
There's a concern about the security implication of remote web font support on Windows. Chromium wants to disable it by default by adding a command line switch. For that, I'm proposing to add an entry to Settings (remote_font_enabled) that will be true by default. Each 'embedder' of Webkit can do whichever default it wants to set.
Attachments
patch (3.60 KB, patch)
2009-04-16 12:01 PDT, Jungshik Shin
no flags
patch 2 with the check moved upstream (3.15 KB, patch)
2009-04-16 13:18 PDT, Jungshik Shin
no flags
patch updated addressing mitz' comments (3.08 KB, patch)
2009-04-29 14:10 PDT, Jungshik Shin
no flags
my patch for this (8.70 KB, patch)
2009-05-15 13:02 PDT, Antti Koivisto
ddkilzer: review+
Eric Seidel (no email)
Comment 1 2009-04-16 11:42:53 PDT
It seems it would make the most sense to fix the security concern. Or at least file a WebKit bug about it and list the bug URL here.
Eric Seidel (no email)
Comment 2 2009-04-16 11:43:38 PDT
Unless of course the security concern is chromium only, in which case it would make sense to reference the Chromium bug here.
Jungshik Shin
Comment 3 2009-04-16 12:01:30 PDT
Created attachment 29542 [details] patch This patch works, but I'm afraid the check for 'remote_font_enabled' is done too late (and perhaps more often than necessary). I'm looking for a better place to add the check.
Mark Rowe (bdash)
Comment 4 2009-04-16 12:21:57 PDT
The changelog mentions "remote_font_enabled" yet the code uses remoteFontEnabled.
Sam Weinig
Comment 5 2009-04-16 12:22:51 PDT
I agree with Eric. Without understanding the specific concerns, it is hard to evaluate the necessity for this.
Jungshik Shin
Comment 6 2009-04-16 13:18:47 PDT
Created attachment 29546 [details] patch 2 with the check moved upstream I moved the check for 'remote font support' 'upstream'. I think it's logically better that way On the other hand, the check may be done more often than before this way. I revised ChangeLog to refer to the actual member variable added to |Settings|. I'll answer to questions about the security shortly.
Jungshik Shin
Comment 7 2009-04-16 13:47:40 PDT
As far as I know, there is not any known risk, but Chromium team feels a bit nervous about relying on t2embed.dll as a black box. So, we decided to turn that off by default ( http://crbug.com/9633). Personally, I hate to see it go off :-) Windows ports of Webkit uses 3 functions in t2embed.dll : TTLoadEmbeddedFont, TTGetNewFontName, TTDeleteEmbeddedFont. t2embed.dll did have a security issue in the past (buffer overrun in the decompression of compressed EOT fonts), but that API is not used by Safari-Win and Chromium-Win because Webkit does not support EOT. http://www.microsoft.com/technet/security/Bulletin/MS06-002.mspx http://www.cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2006-0010 http://www.securityfocus.com/archive/1/archive/1/421885/100/0/threaded
Eric Seidel (no email)
Comment 8 2009-04-16 14:11:47 PDT
Further security sensitive discussion should probably be done in a Security bug. This bug is not marked for security, thus it's world readable.
Jungshik Shin
Comment 9 2009-04-16 15:01:51 PDT
Filed bug 25249. I thought what's in comment #7 is public already, but even if so, I guess it'd have been better to add that to bug 25249 than here.
Jungshik Shin
Comment 10 2009-04-16 15:05:02 PDT
(In reply to comment #9) > Filed bug 25249. I thought what's in comment #7 is public already, but even if > so, I guess it'd have been better to add that to bug 25249 than here. Oops. It's bug 25245.
Jungshik Shin
Comment 11 2009-04-17 13:14:34 PDT
Comment on attachment 29546 [details] patch 2 with the check moved upstream I'm not sure if you want to wait until there's a conclusion in bug 25245. If not, would you review the patch?
Jungshik Shin
Comment 12 2009-04-28 17:29:24 PDT
Dan, can you review the patch? Chromium wants to have this flag and set it off by default (can be enabled by a command line switch). Thanks
mitz
Comment 13 2009-04-28 21:50:20 PDT
Comment on attachment 29546 [details] patch 2 with the check moved upstream Please rename the instance variable, setter and getter to *remoteFontsEnabled* (in plural). I don't think "remote" is the best way to characterize these fonts (they may come from file: or data: URLs) but I could not think of a better name, and they are the opposite of local() fonts, I guess. > + There's no behavior change. So, no new test is necessary. You could have added WebKit SPI for changing the setting and a DumpRenderTree LayoutTestController function for controlling it, making it testable. It is fine if you don't want to do that, but I don't think you should make that comment in the change log. r=me with the renames.
Jungshik Shin
Comment 14 2009-04-29 14:10:31 PDT
Created attachment 29894 [details] patch updated addressing mitz' comments I addressed Dan's concerns and am transferring his r+ here. This patch is ready for check-in. BTW, this does not add a symbol to WebCore.base.exp because a new setRemoteFontsEabled is not called outside Webcore.
Eric Seidel (no email)
Comment 15 2009-04-29 17:05:15 PDT
Comment on attachment 29894 [details] patch updated addressing mitz' comments I would prefer this to actually go through a real second review. If I were reviewing this, I would probably ask for tests. :) I can't remember if DRT supports testing preferences yet or not. I know gwilson was working on a patch for that long ago.
Jungshik Shin
Comment 16 2009-04-30 09:55:44 PDT
(In reply to comment #15) > (From update of attachment 29894 [details] [review]) > I would prefer this to actually go through a real second review. Ok. Dan, can you take another look? > If I were reviewing this, I would probably ask for tests. :) I can't remember > if DRT supports testing preferences yet or not. I know gwilson was working on > a patch for that long ago. It's bug 20534 and is not yet resolved. However, Dan seems to imply that a test can be added without that bug fixed (by tweaking LayoutTestController). Anyway, I'd rather add a test separately if it's ok.
Eric Seidel (no email)
Comment 17 2009-05-05 22:48:22 PDT
Comment on attachment 29546 [details] patch 2 with the check moved upstream Clearing flag on old patch.
Eric Seidel (no email)
Comment 18 2009-05-12 21:52:30 PDT
Mitz? Can I assume this patch is still OK with you? -eric
David Kilzer (:ddkilzer)
Comment 19 2009-05-15 04:37:06 PDT
See also Bug 25815.
Jungshik Shin
Comment 20 2009-05-15 11:15:13 PDT
Should I revise the patch to do what's suggested in bug 25815? That can be done by changing the check to if (item->isSupportedFormat() && (foundSVGFont || (m_document && m_document->settings()->remoteFontEnabled()))) { from if (item->isSupportedFormat() && m_document && m_document->settings()->remoteFontEnabled()) {
Jungshik Shin
Comment 21 2009-05-15 11:20:54 PDT
(In reply to comment #20) > if (item->isSupportedFormat() && (foundSVGFont || (m_document && > m_document->settings()->remoteFontEnabled()))) { s/remoteFont/remoteFonts/
David Kilzer (:ddkilzer)
Comment 22 2009-05-15 11:52:47 PDT
(In reply to comment #20) > Should I revise the patch to do what's suggested in bug 25815? Yes, please do. Thanks!
Antti Koivisto
Comment 23 2009-05-15 12:57:35 PDT
Comment on attachment 29894 [details] patch updated addressing mitz' comments clearing the review flag
Antti Koivisto
Comment 24 2009-05-15 13:02:48 PDT
Created attachment 30397 [details] my patch for this I didn't realize there was a patch attempt for this already so I wrote one. This includes Mac WebKit bits too and differentiates between SVG fonts and binary fonts.
Antti Koivisto
Comment 25 2009-05-15 13:05:06 PDT
*** Bug 25815 has been marked as a duplicate of this bug. ***
mitz
Comment 26 2009-05-15 13:08:04 PDT
Comment on attachment 30397 [details] my patch for this I think m_downloadableBinaryFontsEnabled should default to true. Otherwise your patch will change the behavior for platforms whose WebKit layer you did not change.
Antti Koivisto
Comment 27 2009-05-15 13:11:00 PDT
(In reply to comment #26) > (From update of attachment 30397 [details] [review]) > I think m_downloadableBinaryFontsEnabled should default to true. Otherwise your > patch will change the behavior for platforms whose WebKit layer you did not > change. I don't agree. Settings disables everything else by default. Platforms should opt in.
David Kilzer (:ddkilzer)
Comment 28 2009-05-15 13:13:31 PDT
Comment on attachment 30397 [details] my patch for this >+ Settings* settings = m_document ? m_document->frame() ? m_document->frame()->settings() : 0 : 0; I think it's easier to read if you do: Settings* settings = m_document && m_document->frame() ? m_document->frame()->settings() : 0; r=me Dave
mitz
Comment 29 2009-05-15 13:21:24 PDT
(In reply to comment #27) > I don't agree. Settings disables everything else by default. Platforms should > opt in. Please don't land this without the changes to all existing platforms that are necessary to prevent regressions. Thanks!
Antti Koivisto
Comment 30 2009-05-15 14:56:06 PDT
Sending WebCore/ChangeLog Sending WebCore/WebCore.base.exp Sending WebCore/css/CSSFontSelector.cpp Sending WebCore/page/Settings.cpp Sending WebCore/page/Settings.h Transmitting file data ..... Committed revision 43783. I left the feature enabled by default to avoid changing the behavior, as suggested by Mitz.
Eric Seidel (no email)
Comment 31 2009-05-15 17:17:13 PDT
Thank you Antti & Mitz. Looks like this was intended to be closed since it's now landing. Closing.
Note You need to log in before you can comment on or make changes to this bug.