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)

Other Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- wontfix
firefox52 + fixed
firefox-esr52 --- fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: tsmith, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Keywords: sec-high, testcase, Whiteboard: [adv-main52+])

Attachments

(3 files, 1 obsolete file)

Attached file log.txt
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.
Attached file test_case.ttf (obsolete) —
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.
Flags: needinfo?(mozilla)
I think this should be fixed in newer HarfBuzz already.  Jonathan, can you confirm?
Flags: needinfo?(mozilla)
Tyson, do you have a specific input string that triggered this when used with the attached .ttf?
Flags: needinfo?(twsmith)
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.
Flags: needinfo?(twsmith)
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)
Flags: needinfo?(mozilla)
Keywords: sec-other
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)
Keywords: sec-othersec-high
Looks like kcc has come across this too.
https://github.com/behdad/harfbuzz/issues/139#issuecomment-252016067
See Also: → 1336823
Attached file test_case.ttf
Updated since old test case no longer repro
Attachment #8781258 - Attachment is obsolete: true
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!
Flags: needinfo?(twsmith)
(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)
I've opened https://github.com/behdad/harfbuzz/pull/421 with a proposed fix for this.
Fixed upstream.  Thanks Jonathan.
Flags: needinfo?(mozilla)
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)
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
Attachment #8838470 - Flags: approval-mozilla-beta?
Attachment #8838470 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/37c22fd0475e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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+
Group: gfx-core-security → core-security-release
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-
Whiteboard: [adv-main52+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.