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

RESOLVED FIXED

Status

()

Core
Graphics: Text
--
critical
RESOLVED FIXED
a year ago
26 days ago

People

(Reporter: tsmith, Unassigned)

Tracking

(Blocks: 1 bug, {sec-high, testcase})

Trunk
sec-high, testcase
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45- wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main52+] fixed by harfbuzz 1.4.0)

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8782042 [details]
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.
(Reporter)

Comment 1

a year ago
Created attachment 8782044 [details]
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)

Comment 5

a year ago
Ok, my bad, from the commit hash looks like this is current.  I'll take a look soonish.
Flags: needinfo?(mozilla)
(Reporter)

Comment 6

a year ago
(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.
(Reporter)

Comment 7

a year ago
(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)
(Reporter)

Comment 10

11 months ago
(In reply to Al Billings [:abillings] from comment #9)
> Tyson, is this still reproducing?

Yes
Flags: needinfo?(twsmith)
(Reporter)

Comment 11

11 months ago
(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.

Comment 12

11 months ago
I believe I fixed this in 1.4.0
Flags: needinfo?(mozilla)
(Reporter)

Comment 13

10 months ago
I can confirm this is in fact no longer reproducible.
I understand we can close this bug?
Flags: needinfo?(dveditz)

Comment 15

10 months ago
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
Last Resolved: 10 months ago
status-firefox52: --- → affected
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
Resolution: --- → WORKSFORME
Tyson: Did you ever check the in-tree version (comment 6)?
status-firefox53: unaffected → fixed
status-firefox54: unaffected → fixed
status-firefox-esr45: --- → affected
tracking-firefox-esr45: --- → ?
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)

Updated

10 months ago
Group: gfx-core-security → core-security-release
(Reporter)

Comment 18

10 months ago
(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.
status-firefox51: affected → wontfix

Comment 20

10 months ago
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.
status-firefox52: affected → fixed
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)

Comment 23

9 months ago
(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)
Created attachment 8836289 [details] [diff] [review]
cherry-picked patch for esr45

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)

Comment 25

9 months ago
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.
status-firefox-esr45: affected → wontfix
I'm fine with that.
tracking-firefox-esr45: ? → -
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.