1448771 - Buffer overflow in hyphen (not the same as 1390550)

Status: RESOLVED FIXED

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

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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. 

see https://github.com/hunspell/hyphen/pull/14/

Comment 1

[Jonathan Kew [:jfkthame]]

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).

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Merge fix from upstream

Comment 4

[Jonathan Kew [:jfkthame]]

Well, that didn't exactly go smoothly; sheriff is backing it out:

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8f614217727ae5bf3742ce7623df0433d61c3274

Android bustage failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170476265&repo=mozilla-inbound&lineNumber=7763

Reftest failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170477781&repo=mozilla-inbound&lineNumber=10713

Web platform reftest failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170477933&repo=mozilla-inbound&lineNumber=4416  and  https://treeherder.mozilla.org/logviewer.html#?job_id=170478508&repo=mozilla-inbound&lineNumber=7818

Comment 5

[Jonathan Kew [:jfkthame]]

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/267a6fa83b73

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Update hnjstdio to handle additional functions from stdio.h that libhyphen wants to use

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.

Comment 7

[Ryan VanderMeulen [: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?

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/846bcaa210aa2264bec412c0595113964fafc972
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5db47b5c6416a3cef546de1aa9089d98109f95f

https://hg.mozilla.org/mozilla-central/rev/846bcaa210aa
https://hg.mozilla.org/mozilla-central/rev/f5db47b5c641

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 10

[Jonathan Kew [: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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8962422 [details] [diff] [review]
Merge fix from upstream

Approved for 60.0b8.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2f2e601e1394
https://hg.mozilla.org/releases/mozilla-beta/rev/75cefda36f28