The default bug view has changed. See this FAQ.

pixman image ref-counting races with cairo solid colors

RESOLVED FIXED in Firefox 47

Status

()

Core
Graphics: Layers
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

(Blocks: 1 bug, {csectype-uaf, regression, sec-high})

32 Branch
mozilla49
csectype-uaf, regression, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox46 wontfix, firefox47+ fixed, firefox48+ fixed, firefox49+ fixed, firefox-esr4547+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main47+][adv-esr45.2+])

Attachments

(2 attachments)

(Assignee)

Description

11 months ago
https://cgit.freedesktop.org/cairo/commit/?id=71e8a4c23019b01aa43b334fcb2784c70daae9b5
https://bugs.freedesktop.org/show_bug.cgi?id=

pixman_image_t has virtual function pointers, making use after free serious, but triggering the race is likely difficult.
(Assignee)

Comment 1

11 months ago
Created attachment 8753596 [details]
debug session highlights
(Assignee)

Updated

11 months ago
Blocks: 994541, 1268202
Keywords: regression
(Assignee)

Updated

11 months ago
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Updated

11 months ago
(Assignee)

Comment 2

11 months ago
Created attachment 8753622 [details] [diff] [review]
don't reuse pixman images when not thread-safe

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f064f419ca59
Attachment #8753622 - Flags: review?(jmuizelaar)

Updated

11 months ago
Group: core-security → gfx-core-security
Attachment #8753622 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 3

11 months ago
Comment on attachment 8753622 [details] [diff] [review]
don't reuse pixman images when not thread-safe

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Difficult but conceivable.  An increment needs to be run concurrently with a decrement and these can only happen on main and compositor threads afaik.
Content would need to control the content of an allocation very soon after the refcount decrement, but that could be done on a worker thread.  The |cache| variable provides opportunity to retry continually until success.

> 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?
All of them.

> If not all supported branches, which bug introduced the flaw?
Bug dates back to bug 562746 at least.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The code has not changed much so backports are not difficult.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.  It just removes some reuse of objects, instead now creating a new object each time.
Attachment #8753622 - Flags: sec-approval?
(Assignee)

Comment 4

11 months ago
(In reply to Karl Tomlinson (ni?:karlt) from comment #3)
> [Security approval request comment]
> > If not all supported branches, which bug introduced the flaw?
> Bug dates back to bug 562746 at least.

Sorry, that's not correct.  The bug was introduced with OMTC, bug 899785.
Blocks: 899785
sec-approval+ for trunk. I'd like to get a patch nominated for Aurora, Beta, and ESR45 as well.
status-firefox46: --- → wontfix
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox47: --- → ?
tracking-firefox48: --- → +
tracking-firefox49: --- → +
tracking-firefox-esr45: --- → ?

Updated

11 months ago
Attachment #8753622 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 6

10 months ago
Comment on attachment 8753622 [details] [diff] [review]
don't reuse pixman images when not thread-safe

Approval Request Comment
[Feature/regressing bug #]:
Bug 899785.
[User impact if declined]:
Random crash (bug 1268202), potentially exploitable.
[Describe test coverage new/current, TreeHerder]:
No new test.  Exploiting the race would require careful timing or brute force.
Code is widely used and so exercised by existing tests.
[Risks and why]: 
Unlikely.  The patch just removes some reuse of objects, instead now creating a new object each time.
[String/UUID change made/needed]:
None

[Approval Request Comment]
Fix Landed on Version: 49
Attachment #8753622 - Flags: approval-mozilla-esr45?
Attachment #8753622 - Flags: approval-mozilla-beta?
Attachment #8753622 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 7

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb131a07c5bc
Target Milestone: --- → mozilla49

Comment 8

10 months ago
Comment on attachment 8753622 [details] [diff] [review]
don't reuse pixman images when not thread-safe

Sec-high, Aurora48+, Beta47+, ESR45+
Attachment #8753622 - Flags: approval-mozilla-esr45?
Attachment #8753622 - Flags: approval-mozilla-esr45+
Attachment #8753622 - Flags: approval-mozilla-beta?
Attachment #8753622 - Flags: approval-mozilla-beta+
Attachment #8753622 - Flags: approval-mozilla-aurora?
Attachment #8753622 - Flags: approval-mozilla-aurora+

Updated

10 months ago
tracking-firefox47: ? → +
tracking-firefox-esr45: ? → 47+

Comment 9

10 months ago
https://hg.mozilla.org/releases/mozilla-beta/rev/256e89214e18
status-firefox47: affected → fixed

Comment 10

10 months ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/94a3ac0c8c86
https://hg.mozilla.org/releases/mozilla-esr45/rev/28dcecced055
status-firefox48: affected → fixed
status-firefox-esr45: affected → fixed

Comment 11

10 months ago
https://hg.mozilla.org/mozilla-central/rev/fb131a07c5bc
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox49: affected → fixed
Resolution: --- → FIXED

Updated

10 months ago
Group: gfx-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

Updated

10 months ago
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main47+][adv-esr45.2+]
Version: unspecified → 32 Branch
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.