Closed Bug 1390550 Opened 5 years ago Closed 5 years ago

Buffer Overflow in Hyphen


(Core :: Layout: Text and Fonts, defect)

Not set



Tracking Status
firefox-esr52 56+ fixed
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed


(Reporter: tjr, Assigned: jfkthame)


(Keywords: sec-high, Whiteboard: [third-party-lib-audit][adv-main56+][adv-esr52.4+][post-critsmash-triage])


(1 file)

There was a recent commit in Hyphen ( that fixes a buffer overflow: 

Glen confirmed the vulnerable code is present in our library. I've confirmed the vulnerable function is wired up to Mozilla code:

hnj_hyphen_hyphword (vulnerable) is called by hnj_hyphen_hyphenate2 (inside Hyphen), which is called by nsHyphenator::Hyphenate (mozilla code), which is called by nsLineBreaker::FindHyphenationPoints (mozilla code)
Assignee: nobody → ryanvm
Group: core-security → layout-core-security
Yes, it looks like this could be reachable within gecko. It should be straightforward to drop in the latest upstream code here.
Attachment #8897994 - Flags: review?(ryanvm)
hah, I was already running this through Try!
Comment on attachment 8897994 [details] [diff] [review]
Update to latest libhyphen code from upstream

Here's a link to my Try push.

Please include in your patch as well.
Attachment #8897994 - Flags: review?(ryanvm) → review+
Assignee: ryanvm → jfkthame
> hah, I was already running this through Try!

Cool, thanks! :)

(I try to avoid using Try for sec bugs, so I checked locally that our hyphenation tests still pass as expected. Though in this case as the patch has landed upstream it's not exactly secret, so the try run doesn't actually expose anything new.)

> Please include
> in your patch as well.

Ah yes, good point.
Comment on attachment 8897994 [details] [diff] [review]
Update to latest libhyphen code from upstream

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch in effect points to the upstream changes, which in turn point to a possible buffer overflow; I think it's fairly easy to go from there to a testcase that would crash Gecko. Whether/how easily that could be converted to a more sinister exploit is unclear to me.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The buffer overflow issue is described in github issues upstream, so it is already public

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?
n/a - longstanding flaw in library code from upstream

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial backport, this code hasn't changed recently in m-c.

How likely is this patch to cause regressions; how much testing does it need?
Minimal risk; we have reftests that confirm the basic functionality, and the important change here is just avoiding a fixed-size buffer that could overflow.
Attachment #8897994 - Flags: sec-approval?
I'm going to call this a sec-high and give sec-approval. Please don't mention security issues when updating to the upstream changes.

We'll want this on Beta and ESR52 branches as well.
Attachment #8897994 - Flags: sec-approval? → sec-approval+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8897994 [details] [diff] [review]
Update to latest libhyphen code from upstream

Approval Request Comment
[Feature/Bug causing the regression]: vulnerability in libhyphen
[User impact if declined]: potential buffer overflow in hyphenation code
[Is this code covered by automated tests?]: normal use is exercised by reftests
[Has the fix been verified in Nightly?]: no, it's currently unclear to me how to trigger the flaw within the browser (but not confident it is unreachable)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: minor changes to guard against overflow, tested upstream
[String changes made/needed]: none

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: potential buffer overflow (crash, possibly other exploit) in libhyphen
Fix Landed on Version: 57
Risk to taking this patch (and alternatives if risky): minimal
String or UUID changes made by this patch: none
Attachment #8897994 - Flags: approval-mozilla-esr52?
Attachment #8897994 - Flags: approval-mozilla-beta?
Comment on attachment 8897994 [details] [diff] [review]
Update to latest libhyphen code from upstream

Fix a sec-high. Beta56+ & ESR52+.
Attachment #8897994 - Flags: approval-mozilla-esr52?
Attachment #8897994 - Flags: approval-mozilla-esr52+
Attachment #8897994 - Flags: approval-mozilla-beta?
Attachment #8897994 - Flags: approval-mozilla-beta+
Group: layout-core-security → core-security-release
Whiteboard: [third-party-lib-audit] → [third-party-lib-audit][adv-main56+][adv-esr52.4+]
Flags: qe-verify-
Whiteboard: [third-party-lib-audit][adv-main56+][adv-esr52.4+] → [third-party-lib-audit][adv-main56+][adv-esr52.4+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.