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

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes ASan Firefox)

(Why 62?)

Comment 1

[Jesse Ruderman]

Attachment: ASan stacks

Comment 2

[Jesse Ruderman]

Attachment: debug stack

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

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: 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.

Comment 4

[Lee Salzman [:lsalzman]]

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 5

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: 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.

Comment 7

[Lee Salzman [:lsalzman]]

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.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 9

[Lee Salzman [: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.

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: add crashtest for bug 1230686

Separated crashtest from fix.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 12

[Lee Salzman [:lsalzman]]

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fbd3f00e26a

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Lee Salzman [:lsalzman]]

Attachment: use RefPtr<DrawTarget>& instead of DrawTarget* to track changes in SurfaceFromElement

Refreshed to work against latest ninja changes on central.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f93defa8ee9a

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f93defa8ee9a

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Lee, could you please nominate patches for uplift to Beta, Aurora and ESR38? Thanks!

Comment 18

[Lee Salzman [: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

Comment 19

[Lee Salzman [:lsalzman]]

Attachment: 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

Comment 20

[Lee Salzman [:lsalzman]]

Attachment: 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.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

sec-crit fix, Aurora45+

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

sec-crit fix, ers38.6+

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

sec-crit fix, beta44+

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ec2b2ab4e57d

Comment 26

[Matt Wobensmith [:mwobensmith][:matt:]]

Jesse, can you verify fixed?