1664149 - Crash in [@ nsDataObj::DropTempFile]

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P1)

Description

[Simon Giesecke [:sg] [he/him]]

Crash report: https://crash-stats.mozilla.org/report/index/27dcd26a-c8ef-49f7-869f-ab13a0200908

Top 10 frames of crashing thread:

0 xul.dll nsDataObj::DropTempFile widget/windows/nsDataObj.cpp:1775
1 xul.dll nsDataObj::GetFile widget/windows/nsDataObj.cpp:1542
2 xul.dll nsDataObj::GetData widget/windows/nsDataObj.cpp:690
3 ole32.dll HandleFromHandle com\ole32\ole232\clipbrd\clipapi.cpp:2048
4 ole32.dll RenderCurrentFormat com\ole32\ole232\clipbrd\clipapi.cpp:4052
5 ole32.dll RenderFormat com\ole32\ole232\clipbrd\clipapi.cpp:4185
6 ole32.dll ClipboardWndProc com\ole32\ole232\clipbrd\clipapi.cpp:683
7 user32.dll int64_t UserCallWinProcCheckWow 
8 user32.dll DispatchClientMessage 
9 user32.dll _fnDWORD 

There are multiple reports saying this happens when copying an image to the clipboard.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Some of the crashes are either UAFs (e5e5...) or wildptr's, sec bug
https://crash-stats.mozilla.org/report/index/97817a92-5ca7-462b-800d-de86c0211103

Comment 2

[Bob Owen (:bobowen)]

I've had a look at the code here and I can't immediately see the issue, assuming this is all happening on the main thread, which I think it will be.
There is some passing around of raw pointers, but it at least seems to be doing the Addrefing and Releasing correctly.
I guess I could change it to use smart pointers for those bits to be sure.

I've found steps that flow through this code and I've tried various different ways to trigger a problem, but no luck so far.
I'll spend a bit more time on this to see if I get anywhere.

Comment 3

[Bob Owen (:bobowen)]

One thing I've just noticed is that all of the crashes where it looks like a UAF are x86.

Comment 4

[Bob Owen (:bobowen)]

Coming back with fresh eyes, I think I've found something that might be the issue.
The read here can cause the event loop to be spun here.

Now in theory the caller should be holding a reference, but perhaps some apps process things async and don't expect us to process the events out of turn/recursively.

Anyway, we can hopefully just hold a reference to ourselves and see if that resolves the problem.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Bug 1664149: Hold a self reference in nsDataObj. r=smaug!

Comment 6

[Bob Owen (:bobowen)]

After discussions with smaug we decided we should try and cater for async mode (using StartOperation and EndOperation) as well.

I worked up some patches, but as GetData is called many times (particularly when dragging) it means we are constantly taking references and submitting events for them to be released.
It also requires a fair bit of logic around releasing in EndOperation, GetData and Release (in case EndOperation is not called).

I think I've come up with a simpler plan that should still cover the scenarios, only take and release the internal reference once and simplify the logic.

Comment 7

[Bob Owen (:bobowen)]

Comment on attachment 9254137 [details]
Bug 1664149: Hold a self reference in nsDataObj. r=smaug!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: We don't have steps to reproduce and at the very least it seems you would need to persuade the user to Copy and Paste or Drag and Drop an image.

Over the comments question below, I have added comments, but I don't believe they add much information for an attacker as the fix is already obviously to do with reference counting.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• 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?: 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?: Fairly unlikely, it is relatively simple holding of extra references to keep object alive when released externally.
  Some general Copy/Paste and Drag/Drop testing on Windows would be good.
  If someone can reproduce obviously that would be ideal.

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 9254137 [details]
Bug 1664149: Hold a self reference in nsDataObj. r=smaug!

approved to land and request uplift

Comment 9

[Bob Owen (:bobowen)]

Attachment: *** ESR91 *** Bug 1664149: Hold a self reference in nsDataObj. r=smaug

Comment 10

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

Landed: https://hg.mozilla.org/integration/autoland/rev/08324a167e6bc3e42419d316a3d1fc0eef1f22f6

Backed out for causing mochitest failures with test_bug1123480.xhtml:

https://hg.mozilla.org/integration/autoland/rev/44dadccd67d33f0d5fbde0038277b3d0c11a8f32

• Push with failures
• Failure log

Failure line: TEST-UNEXPECTED-FAIL | widget/tests/test_bug1123480.xhtml | should have cleared the clipboard data - got 1, expected +0

Comment 11

[Bob Owen (:bobowen)]

Attachment: Bug 1664149 p2: Wait for sanitize in test_bug1123480.xhtml. r=smaug!

Depends on D133099

Comment 12

[Bob Owen (:bobowen)]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9ebd0e17f5a71cbecd5ec6b7d360a0550ee4118
Hold a self reference in nsDataObj. r=smaug
https://hg.mozilla.org/integration/autoland/rev/f130cc178f837ad19e1a73a266e0249377416d71
p2: Wait for sanitize in test_bug1123480.xhtml. r=smaug

Comment 13

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

Hold a self reference in nsDataObj. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c9ebd0e17f5a71cbecd5ec6b7d360a0550ee4118
https://hg.mozilla.org/mozilla-central/rev/c9ebd0e17f5a

p2: Wait for sanitize in test_bug1123480.xhtml. r=smaug
https://hg.mozilla.org/integration/autoland/rev/f130cc178f837ad19e1a73a266e0249377416d71
https://hg.mozilla.org/mozilla-central/rev/f130cc178f83

Comment 14

[Bob Owen (:bobowen)]

Comment on attachment 9254137 [details]
Bug 1664149: Hold a self reference in nsDataObj. r=smaug!

Beta/Release Uplift Approval Request

• User impact if declined: Crashes still experienced when copying / dragging images (assuming this fix works).
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: We don't have STR, but if QE can reproduce that would be great.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Only saying medium because while this involves fairly simple extra ref holding, its release is submitted to the event loop.
  One test needed amending for this, but it should be unlikely that other production code relies on the release happening straight away.
• String changes made/needed: None

Comment 15

[Dianna Smith [:diannaS]]

Comment on attachment 9254137 [details]
Bug 1664149: Hold a self reference in nsDataObj. r=smaug!

Approved for 96.0b8

Comment 16

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/29af3a0a582e
https://hg.mozilla.org/releases/mozilla-beta/rev/3c3783572481

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

[Bob Owen (:bobowen)]

Comment on attachment 9256125 [details]
*** ESR91 *** Bug 1664149: Hold a self reference in nsDataObj. r=smaug

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: Crashes still experienced when copying / dragging images. Currently looks like the fix works despite not having STR to prove.
• Fix Landed on Version: 97
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Only saying medium because while this involves fairly simple extra ref holding, its release is submitted to the event loop.
  One test needed amending for this, but it should be unlikely that other production code relies on the release happening straight away.

Comment 19

[Hani Yacoub, Desktop QA]

I couldn't manage to reproduce this crash, I tried to find some accurate steps testing around copping and dragging images but I didn't succeed.
Simon, is it possible if you managed to reproduce this issue before to check if you still could reproduce the issue on the latest Firefox Nightly and on Firefox Beta?
https://www.mozilla.org/en-GB/firefox/channel/desktop/

Thanks.

Comment 20

[Simon Giesecke [:sg] [he/him]]

I don't have a way to reproduce this, unfortunately. I just filed the bug based on a number of crash reports with this signature.

Comment 21

[Mihai Boldan, Desktop QA [:mboldan]]

Based on the last two comments, I am removing the qe-verify+ flag, since we are not able to reproduce the issue.

Comment 22

[(PTO June 19-July 7) Ryan VanderMeulen]

Comment on attachment 9256125 [details]
*** ESR91 *** Bug 1664149: Hold a self reference in nsDataObj. r=smaug

Approved for 91.5esr.

Comment 23

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-esr91/rev/efafbff3bb1f
https://hg.mozilla.org/releases/mozilla-esr91/rev/48f614fa7bc9

Comment 24

[Hani Yacoub, Desktop QA]

Marking this as verified since the latest crashes were on Firefox 97.0a1, Firefox 96.0b7 and Firefox 91.4.1esr.