Closed Bug 1664257 Opened 11 months ago Closed 10 months ago

Crash in [@ mozilla::WeakPtr<T>::operator=]

Categories

(Core :: Graphics: WebRender, defect, P1)

defect

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox80 --- unaffected
firefox81 --- wontfix
firefox82 + fixed
firefox83 + fixed

People

(Reporter: aryx, Assigned: jgilbert)

References

(Blocks 1 open bug, Regression)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][sec-survey][adv-main82+r])

Crash Data

Attachments

(1 file)

First crash for 82.0a1 has build id 20200829091226, for 80 beta first affected is 81.0b4. Is there a change which got uplifted for beta 4 (or 3) and landed shortly before on central?

Crash report: https://crash-stats.mozilla.org/report/index/2ac9de80-6106-4a83-8cfa-318880200830

Top 10 frames of crashing thread:

0 xul.dll mozilla::WeakPtr<mozilla::layers::PersistentBufferProvider>::operator= mfbt/WeakPtr.h:257
1 xul.dll mozilla::layers::BorrowedSourceSurface::BorrowedSourceSurface gfx/layers/CanvasRenderer.cpp:27
2 xul.dll mozilla::layers::CanvasRenderer::BorrowSnapshot const gfx/layers/CanvasRenderer.cpp:66
3 xul.dll mozilla::layers::ShareableCanvasRenderer::UpdateCompositableClient gfx/layers/ShareableCanvasRenderer.cpp:189
4 xul.dll nsDisplayCanvas::CreateWebRenderCommands layout/generic/nsHTMLCanvasFrame.cpp:144
5 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommands gfx/layers/wr/WebRenderCommandBuilder.cpp:1653
6 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList gfx/layers/wr/WebRenderCommandBuilder.cpp:1758
7 xul.dll nsDisplayOwnLayer::CreateWebRenderCommands layout/painting/nsDisplayList.cpp:6335
8 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommands gfx/layers/wr/WebRenderCommandBuilder.cpp:1653
9 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList gfx/layers/wr/WebRenderCommandBuilder.cpp:1758
Flags: needinfo?(kats)

Not sure about the uplift timeline discrepancy but this code looks like it was introduced in bug 1632249. Redirecting to jgilbert.

Flags: needinfo?(kats) → needinfo?(jgilbert)
Flags: needinfo?(jgilbert)
Regressed by: CVE-2020-15678

(In reply to Jeff Gilbert [:jgilbert] from comment #2)

It's bug 1654211.

What do you mean? Is this bug duplicate of bug 1654211, or caused by bug 1654211?

Flags: needinfo?(jgilbert)

This bug is a regression due to bug 1654211, as I labeled them.

Flags: needinfo?(jgilbert)
Severity: -- → S3

(In reply to Jeff Gilbert [:jgilbert] from comment #4)

This bug is a regression due to bug 1654211, as I labeled them.

If you mean the "regressed by" field, it does not show anything for people who are not cc'd on that bug.

Keywords: regression

(In reply to Miko Mynttinen [:miko] from comment #5)

(In reply to Jeff Gilbert [:jgilbert] from comment #4)

This bug is a regression due to bug 1654211, as I labeled them.

If you mean the "regressed by" field, it does not show anything for people who are not cc'd on that bug.

Oh right, of course. :S

Group: core-security
Flags: needinfo?(jgilbert)
Keywords: csectype-uaf
Group: core-security → gfx-core-security

Ok so here we're assigning a dead (UAF) object into a WeakPtr, so we started using WeakPtr too late.

Flags: needinfo?(jgilbert)
Priority: -- → P1
Keywords: sec-high
Blocks: gfx-triage
Blocks: gfx-82
No longer blocks: gfx-triage
Assignee: nobody → jgilbert

Jeff, any update?

Flags: needinfo?(jgilbert)

My theory is that we're taking a reference to provider, but that may get changed out if provider->BorrowSnapshot() fails and we call context->GetFrontBufferSnapshot(...).
We need a strongref to provider instead, and we also need to null provider if BorrowSnapshot fails, since we're not borrowing in that case.

Flags: needinfo?(jgilbert)

Also null out provider if we failed to borrow from the provider.

Comment on attachment 9179389 [details]
Bug 1664257 - Hold a ref to BufferProvider in CanvasRenderer::BorrowSnapshot.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: I don't think an exploit is viable.
  • 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?: 82+
  • If not all supported branches, which bug introduced the flaw?: Bug 1654211
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?:
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions, since things should only change when we would be hitting this UAF, and this patch should prevent the UAF.
Attachment #9179389 - Flags: sec-approval?

Comment on attachment 9179389 [details]
Bug 1664257 - Hold a ref to BufferProvider in CanvasRenderer::BorrowSnapshot.

Beta/Release Uplift Approval Request

  • User impact if declined: sec-high
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: none
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This should just make things better, but it does mess with lifetimes a little.
  • String changes made/needed: none
Attachment #9179389 - Flags: approval-mozilla-beta?

Comment on attachment 9179389 [details]
Bug 1664257 - Hold a ref to BufferProvider in CanvasRenderer::BorrowSnapshot.

approved for 82.0b9

Attachment #9179389 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(jgilbert)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][sec-survey]
Flags: needinfo?(jgilbert)
Whiteboard: [post-critsmash-triage][sec-survey] → [post-critsmash-triage][sec-survey][adv-main82+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.