1547472 - UAF when saving file in Gecko Profiler when capturing with screenshots on

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P2)

Description

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

Start Gecko Profiler with Screenshots on. Capture a profile. Save the profile to a file.

Twice in a row I crashed doing this; the second time in gdb and it appears that the ProfilerScreenshots instance captured in a lambda via "[this, " had been freed.

It needs to have a longer lifetime than the runnable (which might be tricky to ensure) or hold a reference to the object (perhaps via RefPtr<ProfilerScreenshots> self(this); .... [self, ...)

While this is a UAF, assigning sec-moderate due to needing to install the Profiler, be capturing, and have screenshots on (not the default IIRC), which can't be done under external control.

Comment 1

[Markus Stange [:mstange]]

I thought I had commented on this before but I can't find it.

ProfilerScreenshots is not reference counted. The initial implementation made sure that the thread only exists while the ProfilerScreenshots object exists. But then thread shutdown got moved to a main thread runnable, and now the thread exists for too long, and that's allowing this UAF.

I see two possible fixes:

• The ProfilerScreenshots destructor could block until all runnables on the thread have been processed, and only then schedule the thread shutdown.
• Alternatively, we could make ProfilerScreenshots reference counted and only pass the object around via strong pointers, as suggested in comment 0.

Comment 2

[Barret Rennie [:barret] (they/them)]

I'm going to take this over since Markus is going to be away.

Comment 3

[Barret Rennie [:barret] (they/them)]

Attachment: Bug 1547472 - Ref count the ProfilerScreenshots structure r?gerald

Comment 4

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

https://hg.mozilla.org/integration/autoland/rev/cc4f2327ff1941b6fbe29fee50df3172a1a80256
https://hg.mozilla.org/mozilla-central/rev/cc4f2327ff19

The patch applies cleanly on beta. Can you request uplift, please?

Comment 5

[Barret Rennie [:barret] (they/them)]

[Tracking Requested - why for this release]:

Fixes a use-after-free crash present in 68

Comment 6

[Barret Rennie [:barret] (they/them)]

Comment on attachment 9067429 [details]
Bug 1547472 - Ref count the ProfilerScreenshots structure r?gerald

Beta/Release Uplift Approval Request

• User impact if declined: Possible use-after-free when using the gecko profiler, which could be used to exploit or crash the browser.
• 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: Low
• Why is the change risky/not risky? (and alternatives if risky): This is not risky due to this code path only being hit while running the Gecko Profiler with the screenshots feature enabled, which most of our users will not be doing.
• String changes made/needed:

Comment 7

[Barret Rennie [:barret] (they/them)]

This patch should apply cleanly to release if the changes to RendererScreenshotGrabber.{cpp,h} are dropped. Should we target uplift into release after uplift into beta?

Comment 8

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

That's a call for the release managers. The patch applies mostly on release, only patching the RendererScreenshotGrabber.* files fails because those didn't exist yet in 67.

Comment 9

[Julien Cristau [:jcristau]]

We don't normally uplift security fixes to release unless we're in a chemspill situation.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9067429 [details]
Bug 1547472 - Ref count the ProfilerScreenshots structure r?gerald

uaf fix for profiler, approved for 68.0b6

Comment 11

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

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

Comment 12

[Anca Soncutean, Desktop QA]

Unfortunately I couldn’t reproduce this issue with Fx 68.0a1 (2019-04-27) or Fx 67.0b8 using Windows 10 x64 and macOS 10.13. I used Gecko Profiler add-on, version 0.30. I don’t know what I am missing here, steps look pretty straightforward. The only thing that seems different is that on an older version I can see a different UI, the Save as file button is usable directly from the header, not through the Publish - Download buttons, but then again, the actual version was already available when the bug was submitted. Was this crash triggered intermittent, or maybe I misunderstood something here?

Comment 13

[Barret Rennie [:barret] (they/them)]

Yes, this is intermittent as it is a race condition.

Comment 14

[Anca Soncutean, Desktop QA]

Since this issue is triggered intermittent as per comment 13, it is very unlikely to be able to reproduce it or to verified it properly. Therefore I will remove the qa+ flag.
Randell, maybe, if time permits can you please verify that this issue no longer occurs on your side on the latest available Beta ? Thank you!

Comment 15

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

I'm not seeing it, but it is rather random. I'll keep a watch