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.
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.
Unless of course the security concern is chromium only, in which case it would make sense to reference the Chromium bug here.
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.
The changelog mentions "remote_font_enabled" yet the code uses remoteFontEnabled.
I agree with Eric. Without understanding the specific concerns, it is hard to evaluate the necessity for this.
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.
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
Further security sensitive discussion should probably be done in a Security bug. This bug is not marked for security, thus it's world readable.
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.
(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.
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?
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
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.
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.
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.
(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.
Comment on attachment 29546 [details] patch 2 with the check moved upstream Clearing flag on old patch.
Mitz? Can I assume this patch is still OK with you? -eric
See also Bug 25815.
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()) {
(In reply to comment #20) > if (item->isSupportedFormat() && (foundSVGFont || (m_document && > m_document->settings()->remoteFontEnabled()))) { s/remoteFont/remoteFonts/
(In reply to comment #20) > Should I revise the patch to do what's suggested in bug 25815? Yes, please do. Thanks!
Comment on attachment 29894 [details] patch updated addressing mitz' comments clearing the review flag
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.
*** Bug 25815 has been marked as a duplicate of this bug. ***
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.
(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.
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
(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!
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.
Thank you Antti & Mitz. Looks like this was intended to be closed since it's now landing. Closing.