1261752 - hold strong pointers to view managers and widgets and/or check for destruction in various places

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: part 1

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: part 2

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8737704 [details] [diff] [review]
part 2

I think it would be better to move the strong ref out one level.
ProcessPendingUpdatesForView is a private internal method and only called
in two places.  In ProcessPendingUpdates(), I'd like the strong ref to cover
the CallWillPaintOnObservers() call too since using 'mRootView' after it
looks a bit suspicious.

The comment change here makes sense though.

Comment 4

[Daniel Veditz [:dveditz]]

It looks like you're protecting against a UAF. Any evidence we can get these or code-cleanup just in case? Any way we can encourage cases like this to trigger during fuzzing?

If this is fixing a vulnerability it looks like we need it on all the branches based on a quick glance at the patched code.

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to Daniel Veditz [:dveditz] from comment #4)
> It looks like you're protecting against a UAF.

Part 1 does.

The code in part 2 is already safe against UAF, but it protects in the unlikely event that the object is destroy and recreated at the same address before the function continues. The re-created object might have different state which could confuse things. I don't have any specific scenario in mind for this. We should just avoid it.

> Any evidence we can get these or code-cleanup just in case?

No evidence, found by inspection. So "just in case" would be correct.

> Any way we can encourage cases like this to trigger during fuzzing?

For part 1 we'd need to have script resize a document (iframe) and then have the refresh driver tick, which flushes everything. Somehow the resize would still need to be pending despite the flush (or the flush would have to generate an unhandled resize?), and then while processing the resize we'd need to have a script (ie nsContentUtils::AddScriptRunner or something like that), or pending changes that somehow didn't get flushed or handled when the refresh driver flushed. That that script or pending change would need to tear down the iframe.

> If this is fixing a vulnerability it looks like we need it on all the
> branches based on a quick glance at the patched code.

The code hasn't been changed in a while, so yes, it exists on all supported branches.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: part 2

Good idea. Updated patch to comments.

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8737976 [details] [diff] [review]
part 2

Thanks.  Can you also add a "// might destroy us" comment on this line:
http://hg.mozilla.org/mozilla-central/annotate/9bd900888753/layout/forms/nsComboboxControlFrame.cpp#l1484
and do an early return immediately after it, like so:
if (!weakFrame.IsAlive()) {
  return consume;
}

I think we also need strong ref on |this| at the start of WillPaintWindow:
http://hg.mozilla.org/mozilla-central/annotate/9bd900888753/view/nsViewManager.cpp#l694
given that it calls ProcessPendingUpdates() and later uses mPresShell.

(the rest of the call sites for these two methods looked fine, but feel
free to double-check)

r=mats with those two additions

Comment 8

[Mats Palmgren (inactive)]

I suspect that we have some cleanup to do in the widget layer as well, for example:
http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp#112
which I think ends up here:
http://hg.mozilla.org/mozilla-central/annotate/9bd900888753/view/nsView.cpp#l1057
but the widget code seems to be unaware that this call is destructive...

Comment 9

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #7)
> I think we also need strong ref on |this| at the start of WillPaintWindow:

Actually, I see now that nsViewManager::WillPaintWindow is private, so the call
from nsView (in comment 8) should be the only one so I think it's probably fine
to trust nsView holding a ref there.

My comment on the widget code still stands though.

Comment 10

[Timothy Nikkel (:tnikkel)]

Auditing all the other callers of those methods was next on my todo list, so thanks for doing that.

Did you do a full check of all the widget callers? It wasn't quite clear from your comment.

Comment 11

[Mats Palmgren (inactive)]

Yeah, I checked all callers of those two methods and the call sites looked OK, with one
exception noted in nsComboboxControlFrame.cpp.  I think all call sites were already
aware of possible destruction so probably no need to check the callers of those
recursively, except for nsView::WillPaintWindow where at least some calls to that
method from widget code looks unsafe (widget/gonk/nsWindow.cpp#112 as an example).

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: part 3

I did the further changes you suggested, along with the results of my audit, and put it in a new part, part 3.

The objective C autorelease thing is new to me, so not sure I'm doing it right, but it seems right.

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8739849 [details] [diff] [review]
part 3

>view/nsViewManager.cpp
>+    LayerManager *manager = widget->GetLayerManager();

nit: swap the * and space, while we're here

>widget/cocoa/nsChildView.mm

See the long comment here:
http://hg.mozilla.org/mozilla-central/annotate/e847cfcb315f/widget/cocoa/nsChildView.mm#l3979

It says mGeckoChild holds a strong ref to "us" which I assume
means 'self' in this context.  And the code explicitly NS_ADDREF
mGeckoChild (and its ancestors) for the duration of WillPaintWindow(),
assuming [self performSelector:... afterDelay:0] means releaseWidgets
will run async (doing the corresponding NS_RELEASE calls).

So this file looks OK to me and we shouldn't change it.

>widget/gonk/nsWindow.cpp

>    LayerManager* lm = targetWindow->GetLayerManager();
>    if (mozilla::layers::LayersBackend::LAYERS_CLIENT == lm->GetBackendType()) {

It seems nsWindow::GetLayerManager tries to create a new LayerManager
if mLayerManager is null, which seems wrong if stuff we're destroyed.
GetLayerManager() is also using some other members, e.g. mScreen,
which perhaps isn't a good idea (we call mScreen->UnregisterWindow(this)
in nsWindow::Destroy for example).

It seems more robust to avoid calling GetLayerManager() if WillPaintWindow
destroyed the view, so I think we should move this LayerManager stuff
inside the "if (listener) {" below, before the DidPaintWindow call.

>widget/gtk/nsWindow.cpp

I think we have the same problem with the PaintWindow() calls later:
http://hg.mozilla.org/mozilla-central/annotate/e847cfcb315f/widget/gtk/nsWindow.cpp#l2226
http://hg.mozilla.org/mozilla-central/annotate/e847cfcb315f/widget/gtk/nsWindow.cpp#l2281
I think we need to add:
  listener = mAttachedWidgetListener ? mAttachedWidgetListener 
                                     : mWidgetListener;
  if (!listener) {
      return FALSE;
  }

after each PaintWindow call, because 'listener' (and in the latter
case 'dt', 'ctx', etc) might be invalid.


BTW, perhaps we should add a nsBaseWidget convenience method for
getting the relevant WidgetListener?

r- because I think we should also fix nsWindow::OnPaint() in
widget/qt/nsWindow.cpp while we're here.  Other than that it
looks good, with the nits above fixed.

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #13)
> >widget/cocoa/nsChildView.mm
> 
> See the long comment here:
> http://hg.mozilla.org/mozilla-central/annotate/e847cfcb315f/widget/cocoa/
> nsChildView.mm#l3979
> 
> It says mGeckoChild holds a strong ref to "us" which I assume
> means 'self' in this context.  And the code explicitly NS_ADDREF
> mGeckoChild (and its ancestors) for the duration of WillPaintWindow(),
> assuming [self performSelector:... afterDelay:0] means releaseWidgets
> will run async (doing the corresponding NS_RELEASE calls).
> 
> So this file looks OK to me and we shouldn't change it.

Hmm, thanks for finding that. The scenario that I was thinking of was that mGeckoChild stays alive but has Destroy() called on it, which calls nsChildView::TearDownView(). This seems to have two possibilities for how it handles destruction of mView

http://hg.mozilla.org/mozilla-central/annotate/e847cfcb315f/widget/cocoa/nsChildView.mm#l589

The easy case is that it is async released, which is safe for us. In the other case, the comment says, an NSWindow owns mView, so we just release mView and let the owning NSWindow handle destroying mView. So this leaves the question: when does the NSWindow destroy mView? Can it happen from the WillPaintWindow call in viewWillDraw? I don't know enough to know the answer to this.

Comment 15

[Mats Palmgren (inactive)]

I think it may be synchronously deallocated in TearDownView().
OK, so let's add this instead of the line you have in the patch:
  nsAutoRetainCocoaObject kungFuDeathGrip(self);

I don't think your line works correctly since it has no corresponding 'retain'.
But since we have already have a mechanism for this, let's use that.

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #15)
>   nsAutoRetainCocoaObject kungFuDeathGrip(self);

Ah! That's what I was looking for.

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #13)
> >widget/gtk/nsWindow.cpp
> 
> I think we have the same problem with the PaintWindow() calls later:
> http://hg.mozilla.org/mozilla-central/annotate/e847cfcb315f/widget/gtk/
> nsWindow.cpp#l2226
> http://hg.mozilla.org/mozilla-central/annotate/e847cfcb315f/widget/gtk/
> nsWindow.cpp#l2281
> I think we need to add:
>   listener = mAttachedWidgetListener ? mAttachedWidgetListener 
>                                      : mWidgetListener;
>   if (!listener) {
>       return FALSE;
>   }
> 
> after each PaintWindow call, because 'listener' (and in the latter
> case 'dt', 'ctx', etc) might be invalid.

dt and ctx are both RefPtrs so they can't die, but resources they might need could go away. Hopefully they are resilient to this because even if the widget listener stays alive there are other ways their resources could go away.

> BTW, perhaps we should add a nsBaseWidget convenience method for
> getting the relevant WidgetListener?

I looked into this but PuppetWidget has it's own unique way of getting a listener

http://mxr.mozilla.org/mozilla-central/source/widget/PuppetWidget.cpp?rev=42154de581f7#1385

so I just settled for making a helper for each widget backend that I touched (to avoid this bug going too far away from it's main purpose).

Comment 18

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #13)
> r- because I think we should also fix nsWindow::OnPaint() in
> widget/qt/nsWindow.cpp while we're here.

Don't know how I missed that.

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: part 3

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 8740152 [details] [diff] [review]
part 3

Thanks!  r=mats

> widget/gtk/nsWindow.h
>     nsIWidgetListener* GetListener();

nit: please use the same name here as in widget/windows/ and widget/qt/
(i.e. GetPaintListener)

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #20)
> Comment on attachment 8740152 [details] [diff] [review]
> part 3
> 
> Thanks!  r=mats

Thanks for your careful review!

> > widget/gtk/nsWindow.h
> >     nsIWidgetListener* GetListener();
> 
> nit: please use the same name here as in widget/windows/ and widget/qt/
> (i.e. GetPaintListener)

I called it just "GetListener" because it is used in one place that has nothing to do with painting. But I can change it to GetPaintListener to, I don't feel strongly.

Comment 22

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8737703 [details] [diff] [review]
part 1

I'm doing one sec-approval for all three patches.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
hard to say, these were found by inspection. not easy though.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. They give some leads where one might start looking though.

Which older supported branches are affected by this flaw?
Likely all of them.

If not all supported branches, which bug introduced the flaw?
See above.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be very easy to do.

How likely is this patch to cause regressions; how much testing does it need?
It really shouldn't cause regressions.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

This is too late in the current cycle to take. We're building final builds next week and this would have to go into 46 with a beta or two still to go.

I'm giving sec-approval but not for checkin before May 10. (This is what happens late in the cycle.)

Comment 24

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34fe8a381ee93b850e8d7c0f64dd9a9eaa0de446
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed485cc76a622813190f3da12d27f79032bdbade
https://hg.mozilla.org/integration/mozilla-inbound/rev/433c854c4bb5d5ee4820f4622d16b7e9840ccab3

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34fe8a381ee9
https://hg.mozilla.org/mozilla-central/rev/ed485cc76a62
https://hg.mozilla.org/mozilla-central/rev/433c854c4bb5

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Tim, could you please nominate patches for uplift to Aurora, Beta, ESR45? Thanks.

Comment 27

[Timothy Nikkel (:tnikkel)]

Attachment: part3 for aurora

Comment 28

[Timothy Nikkel (:tnikkel)]

Attachment: part3 for beta

Comment 29

[Timothy Nikkel (:tnikkel)]

Attachment: part3 for esr45

Comment 30

[Timothy Nikkel (:tnikkel)]

part 1 and 2 apply fine all the way back to esr45, part3 needs fixups for each one. I'll just provide them here in case the sheriffs have problems uplifting.

Comment 31

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8737703 [details] [diff] [review]
part 1

This approval request is for all three patches.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: potential security issue and/or crashes
Fix Landed on Version: 49
Risk to taking this patch (and alternatives if risky): the patch is adding holding references and checking if things are alive, so is low risk, and only affects situations where the bad thing would be happening (use after free)
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: the patches fix many problems, I could go and look when the problem was introduced for each one, but we know that at least one of the problems is ancient and affects all supported branches
[User impact if declined]: potential security issue and/or crashes
[Describe test coverage new/current, TreeHerder]: nothing specific for this. this is pretty fundamental code, so it's exercised when running pretty much any tests of the browser.
[Risks and why]: the patch is adding holding references and checking if things are alive, so is low risk, and only affects situations where the bad thing would be happening (use after free)
[String/UUID change made/needed]: none

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8737703 [details] [diff] [review]
part 1

Sec-high, approved for uplift to all affected branches.

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ca2f65b2d7b3
https://hg.mozilla.org/releases/mozilla-aurora/rev/30b2b2408007
https://hg.mozilla.org/releases/mozilla-aurora/rev/65b8174889df

https://hg.mozilla.org/releases/mozilla-beta/rev/00774a92ff12
https://hg.mozilla.org/releases/mozilla-beta/rev/9910c3c3639f
https://hg.mozilla.org/releases/mozilla-beta/rev/2dfed599855f


ESR45's closed for bustage, so didn't uplift this there yet.

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/380ddd689680
https://hg.mozilla.org/releases/mozilla-esr45/rev/73cc9a2d8fc1
https://hg.mozilla.org/releases/mozilla-esr45/rev/3c2bd9158ad3