Cherry-pick an upstream FreeType integer overflow fix

RESOLVED FIXED in Firefox 60

Status

()

defect
--
critical
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: RyanVM, Assigned: RyanVM)

Tracking

({csectype-intoverflow, csectype-undefined, sec-low})

unspecified
mozilla61
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52- disabled, firefox59 wontfix, firefox60+ fixed, firefox61+ fixed)

Details

(Whiteboard: [adv-main60+][post-critsmash-triage])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 2

a year ago
(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
(Assignee)

Comment 3

a year ago
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
(Assignee)

Comment 4

a year ago
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+
(Assignee)

Comment 6

a year ago
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
(Assignee)

Comment 7

a year ago
Setting the ESR52 status to disabled to reflect that the buggy code is there, but unused in the builds we ship off that branch.
(Assignee)

Comment 8

a year ago
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
Last Resolved: a year 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+

Comment 14

a year ago
(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)
(Assignee)

Comment 15

a year ago
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)
(Assignee)

Updated

a year ago
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.