1339601 - Possible UAF in AppWindow::Destroy()

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Andrew McCreight [:mccr8]]

mozilla::AutoRestore<bool> guard(mDestroying);
...
nsCOMPtr<nsIXULWindow> placeHolder = this;

If placeHolder is the last reference to the window, then the AutoRestore will turn into a (probably unexploitable) UAF.

Comment 1

[Andrew McCreight [:mccr8]]

The comment suggests the deathgrip isn't really needed:

  // XXXTAB This shouldn't be an issue anymore because the ownership model
  // only goes in one direction.  When webshell container is fully removed
  // try removing this...

Comment 2

[Brian Grinstead [:bgrins]]

This file has been moved so updating the title to match: https://searchfox.org/mozilla-central/rev/13b081a62d3f3e3e3120f95564529257b0bf451c/xpfe/appshell/AppWindow.cpp#636.

Brendan, do you have any idea if the deathgrip here is needed, or who else to ask? The comment there says "When webshell container is fully removed try removing this" but I'm not sure exactly what this is referring to.

Comment 3

[Brendan Dahl [:bdahl]]

I'm not sure what the web shell container refers to. Maybe Olli knows if we can get rid of this^

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

The comment isn't quite right, at least not anymore, since in theory anything could happen when xul-window-destroyed is notified.
Just move nsCOMPtr<nsIAppWindow> placeHolder = this; higher up, or fix all the callers to keep a strong reference?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Ah, but there isn't any security issue here
https://searchfox.org/mozilla-central/rev/557a0e222dd104c5d805ba344c45d6abc27d3db0/xpfe/appshell/AppWindow.cpp#609,624,644

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

And that WebProgressListener removal deals with WeakRefs only.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1339601, remove useless placeHolder variable, r=bgrins

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

review ping for a tiny patch

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #8)

review ping for a tiny patch

Sorry about that - responded in the patch.

Comment 10

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

https://hg.mozilla.org/integration/autoland/rev/72bed4d016bdc463884832b32a21235439ec2571
https://hg.mozilla.org/mozilla-central/rev/72bed4d016bd

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Doesn't sound like this needs to be backported, but feel free to nominate if you feel otherwise.