Closed Bug 1524246 Opened 9 months ago Closed 8 months ago

AddressSanitizer: heap-use-after-free [@ gfxUserFontEntry::FontDataDownloadComplete] with READ of size 8

Categories

(Core :: CSS Parsing and Computation, defect, critical)

x86_64
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- unaffected
firefox66 + fixed
firefox67 + fixed

People

(Reporter: decoder, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage])

Attachments

(2 files)

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 67.0a1-20190128214724-https://hg.mozilla.org/mozilla-central/rev/d305772af471766618393c01065556e739738e84.

For detailed crash information, see attachment.

Flags: sec-bounty?
Keywords: sec-high
Group: core-security → layout-core-security
Flags: needinfo?(emilio)
Keywords: csectype-uaf

I have a patch that I think should address this in bug 1522417, just waiting for review.

Status: NEW → RESOLVED
Closed: 9 months ago
Flags: needinfo?(emilio)
Resolution: --- → DUPLICATE
Duplicate of bug: 1522417

Oh, this is actually slightly different.

Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

That log is amazing, thanks! <3

Assignee: nobody → emilio
Component: Graphics: Text → CSS Parsing and Computation

When the FontFaceSet gets unlinked, we remove the strong pointer it holds to to
the UserFontSet.

This is not strictly necessary, since that object will no longer have any
reference to any other cycle collected object.

In any case, the loaders keep alive the user font entries, which don't keep
alive the user font set (they have a weak reference instead). So if the user
font set is gone, all is bad.

Ensure we cancel all loads when unlinking rather than just when the object is
destroyed, and that the font face loader doesn't keep a reference to the user
font entry anymore after being canceled (this shouldn't be necessary either, but
it's better IMO).

Comment on attachment 9040499 [details]
Bug 1524246 - Cancel font loads from unlinking.

Security Approval Request

How easily could an exploit be constructed based on the patch?

Not easily, it's timing / network dependent.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes

Which older supported branches are affected by this flaw?

66+

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

Bug 1519918

Do you have backports for the affected branches?

Yes

If not, how different, hard to create, and risky will they be?

Should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?

Not likely at all. Just does some cleanup a little earlier.

Attachment #9040499 - Flags: sec-approval?
Duplicate of this bug: 1525093

Comment on attachment 9040499 [details]
Bug 1524246 - Cancel font loads from unlinking.

sec-approval+ for trunk.

Attachment #9040499 - Flags: sec-approval? → sec-approval+
Group: layout-core-security → core-security-release
Status: REOPENED → RESOLVED
Closed: 9 months ago8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Blocks: 1522417

Comment on attachment 9040499 [details]
Bug 1524246 - Cancel font loads from unlinking.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1519918

User impact if declined

Crashes with security impact.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

Bug 1523181

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This patch is really low-risk, just does some cleanup earlier. The patch in bug 1523181 is a bit more risky IMO. Alternative is backing out the regressing bug on beta, which seems straight-forward.

String changes made/needed

none

Attachment #9040499 - Flags: approval-mozilla-beta?

Comment on attachment 9040499 [details]
Bug 1524246 - Cancel font loads from unlinking.

Crash fix with security impact, let's uplift for beta 6.

Attachment #9040499 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1525351
Flags: sec-bounty? → sec-bounty+
Blocks: 1522416
Duplicate of this bug: 1524945
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.