1255731 - disable graphite2 by default on release-channel builds

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

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.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

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

Comment 3

[Jonathan Kew [:jfkthame]]

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

Comment 4

[Jonathan Kew [:jfkthame]]

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

[Jet Villegas (inactive)]

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

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

LGTM.

Comment 6

[Jet Villegas (inactive)]

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

Comment 7

[Sylvestre Ledru [:Sylvestre]]

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

Comment 8

[Jonathan Kew [:jfkthame]]

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.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

We could also disable graphite2 for beta until we have a batch of fixes to uplift. 
Jonathan, what do you think?

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Yes,we should.

Comment 12

[Jonathan Kew [: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?

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?

Comment 13

[Al Billings [:abillings - ex-MoCo]]

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

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 15

[Jonathan Kew [:jfkthame]]

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.

Comment 16

[Jonathan Kew [:jfkthame]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Not much point in waiting for this to hit m-c before uplift since the patch is a no-op on trunk anyway.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this for 46+

Comment 20

[Jonathan Kew [:jfkthame]]

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

Comment 21

[Sylvestre Ledru [:Sylvestre]]

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

Needed for the 45.0.1 releases.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This is also needed for ESR38.7.1. Sylvestre, approved it in comment 21.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Jonathan, does it affect android too?

Comment 24

[Jonathan Kew [:jfkthame]]

Yes, we have the same code in fennec, so the same issues exist there.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79e337a16634

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/18d6afbda28c
https://hg.mozilla.org/releases/mozilla-esr38/rev/21b911ffbd8d

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79e337a16634

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "Disabled Graphite font shaping library" as wording