Closed
Bug 1348424
Opened 8 years ago
Closed 8 years ago
Crash in objc_msgSend | TitlebarDrawCallback
Categories
(Core :: Widget: Cocoa, defect, P1)
Tracking
()
People
(Reporter: marcia, Assigned: mstange)
Details
(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage][adv-main54+][adv-esr52.2+] tpi:+)
Crash Data
Attachments
(2 files, 2 obsolete files)
1.11 KB,
patch
|
spohl
:
review+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
gchang
:
approval-mozilla-aurora-
gchang
:
approval-mozilla-beta+
ritu
:
approval-mozilla-esr52+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
str |
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)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8850632 -
Flags: review?(spohl.mozilla.bugs)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8850634 -
Flags: review?(spohl.mozilla.bugs)
Assignee | ||
Comment 6•8 years ago
|
||
Marcia, how long ago did this appear? Could it be a regression from bug 1212527?
Flags: needinfo?(mozillamarcia.knous)
Updated•8 years ago
|
Attachment #8850634 -
Flags: review?(spohl.mozilla.bugs) → review+
Updated•8 years ago
|
Attachment #8850632 -
Flags: review?(spohl.mozilla.bugs) → review+
Updated•8 years ago
|
Attachment #8850630 -
Flags: review?(spohl.mozilla.bugs) → review+
Reporter | ||
Comment 7•8 years ago
|
||
(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.
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(mozillamarcia.knous)
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
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.
Updated•8 years ago
|
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox-esr45:
--- → affected
status-firefox-esr52:
--- → affected
Updated•8 years ago
|
Group: core-security → layout-core-security
Assignee | ||
Comment 10•8 years ago
|
||
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
Assignee | ||
Comment 11•8 years ago
|
||
moved the "// strong" comments from the first patch into this one
Attachment #8850634 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
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?
Assignee | ||
Comment 13•8 years ago
|
||
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?
Updated•8 years ago
|
Updated•8 years ago
|
Attachment #8855960 -
Flags: sec-approval? → sec-approval+
Comment 14•8 years ago
|
||
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+
Updated•8 years ago
|
Priority: -- → P1
Whiteboard: tpi:+
Comment 15•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a382c6306623
https://hg.mozilla.org/mozilla-central/rev/0f120b51cae5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 17•8 years ago
|
||
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 18•8 years ago
|
||
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-
Comment 19•8 years ago
|
||
uplift |
Updated•8 years ago
|
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+
Comment 21•8 years ago
|
||
uplift |
Comment 22•8 years ago
|
||
(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+
Updated•8 years ago
|
Whiteboard: tpi:+ → [post-critsmash-triage]tpi:+
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage]tpi:+ → [post-critsmash-triage][adv-main54+][adv-esr52.2+] tpi:+
Comment 23•8 years ago
|
||
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)
Assignee | ||
Comment 24•8 years ago
|
||
(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)
Comment 25•8 years ago
|
||
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
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•