1706910 - Crash in [@ objc_msgSend | AppKit@0x424da2] - Implement accessibilityFrame

Status: VERIFIED FIXED

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

Description

[Emil Ghitta, QA [:emilghitta]]

Crash report: https://crash-stats.mozilla.org/report/index/89be788d-bdab-497d-976e-d96d70210422

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0 libobjc.A.dylib objc_msgSend 
1 AppKit AppKit@0x424da2 
2 HIServices HIServices@0x25a78 
3 HIServices HIServices@0x2f344 
4 HIServices HIServices@0x66ec 
5 CoreFoundation CoreFoundation@0x8e3f8 
6 CoreFoundation CoreFoundation@0x8e344 
7 CoreFoundation CoreFoundation@0x85eff 
8 CoreFoundation CoreFoundation@0x85152 
9 HIToolbox HIToolbox@0x2fd95 

Just encountered this crash while using macOS 10.13.6 with VoiceOver enabled (As a note: I have the "Synchronize keyboard focus and VoiceOver cursor enabed" and the mouse pointer is set to follow the VoiceOver cursor).

Affected platforms

• macOS 10.13.6

Unaffected platforms

• macOS 10.14
• macOS 10.15
• macOS 11.3

Steps to reproduce

1. Launch Firefox.
2. Access a random page.
3. Open the browser console.
4. Interact with the browser console by:
   4.1 Clicking the "Clear the Web Console output".
   4.2 Closing the browser console.
   4.3 Restart the browser via the browser console.

Please note that you may have to repeat step 4 (and it's sub steps) in order to reproduce this issue. It seems that the crash only occurs when the browser console is closed while VO has focus inside it.

Actual Result

• Firefox crashes

Expected Result

• Firefox doesn't crash :)

Regression Range

• I'll search for a regression asap.

Additional Notes

• For further information regarding this issue, access this link for screencast (Mozilla account needed).
• This is reproducible on non native context menus as well.
• Please feel free to change the component if necessary.

Comment 1

[Emil Ghitta, QA [:emilghitta]]

Hi Morgan!

I can reproduce this crash on macOS 10.12 as well. The crash occurs with different crash signatures for the same steps mentioned in comment 0.

I couldn't trigger the following signatures on macOS 10.13 (seems to be 10.12 specific):

[@ objc_msgSend | RemoveObserver ] :
bp-b82e5228-86ec-414c-8528-2c1bd0210422

[@ objc_msgSend | CopyAttributeValue ]
bp-ab17cf65-bae3-4f80-b9a9-e21720210422

[@ lookUpImpOrForward ]
bp-6895abb9-a855-4e31-bdf3-c4b950210422

How should I proceed? Should I file different bug reports for each different crash signature even if the same reproducible steps are involved?

Comment 2

[Stephen A Pohl [:spohl]]

The most helpful crash dump might be https://crash-stats.mozilla.org/report/index/6895abb9-a855-4e31-bdf3-c4b950210422 here. The app notes seem to show that we're missing an implementation for accessibilityFrame.

Comment 3

[Morgan Reschenberg [:morgan]]

(In reply to Stephen A Pohl [:spohl] from comment #2)

The most helpful crash dump might be https://crash-stats.mozilla.org/report/index/6895abb9-a855-4e31-bdf3-c4b950210422 here. The app notes seem to show that we're missing an implementation for accessibilityFrame.

Yep, this is the issue -- I'll update this bug and self-assign :)

Thanks for the tag Emil!

Comment 4

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1706910: Introduce testing for AXFrame, accessibilityFrame r?eeejay

Comment 5

[Andrew McCreight [:mccr8]]

Some of these crashes with this signature look like use-after-frees:
bp-fa140464-a1d8-4de6-84be-f7af80210422
bp-e64f93bf-e45c-46e4-88ab-093f60210422

I don't know if these are the same issue, but they also have Accessibility enabled.

Comment 6

[Andrew McCreight [:mccr8]]

Also with this signature: bp-14056105-3e5b-4f0b-8653-866750210422

Comment 7

[Emil Ghitta, QA [:emilghitta]]

Mozregression pointed out Bug 1641002 for causing this regression.

Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=81a5e4af9a33a031513d80c671e253ae82a3c774&tochange=7afbfa63f93709e41af9fe7d4f5b6f0fc408a9d9

Comment 8

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1706910: Implement AXFrame, accessibilityFrame for VoiceOver r?eeejay

Comment 9

[Morgan Reschenberg [:morgan]]

Comment on attachment 9218789 [details]
Bug 1706910: Implement AXFrame, accessibilityFrame for VoiceOver r?eeejay

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not very easily -- this patch implements a feature we didn't have before, but does not expose how the absence of that feature was previously problematic (since the crash was initiated by interaction through a third-party AT).
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all, I think
• If not all supported branches, which bug introduced the flaw?: Bug 1641002
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This patch can be applied directly -- we shouldn't need to modify it to backport it.
• How likely is this patch to cause regressions; how much testing does it need?: This patch likely won't cause regressions because we're supporting something that didn't exist before. That said, it probably needs some manual testing because its a speculative fix -- I don't have a 10.12 or 10.13 machine on which to try to reproduce this crash :(

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9217895 [details]
Bug 1706910: Introduce testing for AXFrame, accessibilityFrame r?eeejay

In this instance I think it is okay to land the test with the patch given the need for an unusual OS setting to be involved.

Comment 11

[Morgan Reschenberg [:morgan]]

(In reply to Tom Ritter [:tjr] (ni? for response to sec-[advisories/bounties/ratings/cves]) from comment #10)

Comment on attachment 9217895 [details]
Bug 1706910: Introduce testing for AXFrame, accessibilityFrame r?eeejay

In this instance I think it is okay to land the test with the patch given the need for an unusual OS setting to be involved.

Do I land these patches through lando now, or is there a special way I should submit them?

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Morgan Reschenberg [:morgan] from comment #11)

Do I land these patches through lando now, or is there a special way I should submit them?

You have sec-approval+, so you can just land them the usual way via Lando.

Comment 13

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

Implement AXFrame, accessibilityFrame for VoiceOver r=eeejay
https://hg.mozilla.org/integration/autoland/rev/67c05979577ea97a10a79f70ccadc039fcc55e4f
Introduce testing for AXFrame, accessibilityFrame r=eeejay
https://hg.mozilla.org/integration/autoland/rev/e0769fe4d6fdb51f604c6dc8d0f9336c9473db43

https://hg.mozilla.org/mozilla-central/rev/67c05979577e
https://hg.mozilla.org/mozilla-central/rev/e0769fe4d6fd

Comment 14

[Julien Cristau [:jcristau]]

Please request uplift when you get a chance.

Comment 15

[Morgan Reschenberg [:morgan]]

We're still seeing similar crashes today -- see bug 1709390
I'd like to hold off on uplift until we can verify the fix

Comment 16

[Emil Ghitta, QA [:emilghitta]]

Unfortunately I can't verify this fix until Bug 1709390 is fixed. Firefox crashes instantly when I turn VoiceOver on and move the focus over the Firefox window.

Comment 17

[Morgan Reschenberg [:morgan]]

Yeah I’m gonna request a back out for this and reinvestigate, given the new (worse) crashes. Something about this patch exacerbates UAF’s 🤔 which is odd.

Comment 18

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

Backed out on request from Morgan for the crashes in bug 1709390.

https://hg.mozilla.org/integration/autoland/rev/104f996f1a1f6f83564f5678add948a5bcd8bbb1

Comment 19

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1706910: Modify accessibilityFrame return value to conform with protocol r?eeejay

Comment 20

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

Backout merged: https://hg.mozilla.org/mozilla-central/rev/104f996f1a1f

Comment 21

[Morgan Reschenberg [:morgan]]

Reassigning to eitan since he has a broader fix to apply here :)

Comment 22

[Phabricator Automation]

Comment on attachment 9218789 [details]
Bug 1706910: Implement AXFrame, accessibilityFrame for VoiceOver r?eeejay

Revision D113585 was moved to bug 1710493. Setting attachment 9218789 [details] to obsolete.

Comment 23

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1706910 - Don't expose MOXAccessible with represented view to AppKit. r?morgan

This crash seems to be a retain/release imbalance that probably has to
do with the inconsistency of what object is returned for the root
accessible.

I was able to reproduce the crash on a 10.12 VM, and this patch seemed
to fix it.

Comment 24

[Eitan Isaacson [:eeejay]]

I realize "seemed to fix it" doesn't communicate real confidence. I'm pretty sure this fixes it.

Comment 25

[Eitan Isaacson [:eeejay]]

Comment on attachment 9221230 [details]
Bug 1706910 - Don't expose MOXAccessible with represented view to AppKit. r?morgan

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it would be very hard.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 86
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: They will be easy to create. The infrequency of this crash makes me question if it should be back ported at all.
• How likely is this patch to cause regressions; how much testing does it need?:

Comment 26

[Tom Ritter [:tjr]]

Comment on attachment 9221230 [details]
Bug 1706910 - Don't expose MOXAccessible with represented view to AppKit. r?morgan

Approved to land. I'll let Relman decide about uplift.

Comment 27

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

Don't expose MOXAccessible with represented view to AppKit. r=morgan
https://hg.mozilla.org/integration/autoland/rev/59039e0d5a3d00c14922d774f56d3215a3d41772
https://hg.mozilla.org/mozilla-central/rev/59039e0d5a3d

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:eeejay, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.
If yes, don't forget to request an uplift for the patches in the regression caused by this fix.

For more information, please visit auto_nag documentation.

Comment 29

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 90.0a1 (BuildId:20210518094531) on macOS 10.13 & macOS 10.12 the crash no longer occurs.

Comment 30

[Pascal Chevrel:pascalc]

Given that it would be very hard to exploit and that it is a rare crash, I think we can let it ride the 90 train.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 32

[Eitan Isaacson [:eeejay]]

Comment on attachment 9221230 [details]
Bug 1706910 - Don't expose MOXAccessible with represented view to AppKit. r?morgan

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Occasional crash
• Fix Landed on Version: 90
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): These are a few simple one line corrections.
• String or UUID changes made by this patch:

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9221230 [details]
Bug 1706910 - Don't expose MOXAccessible with represented view to AppKit. r?morgan

Approved for 78.12esr.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/3d3d5640bfac