Cherry-pick an upstream FreeType integer overflow fix

RESOLVED FIXED in Firefox 60

Status

()

defect
--
critical
RESOLVED FIXED
Last year
10 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)

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

Last year
(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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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: Last year
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

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

Updated

Last year
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.