1348424 - Crash in objc_msgSend | TitlebarDrawCallback

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-656f02c9-25a7-40b6-bcce-656632170316.
=============================================================

Filing because I saw this in nightly, and it appears to be meet the criteria of a UAF.

This crash is on Nightly, but there are a few other crashes on 52 that show the same crash address: http://bit.ly/2mBvhc0

Comment 1

[Andrew McCreight [:mccr8]]

Any ideas, Markus?

Comment 2

[Markus Stange [:mstange]]

I have a few ideas, but nothing that I'm 100% sure about.

We're trying to paint the title bar of a window, and crashing when we access the contents of the NSWindow pointed to by the mWindow field of the TitlebarAndBackgroundColor class. So either the mWindow pointer is stale, or the TitlebarAndBackgroundColor object has been destroyed so we'd accessing an mWindow pointer that's garbage.

Interestingly, these crashes only occur on 10.11.

We have a workaround for a 10.11+ bug here: http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/widget/cocoa/nsChildView.mm#2963
This code touches the TitlebarAndBackgroundColor class, and it only runs on 10.11 and 10.12, so it's possible that it's involved here. I noticed that the AutoBackgroundSetter doesn't keep a reference to mWindow, so the window might conceivably be destroyed in the middle, and then it's calling restoreBackgroundColor on a destroyed window. Also, -[ToolbarWindow restoreBackgroundColor] restores the wrong color object (mBackgroundColor instead of mColor).

The other thing that I've noticed is that in -[ToolbarWindow dealloc] we release a reference to our TitlebarAndBackgroundColor instance but we don't call [super setBackgroundColor:] in order to drop other pointers to this instance. But on the other hand, we shouldn't need to, because setBackgroundColor will have added its own reference, so the TitlebarAndBackgroundColor object should be kept alive until the actual NSWindow destructor releases its background color member.

Comment 3

[Markus Stange [:mstange]]

Attachment: 6-Bug_1348424___Restore_the_right_background_color__r_spohl.diff

Unrelated to the crash, but touches the same code.

Steps to reproduce:
 1. Enable the titlebar for the browser window by clicking the hamburger button, choosing Customize, and clicking the Title Bar button in the bottom left corner.
 2. Go to a regular website with text.
 3. Use three-finger-tap on your touchpad in order to show the dictionary.

This makes the titlebar turn light gray.

Comment 4

[Markus Stange [:mstange]]

Attachment: 7-Bug_1348424___Grab_a_reference_to_the_window_when_swapping_out_its_background_color__r_spohl.diff

Comment 5

[Markus Stange [:mstange]]

Attachment: 8-Bug_1348424___Drop_the_window_s_reference_to_mColor_before_releasing_it__r_spohl.diff

Comment 6

[Markus Stange [:mstange]]

Marcia, how long ago did this appear? Could it be a regression from bug 1212527?

Comment 7

[Marcia Knous [:marcia]]

(In reply to Markus Stange [:mstange] from comment #6)
> Marcia, how long ago did this appear? Could it be a regression from bug
> 1212527?

Here is a years worth of crashes: http://bit.ly/2nsCBe5

It looks as if the earliest crash in this signature was in 24.2.0esr back in 2013 (20131205180928).

After that there are more crashes starting in 2016 with 29.0 (although some don't appear to have the UAF crash address).

So nothing I can see maps to the landing of Bug 1212527 which appears to be around the end of May when Nightly was in 49.

Comment 8

[Markus Stange [:mstange]]

Thanks! In that case, the only explanation I can come up with is that this is a bug in 10.11, which was released in September 2015.
I'll try landing my patches anyway since they make us more correct and might just workaround the bug.

Comment 9

[Markus Stange [:mstange]]

I'm going to move these patches to a new non-security bug because I have no confidence that they're going to fix this one, and because they don't look very security sensitive.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8850630 [details] [diff] [review]
6-Bug_1348424___Restore_the_right_background_color__r_spohl.diff

I've moved this patch to bug 1354715.

Comment 11

[Markus Stange [:mstange]]

Attachment: 6-Bug_1348424___Drop_the_window_s_reference_to_mColor_before_releasing_it__r_spohl.diff

moved the "// strong" comments from the first patch into this one

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8850632 [details] [diff] [review]
7-Bug_1348424___Grab_a_reference_to_the_window_when_swapping_out_its_background_color__r_spohl.diff

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily. I don't know how to trigger this bug and it's Mac OS 10.11-only. These patches are more of a guess, and they're the right thing to do, but it's unclear whether they're going to fix these crashes.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I don't think so.

Which older supported branches are affected by this flaw?
All of them, these crashes go back a long time, all the way to when 10.11 was first released.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
These patches should apply as-is.

How likely is this patch to cause regressions; how much testing does it need?
Extremely unlikely to cause regressions, shouldn't need any testing.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8855960 [details] [diff] [review]
6-Bug_1348424___Drop_the_window_s_reference_to_mColor_before_releasing_it__r_spohl.diff

see comment 12

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 8850632 [details] [diff] [review]
7-Bug_1348424___Grab_a_reference_to_the_window_when_swapping_out_its_background_color__r_spohl.diff

sec-approval=dveditz

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a382c6306623
https://hg.mozilla.org/mozilla-central/rev/0f120b51cae5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this ready for Beta/ESR52 approval requests?

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8855960 [details] [diff] [review]
6-Bug_1348424___Drop_the_window_s_reference_to_mColor_before_releasing_it__r_spohl.diff

I'm requesting approval for both patches. It's unclear whether they fix the crashes, but they shouldn't hurt either.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: crashes on 10.11, sometimes
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none

Approval Request Comment
[Feature/Bug causing the regression]: unknown, possibly 10.11 itself
[User impact if declined]: crashes on 10.11, sometimes
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple and small patches
[String changes made/needed]: none

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8855960 [details] [diff] [review]
6-Bug_1348424___Drop_the_window_s_reference_to_mColor_before_releasing_it__r_spohl.diff

Fix a sec-high. Beta54+. Should be in 54 beta 3. There is no aurora now.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8619db383886

Comment 20

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

Comment on attachment 8855960 [details] [diff] [review]
6-Bug_1348424___Drop_the_window_s_reference_to_mColor_before_releasing_it__r_spohl.diff

Sec-high, let's uplift to ESR52.2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/21bcd10f1a10

https://hg.mozilla.org/releases/mozilla-esr52/rev/6472c7006a73
https://hg.mozilla.org/releases/mozilla-esr52/rev/0d5a26b29816

Comment 22

[Andrei Vaida [:avaida]]

(In reply to Markus Stange [:mstange] from comment #17)
> [User impact if declined]: crashes on 10.11, sometimes
> [Is this code covered by automated tests?]: no
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: no

Based on described user impact and the lack of automated coverage, I think we should at least try to manually verify this fix.

Comment 23

[Petruta Horea [:phorea], Desktop Test Engineering]

I couldn't reproduce the crash on Mac OS X 10.11 retina, but the issue as described in comment 3 still occurs on Firefox 54.0 RC and latest Nightly 55.0a1.

According to crash stats, there are no new crashes after the fix landed, but considering 53.0.3 is the most affected build, we should wait for 54 to be released and re-check afterwards.

Markus, any thoughts? Thank you!

Comment 24

[Markus Stange [:mstange]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #23)
> I couldn't reproduce the crash on Mac OS X 10.11 retina,

Yeah, me neither. I'm not very confident that these patches actually fix the crash.

> but the issue as
> described in comment 3 still occurs on Firefox 54.0 RC and latest Nightly
> 55.0a1.

The issue from comment 3 has been pulled out into a separate bug, bug 1354715. And that bug unfortunately got backed out.

> According to crash stats, there are no new crashes after the fix landed, but
> considering 53.0.3 is the most affected build, we should wait for 54 to be
> released and re-check afterwards.

I agree. Thanks!

Comment 25

[Petruta Horea [:phorea], Desktop Test Engineering]

There are still a few crashes with this signature on Firefox 54, 54.0.1 and 54.0.3:

Eg: https://crash-stats.mozilla.com/report/index/4cf245a7-44ba-4484-b7ea-1b59c0170705
https://crash-stats.mozilla.com/report/index/5c45a553-ece4-44fd-8c5e-34d660170704

https://crash-stats.mozilla.com/report/index/c1565acd-0bd1-4786-a7eb-9d26a0170704

https://crash-stats.mozilla.com/report/index/a60fc549-ddd0-48b1-b823-c18c30170703