If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Heap-use-after-free [@ nsLayoutUtils::SurfaceFromElement] with drawImage, many canvas elements

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Core
Canvas: 2D
--
critical
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: Jesse Ruderman, Assigned: lsalzman, NeedInfo)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla46
crash, csectype-uaf, sec-critical, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 wontfix, firefox44+ fixed, firefox45+ fixed, firefox46+ fixed, firefox-esr3844+ fixed, b2g-v2.5 fixed)

Details

(Whiteboard: [adv-main44+][adv-esr38.6+][post-critsmash-triage])

Attachments

(8 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8696103 [details]
testcase (crashes ASan Firefox)

(Why 62?)
(Reporter)

Comment 1

2 years ago
Created attachment 8696104 [details]
ASan stacks
(Reporter)

Comment 2

2 years ago
Created attachment 8696105 [details]
debug stack

In a non-ASan debug build, it crashes after a few reloads.
(Assignee)

Updated

2 years ago
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
Created attachment 8698205 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement

nsLayoutUtils::SurfaceFromElement(..., mTarget) calls canvas->GetSurfaceSnapshot(), which may change mTarget if it needs to transition from a GPU context to a software context.

Since SurfaceFromElement receives this as a raw DrawTarget*, changes in mTarget are not tracked at all, which causes the problem when mTarget is freed, as it was not ref'd.

However, merely keeping it alive is not what we want, since we want the final value of mTarget by the time we actually need to interact with it later inside SurfaceFromElement. Thus, this patch modifies the interface to receive the DT as a RefPtr<DrawTarget>&, so any changes in the DT are properly tracked and also ensuring there is a ref to it.
Attachment #8698205 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

2 years ago
Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05947082061e

Also of note, the SurfaceFromElement(..., DrawTarget*) change originated with this commit: https://hg.mozilla.org/mozilla-central/rev/491765fa039c
Comment on attachment 8698205 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement

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

Seems safer.
Attachment #8698205 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8698573 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement

This doesn't change anything from the originally reviewed patch, but it adds the testcase as a crashtest.
Attachment #8698205 - Attachment is obsolete: true
Attachment #8698573 - Flags: review+
(Assignee)

Comment 7

2 years ago
Comment on attachment 8698573 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
The crashtest that is added points to how at least the crash can be triggered, but does not highlight the use-after-free or where exactly that is occurring.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
As above, the crashtest does show how to reproduce the crash and the part of the code associated with that crash, but doesn't explain the use-after-free implication.

> Which older supported branches are affected by this flaw?
Everything ESR 38 and above.

> If not all supported branches, which bug introduced the flaw?
https://hg.mozilla.org/mozilla-central/rev/491765fa039c

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch should work (in theory) down to 38. If not, porting it to 38 should be relatively painless.

> How likely is this patch to cause regressions; how much testing does it need?
The patch doesn't alter semantics much, so aside from the risk that the crashtest itself causes some platforms to fail it (which it does not seem to do in try runs - https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fbd3f00e26a), it should be okay.
Attachment #8698573 - Flags: sec-approval?
Two things:

1. As a rule, we don't check in tests for security bugs until well after we ship the fix publicly. So this patch would need to be broken up into the fix and the test, with the test to go in later.

2. This can't get sec-approval without a security rating on the bug. This *sounds like* a drive by triggerable use-after-free but the comments on s-a? are all about the test really, not the actual problem. Is my assessment correct? If so, this is probably a sec-critical.
Flags: needinfo?(lsalzman)
(Assignee)

Comment 9

2 years ago
Comment on attachment 8698205 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Patch doesn't point to the canvas mechanism required to trigger the issue in SurfaceFromElement.

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

> Which older supported branches are affected by this flaw?
38+

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch should apply against most other branches, but may require rebasing for 38.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely, passes all tests on try.
Attachment #8698205 - Attachment is obsolete: false
Flags: needinfo?(lsalzman)
Attachment #8698205 - Flags: sec-approval?
(Assignee)

Updated

2 years ago
Attachment #8698573 - Attachment is obsolete: true
Attachment #8698573 - Flags: sec-approval?
(Assignee)

Comment 10

2 years ago
Created attachment 8698973 [details] [diff] [review]
add crashtest for bug 1230686

Separated crashtest from fix.
Attachment #8698973 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Keywords: sec-critical
Attachment #8698973 - Flags: review?(jmuizelaar) → review+
status-firefox43: --- → wontfix
status-firefox44: --- → affected
status-firefox46: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox44: --- → +
tracking-firefox45: --- → +
tracking-firefox46: --- → +
tracking-firefox-esr38: --- → 44+
Comment on attachment 8698205 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement

sec-approval+ for trunk.
We'll want Aurora, Beta, and ESR38 patches made and nominated once it is on trunk as well.
Attachment #8698205 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 12

2 years ago
Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fbd3f00e26a
Keywords: checkin-needed
this failed to apply:

adding surface-from-element-dt.diff to series file
applying surface-from-element-dt.diff
patching file layout/base/nsLayoutUtils.cpp
Hunk #1 FAILED at 6883
1 out of 4 hunks FAILED -- saving rejects to file layout/base/nsLayoutUtils.cpp.rej
patching file layout/base/nsLayoutUtils.h
Hunk #1 FAILED at 2110
1 out of 1 hunks FAILED -- saving rejects to file layout/base/nsLayoutUtils.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh surface-from-element-dt.diff
Flags: needinfo?(lsalzman)
(Assignee)

Comment 14

2 years ago
Created attachment 8702596 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement

Refreshed to work against latest ninja changes on central.
Flags: needinfo?(lsalzman)
Attachment #8702596 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f93defa8ee9a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f93defa8ee9a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Lee, could you please nominate patches for uplift to Beta, Aurora and ESR38? Thanks!
Flags: needinfo?(lsalzman)
(Assignee)

Comment 18

2 years ago
Comment on attachment 8698205 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement

Approval Request Comment
[Feature/regressing bug #]: bug 948221
[User impact if declined]: sec-critical use-after-free
[Describe test coverage new/current, TreeHerder]: mochitest, reftest
[Risks and why]: Minimal, has been on central with no issues.
[String/UUID change made/needed]: None
Flags: needinfo?(lsalzman)
Attachment #8698205 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 19

2 years ago
Created attachment 8703734 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement (beta)

Approval Request Comment
[Feature/regressing bug #]: bug 948221
[User impact if declined]: sec-critical use-after-free
[Describe test coverage new/current, TreeHerder]: mochitest, reftest
[Risks and why]: Minimal, has been on central with no issues.
[String/UUID change made/needed]: None
Attachment #8703734 - Flags: review+
Attachment #8703734 - Flags: approval-mozilla-beta?
(Assignee)

Comment 20

2 years ago
Created attachment 8703735 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement (esr38)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: sec-critical use-after-free 
Fix Landed on Version: 46
Risk to taking this patch (and alternatives if risky): Minimal, has been on central with no issues.
String or UUID changes made by this patch: None 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8703735 - Flags: review+
Attachment #8703735 - Flags: approval-mozilla-esr38?
Comment on attachment 8698205 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement

sec-crit fix, Aurora45+
Attachment #8698205 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8703735 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement (esr38)

sec-crit fix, ers38.6+
Attachment #8703735 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Comment on attachment 8703734 [details] [diff] [review]
use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement (beta)

sec-crit fix, beta44+
Attachment #8703734 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-esr38/rev/93617c30c0df
https://hg.mozilla.org/releases/mozilla-beta/rev/ec2b2ab4e57d
https://hg.mozilla.org/releases/mozilla-aurora/rev/45faafefe2f8
status-firefox44: affected → fixed
status-firefox45: affected → fixed
status-firefox-esr38: affected → fixed
Group: gfx-core-security → core-security-release
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ec2b2ab4e57d
status-b2g-v2.5: --- → fixed
Jesse, can you verify fixed?
Flags: needinfo?(jruderman)
Whiteboard: [adv-main44+][adv-esr38.6+]
Whiteboard: [adv-main44+][adv-esr38.6+] → [adv-main44+][adv-esr38.6+][post-critsmash-triage]
Group: core-security-release

Updated

10 months ago
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.