Closed Bug 1369672 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics: Text, enhancement)

Product:
Core
Component:
Graphics: Text
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 + fixed

People

(Reporter: jfkthame, Assigned: kevin.hsieh)

References

Details

(Keywords: sec-want, Whiteboard: [adv-main57-])

Attachments

(2 files)

Graphite Table Format.pdf
316.00 KB, application/pdf
Details
Bug 1369672 - Update OTS to support Graphite table sanitization.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details 
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
(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
For convenience, here's a formatted PDF of the latest GTF doc.
Depends on: 1348788
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)
I think we should be ready to land this in m-c fairly soon, so I'm hoping it will make the Fx57 release.
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)
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
The upstream merge is complete, so I updated gfx/ots/README.mozilla with the new commit ID.
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.)
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)
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+
Keywords: checkin-needed
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
*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.)
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.
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)
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.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b228a65aa8b
Update OTS to support Graphite table sanitization. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/5b228a65aa8b
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
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.
See Also: → 1391855
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.

Attachment

General

Creator:
Created:
Updated:
Size: