Closed Bug 1453653 Opened 4 years ago Closed 4 years ago

Cherry-pick an upstream FreeType integer overflow fix

Categories

(Core :: Graphics: Text, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 - disabled
firefox59 --- wontfix
firefox60 + fixed
firefox61 + fixed

People

(Reporter: RyanVM, Assigned: RyanVM)

Details

(Keywords: csectype-intoverflow, csectype-undefined, sec-low, Whiteboard: [adv-main60+][post-critsmash-triage])

Attachments

(1 file, 1 obsolete file)

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.
(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
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 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
Flags: needinfo?(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.
Flags: needinfo?(jfkthame)
Attachment #8967354 - Flags: review?(jfkthame) → review+
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.
Attachment #8967397 - Attachment is obsolete: true
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 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.
Attachment #8967354 - Flags: sec-approval?
Attachment #8967354 - Flags: approval-mozilla-beta?
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).
Attachment #8967354 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/3d736c05f956
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8967354 [details] [diff] [review]
backport the upstream fix

freetype sec fix, approved for 60.0b13
Attachment #8967354 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: needinfo?(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 :)
Flags: needinfo?(ryanvm)
Group: gfx-core-security → core-security-release
Whiteboard: [adv-main60+]
Flags: qe-verify-
Whiteboard: [adv-main60+] → [adv-main60+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.