WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
patch 2 with the check moved upstream
(3.15 KB, patch)
2009-04-16 13:18 PDT
,
Jungshik Shin
no flags
Details
Formatted Diff
Diff
patch updated addressing mitz' comments
(3.08 KB, patch)
2009-04-29 14:10 PDT
,
Jungshik Shin
no flags
Details
Formatted Diff
Diff
my patch for this
(8.70 KB, patch)
2009-05-15 13:02 PDT
,
Antti Koivisto
ddkilzer
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug