1504365 - Potential shutdown UAF in various shutdown observers

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Andrew McCreight [:mccr8]]

It is possible for the event loop to spin while the observer service is notifying things of shutdown. In that case, TextTractManager can be destroyed while its ShutdownObserverProxy is still alive, leading to a UAF. The fix is to clear the pointer for the shutdown observer when destroying the manager.

This happened on tree herder for similar class for the canvas context in bug 1503082.

Comment 1

[Andrew McCreight [:mccr8]]

Another class that uses this bad pattern is WidgetShutdownObserver.

Comment 2

[Andrew McCreight [:mccr8]]

Also, WillShutdownObserver in gfx/thebes/gfxFT2FontList.cpp.

I found these by auditing every class that has "ShutdownObserver" in the name. I don't know if there's a better way to look for these.

Comment 3

[Andrew McCreight [:mccr8]]

I was wrong about WidgetShutdownObserver. The widget dtor calls WidgetShutdownObserver::Unregister(), which does clear the weak pointer.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1504365 - Clear weak pointers in shutdown observers.

Comment 5

[Eric Rahm [:erahm]]

I think Gabriel is ran into something similar in has ObserverService refactoring in bug 1435683. Gabriel do you have patches that already cover this, or am I mistaken?

Comment 6

[Gabriele Svelto [:gsvelto]]

No, I hadn't run into this particular issue yet though I fixed one that's vaguely similar in bug 1476250 (but those were different type of observers).

Comment 7

[Andrew McCreight [:mccr8]]

> This changes the order of NotifyShutdown and UnregisterShutdownObserver, is that intentional and desired?

This is needed because Unregister clears mManager. I think the order shouldn't matter.

> It seems like this won't compile due to mObserver not being a member. I think you want this?

Good catch. I thought this file was Windows-only for some reason, and I did a Windows try run, but it looks like it is Android only. (Of course, the only platform I failed to test...)

> What's the point moving this to a Remove method if this is the only thing that calls it?

Remove() accesses a protected member, mFontList.

> Also, don't we still want to clear mObserver?

mObserver is an nsCOMPtr, so clearing it does not do anything.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Bug 1504365 - Clear weak pointers in shutdown observers.

Comment 9

[Andrew McCreight [:mccr8]]

This version actually builds on Android!

Comment 10

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

https://hg.mozilla.org/integration/autoland/rev/f5c1dbff0c9afe001c1f32576058138c7b799d2b
https://hg.mozilla.org/mozilla-central/rev/f5c1dbff0c9a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta/ESR60 approval when you get a chance. It grafts cleanly to both as-landed.

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 9026223 [details]
Bug 1504365 - Clear weak pointers in shutdown observers.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is probably not an exploitable issue, but it might be, and seems low risk.

User impact if declined: see previous

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Low risk. This just clears out some pointers during shutdown.

String or UUID changes made by this patch: none

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: Yes

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): 

String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9026223 [details]
Bug 1504365 - Clear weak pointers in shutdown observers.

[Triage Comment]
Low-risk sec fix, approved for 64.0b13 and 60.4.0esr.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/76164912276b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b6d0fc61fd0b