1663508 - Crash in [@ -[ChildView pasteboard:item:provideDataForType:]]

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

Crash report: https://crash-stats.mozilla.org/report/index/37e6ce26-cfff-47f4-971a-651310200905

Top 10 frames of crashing thread:

0 XUL -[ChildView pasteboard:item:provideDataForType:] widget/cocoa/nsChildView.mm:4599
1 CoreFoundation -[_CFPasteboardEntry resolveLocalPromisedData] 
2 CoreFoundation ___CFPasteboardHandleFulfillMessage_block_invoke_2 
3 CoreFoundation __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__ 
4 CoreFoundation __CFRunLoopDoBlocks 
5 CoreFoundation __CFRunLoopRun 
6 CoreFoundation CFRunLoopRunSpecific 
7 CoreFoundation CFPasteboardResolveAllPromisedData 
8 CoreFoundation __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ 
9 CoreFoundation ___CFXRegistrationPost_block_invoke 

I found this crash during nightly crash triage but it's not a new one, it seems to have been around for a while as we've got crash reports going back six months. It appears that we're crashing while accessing gDraggedTransferables. The crashing address is either a completely bogus pointer (often a non-canonical address) or it's the poison pattern. My guess is that this is an UAF where sometimes we access memory that has already been reused which would explain the bogus pointers.

I'm not familiar with this code but seeing how gDraggedTransferables is a global holding a raw pointer that is accessed in multiple places my guess is that this is a race of some sort.

Comment 1

[Stephen A Pohl [:spohl]]

Setting n-i for myself to take another look at this.

Comment 2

[Stephen A Pohl [:spohl]]

Attachment: Bug 1663508: Make access to the pasteboard transferable more deliberate on macOS. r=mstange

Comment 3

[Stephen A Pohl [:spohl]]

Attachment: Bug 1663508: Collect information about threads that are used during drag sessions on macOS (nightly-only). r=mstange

Comment 4

[Stephen A Pohl [:spohl]]

Comment on attachment 9255957 [details]
Bug 1663508: Make access to the pasteboard transferable more deliberate on macOS. r=mstange

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This patch serves the purpose of verifying that only the main thread is involved in drag sessions on macOS, and that the underlying reason for the reported crash is not a race by different threads to access a global. This patch should not make it any easier to construct an exploit than before.
• 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?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This patch is not a fix. It only serves the purpose of verifying that only the main thread is involved in drag sessions on macOS, or to collect better crash reports in case other threads are involved.
• How likely is this patch to cause regressions; how much testing does it need?: This patch is restricted to Nightly-only. There is a chance that we will get new crash reports if threads other than the main thread are involved in drag sessions on macOS, but that should help us fix the security bug reported here.

Comment 5

[Stephen A Pohl [:spohl]]

The requested sec-approval is for D134186 only at this time.

Comment 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9255963 [details]
Bug 1663508: Collect information about threads that are used during drag sessions on macOS (nightly-only). r=mstange

Approved to land. I think you could have used MOZ_DIAGNOSTIC_ASSERT, no? I guess that includes early beta...

Comment 7

[Stephen A Pohl [:spohl]]

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #6)

Comment on attachment 9255963 [details]
Bug 1663508: Collect information about threads that are used during drag sessions on macOS (nightly-only). r=mstange

Approved to land. I think you could have used MOZ_DIAGNOSTIC_ASSERT, no? I guess that includes early beta...

I was not aware of MOZ_DIAGNOSTIC_ASSERT. If we don't get enough data as-is it's good to know that we have one more option to try. Thank you!

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Collect information about threads that are used during drag sessions on macOS (nightly-only). r=mstange
https://hg.mozilla.org/integration/autoland/rev/4eef9a6c17cda7bb1163b185b853476cdd51db8c
https://hg.mozilla.org/mozilla-central/rev/4eef9a6c17cd

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Hi Stephen, anything interesting so far?

Comment 10

[Stephen A Pohl [:spohl]]

Yes, I had a chance to briefly look into this and will follow up with a better update shortly. Leaving n-i set.

Comment 11

[Stephen A Pohl [:spohl]]

Attachment: Bug 1663508: Handle interruptions of drag sessions more gracefully on macOS. r=mstange

Comment 12

[Stephen A Pohl [:spohl]]

Comment on attachment 9267311 [details]
Bug 1663508: Handle interruptions of drag sessions more gracefully on macOS. r=mstange

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It is rather unlikely that an exploit can be constructed based on this patch. This intermittent crash requires a rather complicated sequence of events, including a user manually initiating a drag session that is later interrupted (by the primary password dialog for example), and I can't think of an obvious way to reliably exploit this.
• 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?: all
• If not all supported branches, which bug introduced the flaw?: Bug 358093
• 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?: This patch only changes a pointer from a weak pointer to a strong pointer. The strong pointer type (StaticRefPtr) is an existing pointer type that is used all over our codebase, so it is unlikely to introduce any regressions.

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9267311 [details]
Bug 1663508: Handle interruptions of drag sessions more gracefully on macOS. r=mstange

Approved to land and uplift

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Handle interruptions of drag sessions more gracefully on macOS. r=mstange
https://hg.mozilla.org/integration/autoland/rev/de2f992edd63eb4dda772b5ea64f3509a74f3d00
https://hg.mozilla.org/mozilla-central/rev/de2f992edd63

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR approval when you get a chance.

Comment 16

[Stephen A Pohl [:spohl]]

Comment on attachment 9267311 [details]
Bug 1663508: Handle interruptions of drag sessions more gracefully on macOS. r=mstange

Beta/Release Uplift Approval Request

• User impact if declined: Intermittent crashes when dragging.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: It is rather tricky to get a reproducible test case for this crash, so manual verification is unreliable.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch simply changes a weak pointer to a strong pointer.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: Intermittent crashes when dragging.
• Fix Landed on Version: 100
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch simply changes a weak pointer to a strong pointer.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 18

[Stephen A Pohl [:spohl]]

(In reply to Release mgmt bot [:marco/ :calixte] from comment #17)

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.

Completed the form.

Comment 19

[Donal Meehan [:dmeehan]]

Comment on attachment 9267311 [details]
Bug 1663508: Handle interruptions of drag sessions more gracefully on macOS. r=mstange

Approved for 99.0b5. Thanks.

Comment 20

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d2d15933d83b

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9267311 [details]
Bug 1663508: Handle interruptions of drag sessions more gracefully on macOS. r=mstange

Approved for 91.8esr.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/d6b476cadd24