Closed Bug 1448771 Opened 2 years ago Closed 2 years ago

Buffer overflow in hyphen (not the same as 1390550)


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

Not set



Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed


(Reporter: pauljt, Assigned: jfkthame)


(Keywords: csectype-bounds, sec-moderate, Whiteboard: [post-critsmash-triage][third-party-lib-audit][adv-main60+] exploit requires malicious langpack)


(2 files)

I was looking at 1390550 and I noticed that there was a pull request AFTER the one we merged, which appears to fix a buffer overflow. While the pull request description doesn't say buffer overflow, but from reading the comments it sounds like it is. 

Yes, it looks like we should take this.

AFAICS, I don't think this represents an immediately-exploitable issue for Firefox: there are no over-long lines in the patterns we ship, and a site can't directly inject additional patterns. However, if a user could be persuaded to install a malicious add-on that includes a new hyphenation dictionary, it's possible this would open up a vulnerability (e.g. allowing the add-on to corrupt memory, run arbitrary code, etc).
Can Web Extensions install hyphenation dictionaries? Or is that limited to legacy add-ons? I suppose language packs are their own type, but more like legacy add-ons.
Group: network-core-security → layout-core-security
Whiteboard: [third-party-lib-audit] → [third-party-lib-audit] exploit requires malicious langpack
Attachment #8962422 - Flags: review?(ryanvm)
Assignee: nobody → jfkthame
Attachment #8962422 - Flags: review?(ryanvm) → review+
Argh -- I completely forgot that we intercept the C stdio-based i/o that libhyphen wants to use, so that we can read hyphenation resources from omnijar instead of from separate text files. :( So with this update which adds use of fgets and feof, we need to handle those functions as well. This patch (to land before the actual hyphen.c change from upstream) should deal with it.
Attachment #8962741 - Flags: review?(ryanvm)
Comment on attachment 8962741 [details] [diff] [review]
Update hnjstdio to handle additional functions from stdio.h that libhyphen wants to use

Comparing against the upstream commit is one thing, but this isn't something I'm comfortable reviewing. Maybe glandium?
Attachment #8962741 - Flags: review?(ryanvm) → review?(mh+mozilla)
Attachment #8962741 - Flags: review?(mh+mozilla) → review+
I don't think we need to worry about this for ESR52 (feel free to dispute that if you feel strongly otherwise), but I think this is definitely worth a backport to Beta anyway.
Flags: needinfo?(jfkthame)
Comment on attachment 8962422 [details] [diff] [review]
Merge fix from upstream

Yes, probably worth uplifting (although given that it would only affect users who somehow install an extra hyphenation language that turns out to be bad, I'm not overly worried about it).

Approval Request Comment
[Feature/Bug causing the regression]: no regression, existing flaw in library
[User impact if declined]: potential buffer overflow (but not in Firefox as shipped, requires a malicious hyphenation dictionary to be installed) -- could lead to crash, or maybe some other exploit
[Is this code covered by automated tests?]: normal loading of hyphenation patterns is covered
[Has the fix been verified in Nightly?]: no, but is tested in upstream
[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?]: not really
[Why is the change risky/not risky?]: just adds simple error handling
[String changes made/needed]: none
Flags: needinfo?(jfkthame)
Attachment #8962422 - Flags: approval-mozilla-beta?
Attachment #8962741 - Flags: approval-mozilla-beta?
Comment on attachment 8962422 [details] [diff] [review]
Merge fix from upstream

Approved for 60.0b8.
Attachment #8962422 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8962741 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: layout-core-security → core-security-release
Whiteboard: [third-party-lib-audit] exploit requires malicious langpack → [third-party-lib-audit][adv-main60+] exploit requires malicious langpack
Flags: qe-verify-
Whiteboard: [third-party-lib-audit][adv-main60+] exploit requires malicious langpack → [post-critsmash-triage][third-party-lib-audit][adv-main60+] exploit requires malicious langpack
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.