Bug 25239

Summary: Add a settings entry to en/disable web font support
Product: WebKit Reporter: Jungshik Shin <jshin>
Component: Layout and RenderingAssignee: Jungshik Shin <jshin>
Status: RESOLVED FIXED    
Severity: Minor CC: ddkilzer, emacemac7, ifette, koivisto, mal, mitz, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch
none
patch 2 with the check moved upstream
none
patch updated addressing mitz' comments
none
my patch for this ddkilzer: review+

Description Jungshik Shin 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.
Comment 1 Eric Seidel (no email) 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.
Comment 2 Eric Seidel (no email) 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.
Comment 3 Jungshik Shin 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.
Comment 4 Mark Rowe (bdash) 2009-04-16 12:21:57 PDT
The changelog mentions "remote_font_enabled" yet the code uses remoteFontEnabled.
Comment 5 Sam Weinig 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.
Comment 6 Jungshik Shin 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.
Comment 7 Jungshik Shin 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 
Comment 8 Eric Seidel (no email) 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.
Comment 9 Jungshik Shin 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. 
Comment 10 Jungshik Shin 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. 

Comment 11 Jungshik Shin 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?
Comment 12 Jungshik Shin 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
Comment 13 mitz 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.
Comment 14 Jungshik Shin 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.
Comment 15 Eric Seidel (no email) 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.
Comment 16 Jungshik Shin 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.
Comment 17 Eric Seidel (no email) 2009-05-05 22:48:22 PDT
Comment on attachment 29546 [details]
patch 2 with the check moved upstream

Clearing flag on old patch.
Comment 18 Eric Seidel (no email) 2009-05-12 21:52:30 PDT
Mitz?  Can I assume this patch is still OK with you?

-eric
Comment 19 David Kilzer (:ddkilzer) 2009-05-15 04:37:06 PDT
See also Bug 25815.
Comment 20 Jungshik Shin 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()) {

Comment 21 Jungshik Shin 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/



Comment 22 David Kilzer (:ddkilzer) 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!
Comment 23 Antti Koivisto 2009-05-15 12:57:35 PDT
Comment on attachment 29894 [details]
patch updated addressing mitz' comments

clearing the review flag
Comment 24 Antti Koivisto 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.
Comment 25 Antti Koivisto 2009-05-15 13:05:06 PDT
*** Bug 25815 has been marked as a duplicate of this bug. ***
Comment 26 mitz 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.
Comment 27 Antti Koivisto 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.

Comment 28 David Kilzer (:ddkilzer) 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
Comment 29 mitz 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!
Comment 30 Antti Koivisto 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.
Comment 31 Eric Seidel (no email) 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.