Validate Graphite2-related font tables during OTS font decoding/sanitization

RESOLVED FIXED in Firefox 57

Status

()

Core
Graphics: Text
RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: jfkthame, Assigned: Kevin Hsieh)

Tracking

({sec-want})

unspecified
mozilla57
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57+ fixed)

Details

(Whiteboard: [adv-main57-])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

9 months ago
Currently, when we sanitize downloaded fonts, the graphite2 font tables 'Silf', 'Sill', 'Gloc', 'Glat', 'Feat' are passed through unchecked:

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/gfx/thebes/gfxUserFontSet.cpp#206-210

This means we depend entirely on the graphite library handling them safely, in the face of any possible corrupted or malicious data.

It would be good to add explicit sanitization support for these tables, similar to how OTS handles other font tables: parse their content, checking all structures and values against the graphite2 table specifications (and cross-checking against other font tables where appropriate, e.g. the maximum glyph ID), then re-serialize to generate a known-good table that can safely be passed to the graphite code.

I think we should implement this within the framework of OTS (see gfx/ots/src), following its conventions as closely as possible, so that it can ultimately be submitted to the upstream OTS project at https://github.com/khaledhosny/ots/.

Documentation for the graphite font tables can be found within the graphite font compiler project: see the "GraphiteCompiler_Docs.zip" archive at http://scripts.sil.org/cms/scripts/page.php?site_id=nrsi&id=GraphiteCompilerDownload
Assignee: nobody → kevin.hsieh
Status: NEW → ASSIGNED
Keywords: sec-want
(Reporter)

Comment 1

8 months ago
(In reply to Jonathan Kew (:jfkthame) from comment #0)
> Documentation for the graphite font tables can be found within the graphite
> font compiler project: see the "GraphiteCompiler_Docs.zip" archive at
> http://scripts.sil.org/cms/scripts/page.
> php?site_id=nrsi&id=GraphiteCompilerDownload

This is not the most up to date doc, actually. For the current doc, including new versions of some tables (e.g. Glat v3.0), see GTF.txt in the doc/ directory of the graphite project at Github: https://github.com/silnrsi/graphite/blob/master/doc/GTF.txt
(Reporter)

Comment 2

8 months ago
Created attachment 8881789 [details]
Graphite Table Format.pdf

For convenience, here's a formatted PDF of the latest GTF doc.
(Assignee)

Updated

7 months ago
(Assignee)

Updated

7 months ago
Depends on: 1348788
(Assignee)

Comment 3

7 months ago
Graphite support has now been added to OTS (https://github.com/khaledhosny/ots/pull/133). Bug 1348788 updates to the most recent version of OTS prior to the addition of Graphite support.
[Tracking Requested - why for this release]:

Any chance this will be ready to merge into Fx57? Or are there still a bunch of rough edges like https://github.com/khaledhosny/ots/pull/134 ?
tracking-firefox57: --- → ?
Flags: needinfo?(kevin.hsieh)
(Reporter)

Comment 5

7 months ago
I think we should be ready to land this in m-c fairly soon, so I'm hoping it will make the Fx57 release.
(Assignee)

Comment 6

7 months ago
I have a patch with all known issues fixed (including all rewiring needed for Firefox integration), and I'm planning to push it for review once https://github.com/khaledhosny/ots/pull/137 is merged, which should happen any minute. So it should be able to merge into 57.
Flags: needinfo?(kevin.hsieh)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

7 months ago
Publishing the review request since the upstream merge seems to be taking a while. Will update here if any changes are made upstream.

Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39e0cd63de986a9afa97ad87b079e9fa4d1d0fc7
Comment hidden (mozreview-request)
(Assignee)

Comment 10

7 months ago
The upstream merge is complete, so I updated gfx/ots/README.mozilla with the new commit ID.
Comment hidden (mozreview-request)
(Reporter)

Comment 12

6 months ago
mozreview-review
Comment on attachment 8895942 [details]
Bug 1369672 - Update OTS to support Graphite table sanitization.

https://reviewboard.mozilla.org/r/163178/#review174382

Looks good; just a couple of minor adjustments suggested below, then I think we'll be ready to land this.

::: gfx/ots/README.mozilla:12
(Diff revision 3)
>  Upstream files included: LICENSE, src/, include/, tests/*.cc
>  
>  Additional files: README.mozilla, src/moz.build
>  
>  Additional patch: ots-visibility.patch (bug 711079).
>  Additional patch: ots-config.patch (config.h not needed in mozilla build)

ots-config.patch is gone, and there's a new ots-lz4.patch, so please update the note here.

::: gfx/ots/src/moz.build:13
(Diff revision 3)
>      '../include/opentype-sanitiser.h',
>      '../include/ots-memory-stream.h',
>  ]
>  
>  SOURCES += [
>      # don't unify sources that use a (file-specific) DROP_THIS_TABLE macro

The DROP_THIS_TABLE macro is gone, so this comment is obsolete; but actually, it looks like you could move all the source files into UNIFIED_SOURCES, except that gdef.cc will need to stay separate because both it and gpos.cc want to define kMaxClassDefValue.

(Yes, we could have done this last time we took an OTS update, but didn't notice it then.)
Comment hidden (mozreview-request)

Comment 14

6 months ago
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)
(Reporter)

Comment 15

6 months ago
mozreview-review
Comment on attachment 8895942 [details]
Bug 1369672 - Update OTS to support Graphite table sanitization.

https://reviewboard.mozilla.org/r/163178/#review174842

LGTM, thanks! I've triggered a try run just to double-check that it's happy, then let's get this landed.
Attachment #8895942 - Flags: review?(jfkthame) → review+
(Assignee)

Updated

6 months ago
Keywords: checkin-needed

Comment 16

6 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15956a990499
Update OTS to support Graphite table sanitization. r=jfkthame
Keywords: checkin-needed
This caused build failures on osx builds and on linux static builds like https://treeherder.mozilla.org/logviewer.html#?job_id=123949913&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/045912496ae1e27c76b82233bce6940b1147f01b
Flags: needinfo?(kevin.hsieh)
(Reporter)

Comment 18

6 months ago
*sigh* Looks like we need a bunch of constructors to be marked 'explicit'. It's fine to fix that in the patch here and re-land; but please also submit a PR upstream, so that it doesn't come up again next time we import a new version.
(Be sure to trigger a try run with "static analysis" builds before we re-land this, too - those are the builds that run the checks that were tripped here.)
(Reporter)

Comment 20

6 months ago
Yes -- sorry about that; I did a partial try run (as well as having done a local build), thinking that should be sufficient (and save some resources), but in this case didn't get away with it.
Comment hidden (mozreview-request)
interdiff looks good to me -- looks like you fixed up all the classes mentioned in the build failure log linked in comment 17.

I'd say you're good to carry forward r+ and get this re-landed, once we've got a successful static analysis build on Try. (test runs probably don't matter much, given the earlier Try run and the successful-looking-autoland-treeherder-run (successful aside from these 'explicit' things)
(Assignee)

Comment 23

6 months ago
I'm having some issues triggering a try run online, so here's a direct link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15b5e5e2cf4b5b508c1a7e2a73bbf8c7a44a844d
Flags: needinfo?(kevin.hsieh)
Thanks. I see green Linux "S" builds there, so I think we're good to go. I'll trigger autoland.

Comment 25

6 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b228a65aa8b
Update OTS to support Graphite table sanitization. r=jfkthame

Comment 26

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b228a65aa8b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
when this landed originally we had compiler warnings increased:
== Change summary for alert #8845 (as of August 17 2017 18:54 UTC) ==

Regressions:

  2%  compiler warnings summary windows2012-32 pgo      127.00 -> 129.17
  1%  compiler warnings summary windows2012-64 pgo      145.00 -> 147.17

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8845

and improvements on the backout:
== Change summary for alert #8851 (as of August 17 2017 20:51 UTC) ==

Improvements:

  2%  compiler warnings summary windows2012-32 pgo      129.00 -> 126.92
  1%  compiler warnings summary windows2012-64 pgo      147.00 -> 145.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8851


finally on the next landing we see increased compiler warnings:
== Change summary for alert #8857 (as of August 17 2017 23:31 UTC) ==

Regressions:

  1%  compiler warnings summary windows2012-64 pgo      145.00 -> 147.00
  1%  compiler warnings summary windows2012-64 opt      145.15 -> 147.00
  1%  compiler warnings summary windows2012-64 debug    151.15 -> 153.00
  1%  compiler warnings summary windows2012-64-noopt debug 151.15 -> 153.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8857


we don't have rules in the tree for requirements to fix these, it would be good to know if these were expected.
(Assignee)

Comment 28

6 months ago
These compiler warnings are expected:

In file included from /Users/khsieh/Builds/mozilla-central-obj/gfx/ots/src/Unified_cpp_gfx_ots_src0.cpp:65:
/Users/khsieh/Builds/mozilla-central/gfx/ots/src/glat.cc:159:26: warning: comparison of integers of different signs: 'unsigned int' and 'int16_t' (aka 'short') [-Wsign-compare]
  for (unsigned i = 0; i < this->num; ++i) {
                       ~ ^ ~~~~~~~~~
/Users/khsieh/Builds/mozilla-central/gfx/ots/src/glat.cc:391:26: warning: comparison of integers of different signs: 'unsigned int' and 'int16_t' (aka 'short') [-Wsign-compare]
  for (unsigned i = 0; i < this->num; ++i) {
                       ~ ^ ~~~~~~~~~
2 warnings generated.

This occurs because there are two fields in Glat tables which should be unsigned, but are signed according to the specification (and this implementation follows the spec strictly). I'll file a separate bug for this and get it fixed upstream.
(Assignee)

Updated

6 months ago
See Also: → bug 1391855

Updated

6 months ago
tracking-firefox57: ? → +
Depends on: 1395053
Depends on: 1396026
Whiteboard: [adv-main57-]
You need to log in before you can comment on or make changes to this bug.