Closed
Bug 1295299
Opened 8 years ago
Closed 7 years ago
[harfbuzz] Assertion `i <= out_len + (len - idx)' failed [@hb_buffer_t::move_to]
Categories
(Core :: Graphics: Text, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: tsmith, Assigned: jfkthame)
References
(Blocks 1 open bug)
Details
(Keywords: sec-high, testcase, Whiteboard: [adv-main52+])
Attachments
(3 files, 1 obsolete file)
3.57 KB,
text/plain
|
Details | |
1.33 KB,
application/x-font-ttf
|
Details | |
1.90 KB,
patch
|
jrmuizel
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
Found while fuzzing harfbuzz revision 18c19dd34dcdcaab0a6d47768339f8fb70c0d3f0 hb-fuzzer: hb-buffer.cc:419: bool hb_buffer_t::move_to(unsigned int): Assertion `i <= out_len + (len - idx)' failed. #0 0x00007ffff65be418 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54 #1 0x00007ffff65c001a in __GI_abort () at abort.c:89 #2 0x00007ffff65b6bd7 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x646c40 <.str> "i <= out_len + (len - idx)", file=file@entry=0x646960 <.str.1> "hb-buffer.cc", line=line@entry=419, function=function@entry=0x646be0 <__PRETTY_FUNCTION__._ZN11hb_buffer_t7move_toEj> "bool hb_buffer_t::move_to(unsigned int)") at assert.c:92 #3 0x00007ffff65b6c82 in __GI___assert_fail ( assertion=0x646c40 <.str> "i <= out_len + (len - idx)", file=0x646960 <.str.1> "hb-buffer.cc", line=419, function=0x646be0 <__PRETTY_FUNCTION__._ZN11hb_buffer_t7move_toEj> "bool hb_buffer_t::move_to(unsigned int)") at assert.c:101 #4 0x00000000004f7e5a in hb_buffer_t::move_to (this=0x61200000bd40, i=<optimized out>) at hb-buffer.cc:419 #5 0x00000000005c079f in OT::apply_lookup (c=<optimized out>, count=<optimized out>, match_positions=<optimized out>, lookupCount=<optimized out>, lookupRecord=<optimized out>, match_length=<optimized out>) at ./hb-ot-layout-gsubgpos-private.hh:1036 #6 0x0000000000612ff4 in OT::chain_context_apply_lookup (c=0x7fffffffd220, backtrackCount=<optimized out>, backtrack=<optimized out>, inputCount=1, lookaheadCount=0, lookupCount=4133217304, input=<optimized out>, lookahead=<optimized out>, lookupRecord=<optimized out>, lookup_context=...) at ./hb-ot-layout-gsubgpos-private.hh:1649 #7 OT::ChainContextFormat3::apply (this=<optimized out>, c=<optimized out>) at ./hb-ot-layout-gsubgpos-private.hh:2089 #8 0x00000000005c1513 in hb_get_subtables_context_t::hb_applicable_t::apply (c=0x7fffffffd220, this=<optimized out>) at hb-ot-layout.cc:999 ... Full log is attached.
Reporter | ||
Comment 1•8 years ago
|
||
Comment 2•8 years ago
|
||
I think I've fixed this in newer HarfBuzz. Will check later.
Reporter | ||
Comment 3•8 years ago
|
||
(In reply to Behdad Esfahbod from comment #2) > I think I've fixed this in newer HarfBuzz. Will check later. Where can I find the latest code? I've been using https://github.com/behdad/harfbuzz.
Updated•8 years ago
|
Flags: needinfo?(mozilla)
Comment 4•8 years ago
|
||
I think this should be fixed in newer HarfBuzz already. Jonathan, can you confirm?
Flags: needinfo?(mozilla)
Assignee | ||
Comment 5•8 years ago
|
||
Tyson, do you have a specific input string that triggered this when used with the attached .ttf?
Flags: needinfo?(twsmith)
Comment 6•8 years ago
|
||
Ok, my bad, from the commit hash looks like this is current. I'll take a look soonish.
Reporter | ||
Comment 7•8 years ago
|
||
Great thanks! FYI: I am frequently updating and fuzzing the latest code to help catch issues before the make it into Firefox or other products.
Flags: needinfo?(twsmith)
Reporter | ||
Comment 8•8 years ago
|
||
According to https://dxr.mozilla.org/mozilla-central/source/gfx/harfbuzz/README-mozilla We are including commit 547ddb0...c5f82 from Aug 18th which will be affected by this issue.
Comment 9•8 years ago
|
||
rating "sec-other" because this doesn't seem to be a problem in Firefox--the font is rejected prior to being seen by harfbuzz. Could be a bad boundary bug in harfbuzz leading to vulnerabilities in other products, though. Behdad: did you get a chance to look at this? (comment 6)
Flags: needinfo?(mozilla)
Keywords: sec-other
Comment 10•8 years ago
|
||
Although _this_ testcase/font is rejected before getting handed to harfbuzz, we don't know that another font couldn't be constructed to trigger this same problem (bug 1296024 comment 3)
Reporter | ||
Comment 11•7 years ago
|
||
Looks like kcc has come across this too. https://github.com/behdad/harfbuzz/issues/139#issuecomment-252016067
Reporter | ||
Comment 13•7 years ago
|
||
Updated since old test case no longer repro
Attachment #8781258 -
Attachment is obsolete: true
Reporter | ||
Comment 14•7 years ago
|
||
Looks like it is happening in the wild: https://crash-stats.mozilla.com/report/index/f304c926-629c-4080-8dc8-dddae2161225
Assignee | ||
Comment 16•7 years ago
|
||
Tyson, is there an input string that goes along with the test_case.ttf to trigger this? Thanks!
Flags: needinfo?(twsmith)
Reporter | ||
Comment 17•7 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #16) > Tyson, is there an input string that goes along with the test_case.ttf to > trigger this? Thanks! I am using https://github.com/behdad/harfbuzz/blob/master/test/fuzzing/hb-fuzzer.cc as a fuzzing harness. The value used when fuzzing is "ABCDEXYZ123@_%&)*$!"
Flags: needinfo?(twsmith)
Assignee | ||
Comment 18•7 years ago
|
||
I've opened https://github.com/behdad/harfbuzz/pull/421 with a proposed fix for this.
Assignee | ||
Comment 20•7 years ago
|
||
As it's a simple fix, I think we can safely cherry-pick this (and potentially uplift it) without waiting to take a complete new HB release.
Attachment #8838470 -
Flags: review?(jmuizelaar)
Comment 21•7 years ago
|
||
I *think* that esr45 is affected as well, it's going to need a different patch than the one posted here. https://dxr.mozilla.org/mozilla-esr45/source/gfx/harfbuzz/src/hb-ot-layout-gsubgpos-private.hh#991
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox-esr45:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
tracking-firefox-esr45:
--- → ?
tracking-firefox-esr52:
--- → ?
Assignee | ||
Comment 22•7 years ago
|
||
Yes, I wouldn't be surprised if esr45 could be affected by this or a similar issue. Some ways of hitting this problem don't exist because at that point, harfbuzz didn't allow a GSUB MultipleSubst table to delete a glyph (see comment in the code there); but it's possible that other approaches involving things like ContextSubst would still lead to the same fault. The fix for esr45 will be essentially the same -- use a signed value to avoid integer underflow -- just adapted to the older form of the HB code; I'll attach a patch.
Assignee | ||
Comment 23•7 years ago
|
||
Actually, looking at that code again, I don't think there is a problem. It calculates (end + delta), like the newer code, using signed integer arithmetic so it could yield a negative result; but it doesn't simply assign that to the (unsigned) `end` variable, which is where things go bad in later versions. Here, though, that's wrapped in the MAX(MIN(...)) stuff which will prevent a negative value "leaking" out to the unsigned assignment. So esr45 is unaffected, I believe.
Comment 24•7 years ago
|
||
Tracking this since it's sec-high.
Updated•7 years ago
|
Attachment #8838470 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 25•7 years ago
|
||
Comment on attachment 8838470 [details] [diff] [review] Cherry-pick harfbuzz fix 44f7d6ecde9bf7427a05cbe73ed5d668b8a72b2a [Security approval request comment] How easily could an exploit be constructed based on the patch? For someone with a good understanding of OpenType font structures and behavior, it's not too hard to guess how to cause an out-of-bounds read that will probably cause a crash (e.g. SEGV). It's less obvious how to turn that into an exploit (rather than a DoS), however. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? Firefox 51 and later (Beta, Aurora). Esr-45 looks unaffected. If not all supported branches, which bug introduced the flaw? Harfbuzz update in bug 1269343. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Trivial backport. How likely is this patch to cause regressions; how much testing does it need? Minimal regression risk, just ensures we use a signed variable for a value that might go negative, to avoid unsigned-int underflow. No change in behavior for any currently-working font.
Attachment #8838470 -
Flags: sec-approval?
Comment 26•7 years ago
|
||
Comment on attachment 8838470 [details] [diff] [review] Cherry-pick harfbuzz fix 44f7d6ecde9bf7427a05cbe73ed5d668b8a72b2a sec-approval+ for trunk. We should get this on Aurora and Beta so it makes it to ESR52 as well. Please nominate for those.
Attachment #8838470 -
Flags: sec-approval? → sec-approval+
Updated•7 years ago
|
tracking-firefox-esr45:
? → ---
tracking-firefox-esr52:
? → ---
Assignee | ||
Comment 27•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/37c22fd0475e9ecddb207d5eed729ab4a1945ad7 Bug 1295299 - Cherry-pick harfbuzz fix 44f7d6ecde9bf7427a05cbe73ed5d668b8a72b2a. r=jrmuizel
Assignee | ||
Comment 28•7 years ago
|
||
Comment on attachment 8838470 [details] [diff] [review] Cherry-pick harfbuzz fix 44f7d6ecde9bf7427a05cbe73ed5d668b8a72b2a Approval Request Comment [Feature/Bug causing the regression]: upstream harfbuzz bug, picked up when we took HB 1.3.0 in bug 1269343 [User impact if declined]: potential for a bad/malicious webfont to cause an out-of-bounds access, probably leading to a crash; unclear whether a worse exploit could be devised [Is this code covered by automated tests?]: yes, exercised in general by opentype font shaping; specific testcase for this flaw added to upstream [Has the fix been verified in Nightly?]: tested in standalone harfbuzz [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?]: simple type change to avoid risk of unsigned-int underflow/wrapping to huge value [String changes made/needed]: none
Attachment #8838470 -
Flags: approval-mozilla-beta?
Attachment #8838470 -
Flags: approval-mozilla-aurora?
Comment 29•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/37c22fd0475e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 30•7 years ago
|
||
Comment on attachment 8838470 [details] [diff] [review] Cherry-pick harfbuzz fix 44f7d6ecde9bf7427a05cbe73ed5d668b8a72b2a sec-high harfbuzz bug fix for aurora53+beta52
Attachment #8838470 -
Flags: approval-mozilla-beta?
Attachment #8838470 -
Flags: approval-mozilla-beta+
Attachment #8838470 -
Flags: approval-mozilla-aurora?
Attachment #8838470 -
Flags: approval-mozilla-aurora+
Comment 31•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/ba7e52e5c26a https://hg.mozilla.org/releases/mozilla-beta/rev/10bce9625147
status-firefox51:
--- → wontfix
tracking-firefox-esr52:
--- → ?
Updated•7 years ago
|
Group: gfx-core-security → core-security-release
Comment 32•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/10bce9625147
Comment 33•7 years ago
|
||
Setting qe-verify- based on Jonathan's assessment on manual testing needs (see Comment 28) and the fact that this fix has automated coverage.
Flags: qe-verify-
Updated•7 years ago
|
tracking-firefox-esr52:
? → ---
Whiteboard: [adv-main52+]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•