1453653 - Cherry-pick an upstream FreeType integer overflow fix

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Reported by OSS-Fuzz, appears to affect all supported versions.
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=bd9400bd464f6cd7c74f52ece1c1065fe2a87aab

Not sure what the rating on this bug should be.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: backport the upstream fix

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df480408aa05e9604f862bfd6cdf9c415f8a16d1

Comment 2

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=df480408aa05e9604f862bfd6cdf9c415f8a16d1

This time with Ctrl+A in that fuzzy run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d62f76221adae08e1cfa115e34e34d0b96f4ba72

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Attachment: [esr52] backport the upstream fix

Here's the rebased patch for ESR52. The trunk patch grafts cleanly to Beta as-is.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b064a00f9ea2977978e09f7fd5950de5761107b4

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8967397 [details] [diff] [review]
[esr52] backport the upstream fix

Though looking at the upstream commit history for this file, there's other integer overflow bugs that we may want to consider backporting? i.e.
http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/src/truetype/ttinterp.c?id=9038837ee23460ce98715fe0f68497e7dcb69174

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)
> Comment on attachment 8967397 [details] [diff] [review]
> [esr52] backport the upstream fix
> 
> Though looking at the upstream commit history for this file, there's other
> integer overflow bugs that we may want to consider backporting? i.e.
> http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/src/truetype/
> ttinterp.c?id=9038837ee23460ce98715fe0f68497e7dcb69174

Probably not worth it, given that we use our in-tree freetype only on Android AFAIK.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8967397 [details] [diff] [review]
[esr52] backport the upstream fix

Jonathan helpfully reminded me that we only use the in-tree FT2 on Android, so backporting to ESR52 seems unnecessary.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Setting the ESR52 status to disabled to reflect that the buggy code is there, but unused in the builds we ship off that branch.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8967354 [details] [diff] [review]
backport the upstream fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I have no idea, TBH. The issue was found by oss-fuzz and is referenced in the upstream commit (though it's still restricted access).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I left the commit message generic, but obviously someone is free to follow the link to the upstream commit to see that it's an integer overflow fix.

Which older supported branches are affected by this flaw?
All, though only 59+ in practice where we ship Android builds.

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Applies cleanly to Beta as-is.

How likely is this patch to cause regressions; how much testing does it need?
Seems highly unlikely. Android tests are green on the Try push.

Comment 9

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

Comment on attachment 8967354 [details] [diff] [review]
backport the upstream fix

Clearing sec-approval since we made this a sec-low. I'm good with taking it on Beta though (will let Relman decide).

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Thanks everyone!

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d736c05f9568c378edc7872127511a0a8f193b5

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3d736c05f956

Comment 12

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8967354 [details] [diff] [review]
backport the upstream fix

freetype sec fix, approved for 60.0b13

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4cd0924bad77

Comment 14

[Georg Koppen]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)
> Comment on attachment 8967397 [details] [diff] [review]
> [esr52] backport the upstream fix
> 
> Though looking at the upstream commit history for this file, there's other
> integer overflow bugs that we may want to consider backporting? i.e.
> http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/src/truetype/
> ttinterp.c?id=9038837ee23460ce98715fe0f68497e7dcb69174

What's the plan regarding those other integer overflow bugs? It seems to me Jonathan's comment in comment:5 was only saying that it's not worth to backport the patch in this bug to ESR52, but nothing with respect to how the other integer overflows you found should be dealt with.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

In Firefox 60+, we're already using FreeType 2.9 w/ relevant fixes already cherry-picked via bug 1438522. ESR52 was the only branch in question, so comment 5 covered the relevant concern :)