Closed Bug 1425257 Opened 2 years ago Closed 2 years ago

Crash in mozilla::gfx::DrawTargetD2D1::Flush

Categories

(Core :: Graphics, defect, critical)

58 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- wontfix
firefox59 + fixed
firefox60 + fixed

People

(Reporter: philipp, Assigned: bas.schouten)

References

Details

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

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is
report bp-4161558c-9197-45e7-ba1c-cf2190171214.
=============================================================

Top 6 frames of crashing thread:

0 xul.dll mozilla::gfx::DrawTargetD2D1::Flush gfx/2d/DrawTargetD2D1.cpp:176
1 xul.dll mozilla::gfx::DrawTargetD2D1::MarkChanged gfx/2d/DrawTargetD2D1.cpp:1295
2 xul.dll mozilla::gfx::DrawTargetD2D1::ClearRect gfx/2d/DrawTargetD2D1.cpp:327
3 xul.dll mozilla::dom::CanvasRenderingContext2D::ClearRect dom/canvas/CanvasRenderingContext2D.cpp:3219
4 xul.dll mozilla::dom::CanvasRenderingContext2DBinding::clearRect dom/bindings/CanvasRenderingContext2DBinding.cpp:5307
5  @0x31f93e112c7 

=============================================================

these crashes in the content process on windows are continuing after the patch for bug 1416864 has landed in 58.0b9 targetting the same signature.

https://mozilla.github.io/stab-crashes/supergraph.html?s=https%3A%2F%2Fcrash-stats.mozilla.com%2Fsearch%2F%3Fsignature%3D%253Dmozilla%253A%253Agfx%253A%253ADrawTargetD2D1%253A%253AFlush%26version%3D58.0b
From the crash dump, it crashed at [1] and the address of DrawTargetD2D1 instance was filled with 0xe5e5e5. I think this is a UAF issue. 

mDependingOnTargets is used to store the raw pointer of DrawTargetD2D1. Therefore, it is possible to access a DrawTargetD2D1 which got destroyed.

[1]https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/gfx/2d/DrawTargetD2D1.cpp#176
'mDependentTargets' is also used to store the raw pointer of DrawTargetD2D1. These two structures are used to perform some synchronization. I'm thinking to add a lock to protect 'mDependentTargets' but not mDependingOnTargets to prevent the deadlock.
Group: gfx-core-security
Lee, this looks pretty inactive while we aim for a ~2 week timeframe for all sec-highs.
Can you bump this in your queue? If not, is there a way we can support you getting there?
Flags: needinfo?(lsalzman)
Bas, ideas? Looks like something still not going right with the locking added in bug 1416864?
Assignee: lsalzman → bas
Flags: needinfo?(lsalzman) → needinfo?(bas)
I feel I know what's going on here. (For the D2D case, I don't think that particular Skia issue has anything to do with this)
Flags: needinfo?(bas)
This seems like the safest and most upliftable approach.
Attachment #8948440 - Flags: review?(lsalzman)
Comment on attachment 8948440 [details] [diff] [review]
Use a global mutex to guard the DT dependency graph.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very difficult, the patch gives little information about where the freed objects are actually located.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? On the bug, yes, on a way to exploit it, no.

Which older supported branches are affected by this flaw? 58+ This patch applies there.

How likely is this patch to cause regressions; how much testing does it need? Extremely unlikely, the patch only introduces an additional lock.
Attachment #8948440 - Flags: sec-approval?
Attachment #8948440 - Flags: review?(lsalzman) → review+
sec-approval+ for trunk.
Please request bounty approval on the patch (or another if necessary) as well.
Attachment #8948440 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #9)
> sec-approval+ for trunk.
> Please request bounty approval on the patch (or another if necessary) as
> well.

I don't know much about out procedures here.. This was posted from the Socorro interface and not as a security bug, is a bounty appropriate here? (Sorry Philipp :-))
Flags: needinfo?(abillings)
https://hg.mozilla.org/mozilla-central/rev/e30390f2f53a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(In reply to Bas Schouten (:bas.schouten) from comment #10)
> 
> I don't know much about out procedures here.. This was posted from the
> Socorro interface and not as a security bug, is a bounty appropriate here?
> (Sorry Philipp :-))

As something without a test case and which only has data as a top crash, it doesn't really meet the bar.
Flags: needinfo?(abillings)
Given the low frequency with which this is happening on Nightly, are we likely to get any good sense of whether this is fixed before uplifting to Beta?
Flags: needinfo?(bas)
Comment on attachment 8948440 [details] [diff] [review]
Use a global mutex to guard the DT dependency graph.

I was trying to determine that, I'd say the answer is no.

Approval Request Comment
[Feature/Bug causing the regression]: OMTP
[User impact if declined]: Rare, but sec-high crash.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]:Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only adds a lock, a tiny perf regression is the only potential risk.
[String changes made/needed]: None
Flags: needinfo?(bas)
Attachment #8948440 - Flags: approval-mozilla-beta?
Comment on attachment 8948440 [details] [diff] [review]
Use a global mutex to guard the DT dependency graph.

Let's get this into today's 59b8 so we can see if it helps sooner.
Attachment #8948440 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: gfx-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.