Closed Bug 970710 Opened 6 years ago Closed 6 years ago

"ASSERTION: Invalid offset" in gfxSkipChars with MathML, LRO

Categories

(Core :: Layout: Text and Fonts, defect)

x86_64
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox27 --- unaffected
firefox28 + fixed
firefox29 + verified
firefox30 + verified
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jruderman, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-moderate, testcase, Whiteboard: [fuzzblocker])

Attachments

(3 files)

Attached file testcase
###!!! ASSERTION: Invalid offset: 'uint32_t(aOffset) <= mSkipChars->mCharCount', file gfx/thebes/gfxSkipChars.cpp, line 13

Regression from bug 955957.

Might be related to bug 961859.
Attached file stack
Whiteboard: [fuzzblocker]
Are you sure this is a regression? I tried backing out bug 955957 locally, and still hit the same assertion (it just moved to a new source line number):

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file gfx/thebes/gfxSkipChars.cpp, line 61
I confess I don't really understand this code, but it seems like we're calling GetTrimmedOffsets from SetupJustificationSpacing with a default aPostReflow parameter, which doesn't match the explicit value used in InitializeForMeasure. Is that OK, or should the two calls be consistent? This patch prevents us hitting the assertion here, and doesn't break anything that I can see on tryserver... https://tbpl.mozilla.org/?tree=Try&rev=c5c2e99f3b96.
Attachment #8374368 - Flags: review?(roc)
Comment on attachment 8374368 [details] [diff] [review]
ensure GetTrimmedOffsets is called with consistent parameters from PropertyProvider::InitializeForMeasure and SetupJustificationSpacing

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

This is a simple bug fix basically. InitializeForMeasure is called during reflow, so we should be passing false for aPostReflow in GetTrimmedOffsets.
Attachment #8374368 - Flags: review?(roc) → review+
Comment on attachment 8374368 [details] [diff] [review]
ensure GetTrimmedOffsets is called with consistent parameters from PropertyProvider::InitializeForMeasure and SetupJustificationSpacing

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not readily, afaics. The error occurs during measurement, not frame/textrun construction, so it seems unlikely to lead directly to an out-of-bounds write or anything like that; the risk is only that we do an out-of-bounds read, which might result in a crash.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, it seems far from obvious to me. The one clue is that "text-align:justify" may somehow be relevant, but that alone doesn't lead anywhere interesting.

Which older supported branches are affected by this flaw? mozilla-28 and later

If not all supported branches, which bug introduced the flaw? bug 415413

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? patch applies cleanly on current mozilla-aurora and mozilla-beta

How likely is this patch to cause regressions; how much testing does it need? risk looks minimal to me
Attachment #8374368 - Flags: sec-approval?
The relevant code here was introduced in bug 415413; marking affected Firefox versions accordingly. I'm not sure exactly how that translates to b2g versions...
Assignee: nobody → jfkthame
Blocks: 415413
Comment on attachment 8374368 [details] [diff] [review]
ensure GetTrimmedOffsets is called with consistent parameters from PropertyProvider::InitializeForMeasure and SetupJustificationSpacing

sec-approval+.

Please nominate the patch for Aurora and Beta after it goes in (assuming there are no issues) so we can avoid shipping the issue.
Attachment #8374368 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/f2430a5e4379
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
(In reply to Al Billings [:abillings] from comment #7)
> Please nominate the patch for Aurora and Beta after it goes in (assuming
> there are no issues) so we can avoid shipping the issue.
Flags: needinfo?(jfkthame)
Comment on attachment 8374368 [details] [diff] [review]
ensure GetTrimmedOffsets is called with consistent parameters from PropertyProvider::InitializeForMeasure and SetupJustificationSpacing

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 415413

User impact if declined: potential crash (though not obvious whether it might be further exploitable)

Testing completed (on m-c, etc.): verified locally to avoid the inconsistency that may lead to out-of-bounds read; landed on m-c without problems

Risk to taking this patch (and alternatives if risky): minimal; simple bug-fix

String or IDL/UUID changes made by this patch: none
Attachment #8374368 - Flags: approval-mozilla-beta?
Attachment #8374368 - Flags: approval-mozilla-b2g28?
Attachment #8374368 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jfkthame)
Attachment #8374368 - Flags: approval-mozilla-beta?
Attachment #8374368 - Flags: approval-mozilla-beta+
Attachment #8374368 - Flags: approval-mozilla-aurora?
Attachment #8374368 - Flags: approval-mozilla-aurora+
Attachment #8374368 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Are you sure this is a regression? I tried backing out bug 955957 locally,
> and still hit the same assertion (it just moved to a new source line number):
> 
> ###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file
> gfx/thebes/gfxSkipChars.cpp, line 61

I see what happened. The assertion in comment 2, but not the assertion in comment 0, is in my ignore list (due to bug 847242). I ran my bisect tool in a way that used the ignore list, so it blamed the changeset that changed which assertion fired.
Confirmed assert on Fx29, 2014-02-10.
Verified fixed on Fx29, Fx30, 2014-03-25.
Status: RESOLVED → VERIFIED
Landed the test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f7d8b2ca94
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.