Closed Bug 1504365 Opened 10 months ago Closed 9 months ago

Potential shutdown UAF in various shutdown observers

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 64+ fixed
firefox63 --- wontfix
firefox64 + fixed
firefox65 + fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [adv-main64+][adv-esr60.4+])

Attachments

(1 file, 1 obsolete file)

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.
Another class that uses this bad pattern is WidgetShutdownObserver.
Component: DOM → General
Summary: Potential shutdown UAF in TextTrackManager::ShutdownObserverProxy::Observe() → Potential shutdown UAF in various shutdown observers
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.
I was wrong about WidgetShutdownObserver. The widget dtor calls WidgetShutdownObserver::Unregister(), which does clear the weak pointer.
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?
Flags: needinfo?(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).
Flags: needinfo?(gsvelto)
Blocks: 1506911
> 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.
Attachment #9023443 - Attachment is obsolete: true
This version actually builds on Android!
https://hg.mozilla.org/integration/autoland/rev/f5c1dbff0c9afe001c1f32576058138c7b799d2b
https://hg.mozilla.org/mozilla-central/rev/f5c1dbff0c9a
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Please nominate this for Beta/ESR60 approval when you get a chance. It grafts cleanly to both as-landed.
Flags: needinfo?(continuation)
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:
Flags: needinfo?(continuation)
Attachment #9026223 - Flags: approval-mozilla-esr60?
Attachment #9026223 - Flags: approval-mozilla-beta?
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.
Attachment #9026223 - Flags: approval-mozilla-esr60?
Attachment #9026223 - Flags: approval-mozilla-esr60+
Attachment #9026223 - Flags: approval-mozilla-beta?
Attachment #9026223 - Flags: approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [adv-main64+][adv-esr60.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.