Closed Bug 1448771 Opened 2 years ago Closed 2 years ago
Buffer overflow in hyphen (not the same as 1390550)
1.76 KB, patch
|Details | Diff | Splinter Review|
3.51 KB, patch
|Details | Diff | Splinter Review|
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/
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.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8962422 - Flags: review?(ryanvm) → review+
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
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+
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
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 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
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+
Whiteboard: [third-party-lib-audit] exploit requires malicious langpack → [third-party-lib-audit][adv-main60+] exploit requires malicious langpack
Whiteboard: [third-party-lib-audit][adv-main60+] exploit requires malicious langpack → [post-critsmash-triage][third-party-lib-audit][adv-main60+] exploit requires malicious langpack
You need to log in before you can comment on or make changes to this bug.