Bug 25239 – Add a settings entry to en/disable web font support

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jungshik Shin]

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.

[Eric Seidel (no email)]

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)]

Unless of course the security concern is chromium only, in which case it would make sense to reference the Chromium bug here.

[Jungshik Shin]

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)]

The changelog mentions "remote_font_enabled" yet the code uses remoteFontEnabled.

[Sam Weinig]

I agree with Eric.  Without understanding the specific concerns, it is hard to evaluate the necessity for this.

[Jungshik Shin]

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]

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)]

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]

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]

(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 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]

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.

[Jungshik Shin]

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 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]

(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 on attachment 29546 [details]
patch 2 with the check moved upstream

Clearing flag on old patch.

[Eric Seidel (no email)]

Mitz?  Can I assume this patch is still OK with you?

-eric

[David Kilzer (:ddkilzer)]

See also Bug 25815.

[Jungshik Shin]

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]

(In reply to comment #20)

> if (item->isSupportedFormat() && (foundSVGFont || (m_document &&
> m_document->settings()->remoteFontEnabled()))) {

s/remoteFont/remoteFonts/

[David Kilzer (:ddkilzer)]

(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 on attachment 29894 [details]
patch updated addressing mitz' comments

clearing the review flag

[Antti Koivisto]

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]

*** 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.

[Antti Koivisto]

(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 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!

[Antti Koivisto]

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)]

Thank you Antti & Mitz.  Looks like this was intended to be closed since it's now landing.  Closing.