disable graphite2 by default on release-channel builds

RESOLVED FIXED in Firefox 45

Status

()

Core
Graphics: Text
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

({sec-other})

unspecified
mozilla48
sec-other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45blocking fixed, firefox46+ fixed, firefox47+ fixed, firefox48+ fixed, firefox-esr3845+ fixed, firefox-esr4545+ fixed, relnote-firefox 45+)

Details

(Whiteboard: [post-critsmash-triage])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Given the current churn/instability in the graphite2 code, with a stream of bugs found by fuzzing, it may be safest to turn it off by default for end users on release channels.

The (small proportion of) users who specifically need graphite font shaping will still be able to enable it manually in about:config, but the majority (for whom it is not relevant) will no longer be exposed to potential webfont-based threats.

Once the library is stabilized and the rate of incoming fuzz-bugs has dropped substantially, we'll restore the on-by-default state for all builds so that sites relying on graphite fonts will once again work correctly out-of-the-box without users needing to know "special magic".

Filing this as security-sensitive so that we can discuss as needed without drawing extra attention for now.


[Tracking Requested - why for this release]:
This is designed to mitigate the current concern about graphite2 vulnerabilities by default-disabling on release channels -- we want to do this ASAP for those users, not wait for it to ride the full train journey.
(Assignee)

Comment 1

2 years ago
Created attachment 8729403 [details] [diff] [review]
Disable graphite2 by default on release-channel builds

Here's the default pref change we discussed doing. If we take this, I expect there'll be a handful of reftests that will also need a fix (to explicitly turn the pref back on); I'll post a separate patch for that, after testing further.
Attachment #8729403 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Oh, if it is just a pref change, we can do a hotfix.

However, when was the last time we shipped a Firefox release without graphite2? (rephrasing the question: how well the fallback code tested and covered?)
(Assignee)

Comment 3

2 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #2)

> Oh, if it is just a pref change, we can do a hotfix.

Oh, is that something different (lighter-weight?) than patching the default in all.js? I'm not familiar with our deployment options here. Whatever works...

My suggestion -- if we're sufficiently worried about the potential exploitability of the graphite bugs -- would be to flip this pref for the release (and beta) channels at the same time as we take a new graphite release on trunk in bug 1255158 (and uplift it to aurora).

> However, when was the last time we shipped a Firefox release without
> graphite2? (rephrasing the question: how well the fallback code tested and
> covered?)

The fallback will be to simply send those fonts through the same harfbuzz codepath as everything else we render, as if they didn't include graphite tables. There's no different code involved.

The result will be somewhat inferior text (where the font doesn't have corresponding OpenType layout tables, or its OpenType implementation is inferior to the graphite one); certain fonts won't render as well as they currently do. But the result will generally be the same as other, non-graphite-enabled browsers.

So this would mean regressing behavior for sites/users that depend on graphite fonts, but they would still have the option of re-enabling it through about:config. My concern is that the 1.3.7 update, when we take it on trunk, is likely to look rather big and scary to uplift to beta/release/esr (see also bug 1248876, where this was a concern for 1.3.6); preffing off for the release channels lets us take ongoing updates on Nightly without zero-day'ing our release users or needing to chemspill every time.

And it seems plausible that whenever we do take a 1.3.7 release, even if we uplift it to all our channels at that time, there'll just be a new fuzzbug reported in the next day or two... I'd like to see that rate drop significantly so that we're not in the position of either having to update continually (for release users) or leave them exposed to issues that have been made public via upstream patches.
(Assignee)

Comment 4

2 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> However, when was the last time we shipped a Firefox release without
> graphite2?

And to answer this aspect, we enabled graphite in bug 700023, firefox 22. We've been shipping it as a standard part of our platform for the past 3 years. (For a year before that, it was present, but preffed off by default.)
Comment on attachment 8729403 [details] [diff] [review]
Disable graphite2 by default on release-channel builds

Review of attachment 8729403 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8729403 - Flags: review?(bugs) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> And it seems plausible that whenever we do take a 1.3.7 release, even if we
> uplift it to all our channels at that time, there'll just be a new fuzzbug
> reported in the next day or two... I'd like to see that rate drop
> significantly so that we're not in the position of either having to update
> continually (for release users) or leave them exposed to issues that have
> been made public via upstream patches.

It makes sense to let the Graphite 1.3.x fixes ride the trains and then we flip this pref again on Beta when we're comfortable with the quality and rate of changes.
OK, thanks. Do we have an idea on how many people or websites will see the font quality decrease? (I don't need specific numbers). A raw percentage would be enough for me to  understand the impact! thanks
tracking-firefox45: ? → blocking
(Assignee)

Comment 8

a year ago
We don't have any actual numbers (whether percentages or specifics), but we can be confident they're small. I would say "almost negligible", except that I don't think it's right to neglect a user community (such as a minority-language group in Myanmar, to pick a likely example) just because they're few in number.

Part of our mission is to make the web open and accessible to all -- and "all" should include communities with their own languages/writing systems, even if they're too small and/or poor to be commercially interesting to our competitors, and hence not supported by mainstream technologies (OpenType). That's why we include graphite, and why I'd be opposed to simply dropping it altogether; but we have to balance that against the risk of exposing the other 99.99+%* of our users to unnecessary security hazards.

We think temporarily preffing-off by default for release (while the code is cleaned up, hardened, and tested better upstream and on nightly) provides that balance, while not subjecting us to constant churn or the potential need for repeated chemspills on what should be stable channels.


* A rhetorical figure pulled from thin air, with no actual basis in data. We'd have to add some telemetry (and wait for a while to gather results) if we wanted actual numbers.
We could also disable graphite2 for beta until we have a batch of fixes to uplift. 
Jonathan, what do you think?
Flags: needinfo?(jfkthame)
Al, should we also consider uplifting this to esr38 branch and doing a dot release for esr38.7? Please let me know and I can get it going and push it live by tomm (if all goes well). Thanks!
Flags: needinfo?(abillings)

Updated

a year ago
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox-esr38: --- → affected
Yes,we should.
Flags: needinfo?(abillings)
(Assignee)

Comment 12

a year ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #9)
> We could also disable graphite2 for beta until we have a batch of fixes to
> uplift. 
> Jonathan, what do you think?

Yes - the proposed patch here was intended to disable on release+beta, leaving enabled on nightly+aurora.

I figure that we may not want to take the churn of multiple fixes on beta, especially when it gets later in the cycle, so having it enabled only for nightly+aurora until we're happy with the stability seemed like the appropriate plan. (And yes, we should treat esr38 the same as beta/release.)

Do you need me to land the pref-default patch here on the relevant branches (if so, it'll need to be marked with approval for the various repositories), or do you have other mechanisms to ship the pref change, and we can just take this on the trains in due course?
Flags: needinfo?(jfkthame)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #9)
> We could also disable graphite2 for beta until we have a batch of fixes to
> uplift. 
> Jonathan, what do you think?

Graphite2 folks are waiting for us to tell them to generate 1.3.7.
Depends on: 1255158
Jonathan, we will also need to uplift this patch to esr38 branch. Are we ready to do that now? What sort of testing do we plan to do on this one e.g. try push, more, less? Thanks!
Flags: needinfo?(jfkthame)
(Assignee)

Comment 15

a year ago
The only concern I have is that we might have a reftest or two that will go orange; if so, we'll want to fix them by adding pref("gfx.font_rendering.graphite.enabled",true) in the manifest. A try run would tell us if that's needed; I know that many of our graphite-related tests already have explicit pref assignments (left over from before it was enabled-by-default), so it may be that we're OK already.

I don't think any other kind of testing is needed. We know the alternative codepath we'll be hitting instead (harfbuzz) is already being heavily exercised.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 16

a year ago
(In reply to Jonathan Kew (:jfkthame) from comment #15)
> The only concern I have is that we might have a reftest or two that will go
> orange; if so, we'll want to fix them by adding
> pref("gfx.font_rendering.graphite.enabled",true) in the manifest. A try run
> would tell us if that's needed;

...or we can just keep an eye on the release-channel trees when we push the patch, and if any related test failures do come up, we push a followup to annotate them with the pref setting.
Not much point in waiting for this to hit m-c before uplift since the patch is a no-op on trunk anyway.
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox-esr38: --- → ?
tracking-firefox-esr45: --- → ?
Jonathan, we are still waiting for the patch to be nominated for uplift to moz-release and moz-esr38. If the patch is ready, can you please submit the uplift request? Sylvestre and I will both gtb 45.0.1 and 38.7.1 with this fix in it (among other ride-alongs). Thanks!
Flags: needinfo?(jfkthame)
Tracking this for 46+
tracking-firefox46: ? → +
tracking-firefox47: ? → +
tracking-firefox48: ? → +
(Assignee)

Comment 20

a year ago
Comment on attachment 8729403 [details] [diff] [review]
Disable graphite2 by default on release-channel builds

Approval Request Comment
[Feature/regressing bug #]: bugs in graphite2 text shaping library

[User impact if declined]: potential for webfont-based exploits leading to various crashes, memory corruption, or arbitrary code execution

[Describe test coverage new/current, TreeHerder]: not as such; we already have reftests that we run with graphite both enabled and disabled, and we're just flipping the default

[Risks and why]: minimal; there will be a known regression in text rendering for a tiny minority of sites/users (but those affected have the option of explicitly re-enabling the feature for themselves)

[String/UUID change made/needed]: n/a
Flags: needinfo?(jfkthame)
Attachment #8729403 - Flags: approval-mozilla-release?
Attachment #8729403 - Flags: approval-mozilla-esr38?

Updated

a year ago
Group: core-security → core-security-release
Comment on attachment 8729403 [details] [diff] [review]
Disable graphite2 by default on release-channel builds

Needed for the 45.0.1 releases.
Attachment #8729403 - Flags: approval-mozilla-release?
Attachment #8729403 - Flags: approval-mozilla-release+
Attachment #8729403 - Flags: approval-mozilla-esr45+
Attachment #8729403 - Flags: approval-mozilla-esr38?
Attachment #8729403 - Flags: approval-mozilla-esr38+
This is also needed for ESR38.7.1. Sylvestre, approved it in comment 21.
Keywords: sec-other
Jonathan, does it affect android too?
Flags: needinfo?(jfkthame)
(Assignee)

Comment 24

a year ago
Yes, we have the same code in fennec, so the same issues exist there.
Flags: needinfo?(jfkthame)
https://hg.mozilla.org/integration/mozilla-inbound/rev/79e337a16634
https://hg.mozilla.org/releases/mozilla-aurora/rev/8c6365c22bb8
https://hg.mozilla.org/releases/mozilla-beta/rev/f487458614db
https://hg.mozilla.org/releases/mozilla-release/rev/18d6afbda28c
status-firefox45: affected → fixed
status-firefox46: affected → fixed
status-firefox47: affected → fixed
https://hg.mozilla.org/releases/mozilla-esr45/rev/18d6afbda28c
https://hg.mozilla.org/releases/mozilla-esr38/rev/21b911ffbd8d
status-firefox-esr38: affected → fixed
status-firefox-esr45: affected → fixed

Updated

a year ago
tracking-firefox-esr38: ? → 45+
tracking-firefox-esr45: ? → 45+
https://hg.mozilla.org/mozilla-central/rev/79e337a16634
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Added to the release notes with "Disabled Graphite font shaping library" as wording
relnote-firefox: --- → 45+

Updated

a year ago
Blocks: 1257639
Whiteboard: [post-critsmash-triage]

Updated

11 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.