Crash in objc_msgSend | TitlebarDrawCallback

RESOLVED FIXED in Firefox -esr52

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: marcia, Assigned: mstange)

Tracking

({crash, csectype-uaf, sec-high})

Trunk
mozilla55
Unspecified
macOS
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox52 wontfix, firefox-esr5254+ fixed, firefox53 wontfix, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main54+][adv-esr52.2+] tpi:+, crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

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
Any ideas, Markus?
Flags: needinfo?(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.
Flags: needinfo?(mstange)
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.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #8850630 - Flags: review?(spohl.mozilla.bugs)
Marcia, how long ago did this appear? Could it be a regression from bug 1212527?
Flags: needinfo?(mozillamarcia.knous)
Attachment #8850634 - Flags: review?(spohl.mozilla.bugs) → review+
Attachment #8850632 - Flags: review?(spohl.mozilla.bugs) → review+
Attachment #8850630 - Flags: review?(spohl.mozilla.bugs) → review+
(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.
Flags: needinfo?(mozillamarcia.knous)
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.
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.
Group: core-security → layout-core-security
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.
Attachment #8850630 - Attachment is obsolete: true
moved the "// strong" comments from the first patch into this one
Attachment #8850634 - Attachment is obsolete: true
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.
Attachment #8850632 - Flags: sec-approval?
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
Attachment #8855960 - Flags: sec-approval?
Attachment #8855960 - Flags: sec-approval? → sec-approval+
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
Attachment #8850632 - Flags: sec-approval? → sec-approval+
Priority: -- → P1
Whiteboard: tpi:+
https://hg.mozilla.org/mozilla-central/rev/a382c6306623
https://hg.mozilla.org/mozilla-central/rev/0f120b51cae5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Is this ready for Beta/ESR52 approval requests?
Flags: needinfo?(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
Flags: needinfo?(mstange)
Attachment #8855960 - Flags: approval-mozilla-esr52?
Attachment #8855960 - Flags: approval-mozilla-beta?
Attachment #8855960 - Flags: approval-mozilla-aurora?
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.
Attachment #8855960 - Flags: approval-mozilla-beta?
Attachment #8855960 - Flags: approval-mozilla-beta+
Attachment #8855960 - Flags: approval-mozilla-aurora?
Attachment #8855960 - Flags: approval-mozilla-aurora-
Group: layout-core-security → core-security-release
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
Attachment #8855960 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
(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.
Flags: qe-verify+
Whiteboard: tpi:+ → [post-critsmash-triage]tpi:+
Whiteboard: [post-critsmash-triage]tpi:+ → [post-critsmash-triage][adv-main54+][adv-esr52.2+] tpi:+
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!
Flags: needinfo?(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!
Flags: needinfo?(mstange)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.