Closed Bug 1230686 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics: Canvas2D, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox43 --- wontfix
firefox44 + fixed
firefox45 + fixed
firefox46 + fixed
firefox-esr38 44+ fixed
b2g-v2.5 --- fixed

People

(Reporter: jruderman, Assigned: lsalzman, NeedInfo)

References

Details

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

Attachments

(8 files, 1 obsolete file)

(Why 62?)
Attached file ASan stacks
Attached file debug stack
In a non-ASan debug build, it crashes after a few reloads.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
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)
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+
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+
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)
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?
Attachment #8698573 - Attachment is obsolete: true
Attachment #8698573 - Flags: sec-approval?
Separated crashtest from fix.
Attachment #8698973 - Flags: review?(jmuizelaar)
Keywords: sec-critical
Attachment #8698973 - Flags: review?(jmuizelaar) → review+
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+
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)
Refreshed to work against latest ninja changes on central.
Flags: needinfo?(lsalzman)
Attachment #8702596 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f93defa8ee9a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Lee, could you please nominate patches for uplift to Beta, Aurora and ESR38? Thanks!
Flags: needinfo?(lsalzman)
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?
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?
[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+
Group: gfx-core-security → core-security-release
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
You need to log in before you can comment on or make changes to this bug.