Closed Bug 1295299 Opened 5 years ago Closed 4 years ago
[harfbuzz] Assertion `i <= out
_len + (len - idx)' failed [@hb _buffer _t::move _to]
3.57 KB, text/plain
1.33 KB, application/x-font-ttf
1.90 KB, patch
|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.
I think I've fixed this in newer HarfBuzz. Will check later.
(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.
I think this should be fixed in newer HarfBuzz already. Jonathan, can you confirm?
Tyson, do you have a specific input string that triggered this when used with the attached .ttf?
Ok, my bad, from the commit hash looks like this is current. I'll take a look soonish.
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.
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.
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)
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)
Looks like kcc has come across this too. https://github.com/behdad/harfbuzz/issues/139#issuecomment-252016067
Updated since old test case no longer repro
Attachment #8781258 - Attachment is obsolete: true
Looks like it is happening in the wild: https://crash-stats.mozilla.com/report/index/f304c926-629c-4080-8dc8-dddae2161225
Can you take this bug, Jonathan?
Assignee: nobody → jfkthame
Tyson, is there an input string that goes along with the test_case.ttf to trigger this? Thanks!
(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@_%&)*$!"
I've opened https://github.com/behdad/harfbuzz/pull/421 with a proposed fix for this.
Fixed upstream. Thanks Jonathan.
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)
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
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.
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.
Tracking this since it's sec-high.
Attachment #8838470 - Flags: review?(jmuizelaar) → review+
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 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/37c22fd0475e9ecddb207d5eed729ab4a1945ad7 Bug 1295299 - Cherry-pick harfbuzz fix 44f7d6ecde9bf7427a05cbe73ed5d668b8a72b2a. r=jrmuizel
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
Comment on attachment 8838470 [details] [diff] [review] Cherry-pick harfbuzz fix 44f7d6ecde9bf7427a05cbe73ed5d668b8a72b2a sec-high harfbuzz bug fix for aurora53+beta52
Setting qe-verify- based on Jonathan's assessment on manual testing needs (see Comment 28) and the fact that this fix has automated coverage.
You need to log in before you can comment on or make changes to this bug.