Closed Bug 1296024 Opened 8 years ago Closed 7 years ago

[harfbuzz] Assertion `end == match_positions[idx]' failed [@OT::apply_lookup]

Categories

(Core :: Graphics: Text, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr45 - wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: tsmith, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: sec-high, testcase, Whiteboard: [post-critsmash-triage][adv-main52+] fixed by harfbuzz 1.4.0)

Attachments

(3 files)

Attached file log.txt
Found while fuzzing harfbuzz revision 18c19dd34dcdcaab0a6d47768339f8fb70c0d3f0

hb-fuzzer: hb-ot-layout-gsubgpos-private.hh:1003: bool OT::apply_lookup(OT::hb_apply_context_t*, unsigned int, unsigned int*, unsigned int, const OT::LookupRecord*, unsigned int): Assertion `end == match_positions[idx]' failed.

Program received signal SIGABRT, Aborted.
0x00007ffff719a418 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007ffff719a418 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff719c01a in __GI_abort () at abort.c:89
#2  0x00007ffff7192bd7 in __assert_fail_base (fmt=<optimized out>, 
    assertion=assertion@entry=0x44c139 "end == match_positions[idx]", 
    file=file@entry=0x44c1d8 "hb-ot-layout-gsubgpos-private.hh", line=line@entry=1003, 
    function=function@entry=0x44c500 <OT::apply_lookup(OT::hb_apply_context_t*, unsigned int, unsigned int*, unsigned int, OT::LookupRecord const*, unsigned int)::__PRETTY_FUNCTION__> "bool OT::apply_lookup(OT::hb_apply_context_t*, unsigned int, unsigned int*, unsigned int, const OT::LookupRecord*, unsigned int)") at assert.c:92
#3  0x00007ffff7192c82 in __GI___assert_fail (
    assertion=assertion@entry=0x44c139 "end == match_positions[idx]", 
    file=file@entry=0x44c1d8 "hb-ot-layout-gsubgpos-private.hh", line=line@entry=1003, 
    function=function@entry=0x44c500 <OT::apply_lookup(OT::hb_apply_context_t*, unsigned int, unsigned int*, unsigned int, OT::LookupRecord const*, unsigned int)::__PRETTY_FUNCTION__> "bool OT::apply_lookup(OT::hb_apply_context_t*, unsigned int, unsigned int*, unsigned int, const OT::LookupRecord*, unsigned int)") at assert.c:101
#4  0x0000000000425d43 in OT::apply_lookup (c=0x7fffffffd9d0, count=<optimized out>, 
    match_positions=0x7fffffffd4b0, lookupCount=<optimized out>, lookupRecord=<optimized out>, 
    match_length=<optimized out>) at hb-ot-layout-gsubgpos-private.hh:1003
...
Full log is attached.
Attached file test_case.ttf
I'm guessing you're fuzzing harfbuzz directly; does the Open Type Sanitizer reject this font if you try to use it in Firefox? That would affect the severity rating (for mozilla).
Flags: needinfo?(twsmith)
OTS doesn't validate the OpenType Layout tables that harfbuzz uses; it passes them through as-is, because harfbuzz performs its own sanitization.
Tyson: does this happen in the in-tree version of harfbuzz, or only the upstream?

Behdad: is this benign (handled)? Immediately after the assert there's a break and then the value isn't used until the moveto at the end. The condition before the assert is end <= the index so if the assert fires end is less than we think which sounds better than being too big -- but I don't know what happens with the buffer after that.
Flags: needinfo?(mozilla)
Ok, my bad, from the commit hash looks like this is current.  I'll take a look soonish.
Flags: needinfo?(mozilla)
(In reply to Daniel Veditz [:dveditz] from comment #2)
> I'm guessing you're fuzzing harfbuzz directly; does the Open Type Sanitizer
> reject this font if you try to use it in Firefox? That would affect the
> severity rating (for mozilla).

This test case will likely not affect Firefox even if the vulnerable code is in the browser because this test case has been reduced. So it will likely look invalid to anything other than the tool it was found with (hb-fuzzer in this case).

I'll check the in-tree version and post and update.
(In reply to Daniel Veditz [:dveditz] from comment #4)
> Tyson: does this happen in the in-tree version of harfbuzz, or only the
> upstream?
> 
> Behdad: is this benign (handled)? Immediately after the assert there's a
> break and then the value isn't used until the moveto at the end. The
> condition before the assert is end <= the index so if the assert fires end
> is less than we think which sounds better than being too big -- but I don't
> know what happens with the buffer after that.

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.
Flags: needinfo?(twsmith)
Behdad: did you get a chance to look at this? (comment 5)
Flags: needinfo?(mozilla)
Keywords: sec-high
Tyson, is this still reproducing?
Flags: needinfo?(twsmith)
(In reply to Al Billings [:abillings] from comment #9)
> Tyson, is this still reproducing?

Yes
Flags: needinfo?(twsmith)
(In reply to Tyson Smith [:tsmith] from comment #10)
> (In reply to Al Billings [:abillings] from comment #9)
> > Tyson, is this still reproducing?
> 
> Yes

Scratch that sorry. I was looking at the wrong bug. I'll have a look.
I believe I fixed this in 1.4.0
Flags: needinfo?(mozilla)
I can confirm this is in fact no longer reproducible.
I understand we can close this bug?
Flags: needinfo?(dveditz)
The latest update was to harfbuzz 1.4.2 (bug 1333656) for v54, and 1.4.1 (bug 1325775) for v53.
Behdad says he believes he fixed this in 1.4.0 (comment 12) and since it stopped being
reproducible (comment 13) that seems to be the case.  So I'd say v53/v54 is fixed.

We're using harfbuzz 1.3.3 in Beta (v52) so I assume v52 and older are still affected.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
Tyson: Did you ever check the in-tree version (comment 6)?
Depends on: 1325775
Flags: needinfo?(dveditz) → needinfo?(twsmith)
Resolution: WORKSFORME → FIXED
Whiteboard: fixed by harfbuzz 1.4.0
(I ask because we may need to fix this on 52 and ESR-45, either by upgrading harfbuzz or cherrypicking Behdad's patch)
Group: gfx-core-security → core-security-release
(In reply to Daniel Veditz [:dveditz] from comment #16)
> Tyson: Did you ever check the in-tree version (comment 6)?

This is reproducible in 1.3.3 and older. I have also verified the current in-tree version 1.4.1 is fixed.
Flags: needinfo?(twsmith)
To close the loop here, I'll request Beta/ESR45 approval on the 1.4.1 update over in bug 1325775.
Hi Dan, is there something we need to uplift to Beta52 for this? Who might be able to help?
Flags: needinfo?(dveditz)
It was done over in bug 1325775.
Flags: needinfo?(dveditz)
Behdad, our version of HarfBuzz on ESR45 is way too old to comfortably update to version 1.4.1. Can you please point me to the upstream commit(s) that resolved this so we can look into cherry-picking instead? Thanks!
Flags: needinfo?(mozilla)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> Behdad, our version of HarfBuzz on ESR45 is way too old to comfortably
> update to version 1.4.1. Can you please point me to the upstream commit(s)
> that resolved this so we can look into cherry-picking instead? Thanks!

If your version is way too old, it's probably way too much work to list all the relevant commits...
This particular one was 359dead96, but I don't vouch for a way-too-old HB plus this patch.
Flags: needinfo?(mozilla)
Understood, thanks Behdad! That change actually applies cleanly to the version included with ESR45, so that's nice anyway :)

That said, Tyson was unable to reproduce this assertion in the browser, so I'm not sure what we should even do here. Jonathan, should we take this for belts and suspenders' sake or just wontfix for esr45?
Attachment #8836289 - Flags: feedback?(jfkthame)
That's because hb-fuzzer puts smaller limits on buffer expansion factors before further expansion is declined.  As such, what hits a bug in hb-fuzzer does not reproduce in production builds.  But it's possible to create fonts to hit it in production as well, just bigger.
Comment on attachment 8836289 [details] [diff] [review]
cherry-picked patch for esr45

Review of attachment 8836289 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like the commit that -introduced- that assertion, rather than fixed it; it was later removed in 4b4a1b9f235598b04ce9ae1f9670fc978ab7620d.

AFAICS, esr45 is on a much older harfbuzz version (1.1.0 or thereabouts), where this isn't present at all. (So that version still suffers from bug 1128658, which is what https://github.com/behdad/harfbuzz/issues/253 was about, and doubtless various other issues, but not the assertion issue here.)
Attachment #8836289 - Flags: feedback?(jfkthame) → feedback-
I'm calling this wontfix for esr45 then (potentially even unaffected would have been better). But either way, this feels like more backport trouble than it's worth. Feel free to change it back if you feel otherwise, Jonathan.
I'm fine with that.
Flags: qe-verify-
Whiteboard: fixed by harfbuzz 1.4.0 → [post-critsmash-triage] fixed by harfbuzz 1.4.0
Whiteboard: [post-critsmash-triage] fixed by harfbuzz 1.4.0 → [post-critsmash-triage][adv-main52+] fixed by harfbuzz 1.4.0
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.