Bug 1210023 (CVE-2015-7192)

Fix accessibilityAttributeValue for NSAccessibilityIndexAttribute

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

({sec-moderate})

Trunk
mozilla44
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41- wontfix, firefox42 fixed, firefox43 fixed, firefox44 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main42+])

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

4 years ago
No description provided.
Assignee

Comment 1

4 years ago
Posted patch PatchSplinter Review
Attachment #8667963 - Flags: review?(surkov.alexander)
Assignee

Comment 2

4 years ago
Posted file Crash test (obsolete) —
Assignee

Comment 3

4 years ago
Posted file Crash test
This crashes when you select the third row accessible with the XCode Accessibility Inspector.
Attachment #8667970 - Attachment is obsolete: true
Attachment #8667963 - Flags: review?(surkov.alexander) → review+
Assignee

Updated

4 years ago
Group: core-security
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Updated

4 years ago
Depends on: 744790, 1171995
https://hg.mozilla.org/mozilla-central/rev/860c5decce24
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: core-security → core-security-release
Assignee

Comment 7

4 years ago
Comment on attachment 8667963 [details] [diff] [review]
Patch

Sorry, I believe I should have asked sec-approval before asking checking-needed... Doing it now before asking approval for old branches...

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The original code calls A->GetChildAt(i) on an accessible object A where the index i takes all values before "the index of A in its parent". This does not make sense at all and the patch fixes that by calling GetChildAt(i) on *the parent* of A instead.

A careful read of these modifications show that a read access violation may happen if the number of children of A is less than the index of A in its parent. A typical HTML table demonstrating potential crash is provided in attachment 8667978 [details]. However it is not clear to me how an exploit can be created from that information.

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?

Firefox 41 (with e10s disabled), Firefox 42 and Firefox 43.

Note that this is only likely to affect Mac users who have VoiceOver activated but according to Marco Zehe our accessibility support on Mac is too limited to be very used in practice. For some reason, before taking the patch for bug 1177640 we were actually only able to reproduce the crash with a developer tool (such as XCode Accessibility Inspector) not a user assistive technology (such as VoiceOver).

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

Bug 744790 introduced the flaw for Firefox with e10s disabled, bug 1171995 extended the flaw to Firefox with e10s enabled.

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

The changes simply apply to two "for" loops to fix read access violation and so should not be risky. The patch can just be applied as it to Firefox 42 and Firefox 43 branches. For Firefox 41, a patch can easily be created by extracting the change to the first loop.

How likely is this patch to cause regressions; how much testing does it need?

The faulty code has limited impact: It happens only when an assistive technology such as VoiceOver requests the index of a table row via the NSAccessibilityIndexAttribute value. Note that before bug 744790, HTML tables were not exposed at all to assistive technologies and even after bug 744790 the NSAccessibilityIndexAttribute value is still totally wrong. So the only thing that this patch can do besides fixing a potential crash is to make Gecko return the correct value for NSAccessibilityIndexAttribute.

Unfortunately, our current automated testing framework can not verify platform-specific accessibility code. Hence the patch should be verified by hand using the XCode Accessibility Inspector on attachment 8667978 [details]: select the accessible object corresponding to the <tr> element and open the appropriate panel to display its accessibility attributes and thus force request of NSAccessibilityIndexAttribute.
Attachment #8667963 - Flags: sec-approval?
Attachment #8667963 - Flags: sec-approval? → sec-approval+
Assignee

Comment 8

4 years ago
Comment on attachment 8667963 [details] [diff] [review]
Patch

Approval Request Comment

[Feature/regressing bug #]:

Two read access violations introduced in bug 744790 and bug 1171995.

[User impact if declined]:

Potential crashes on Mac users when the index of a row in a table is requested by an assistive technology (such as VoiceOver).

Crash can be reproduced using the XCode Accessibility Inspector on attachment 8667978 [details]: select the accessible object corresponding to the third <tr> element and open the appropriate panel to display its accessibility attributes and thus force request of NSAccessibilityIndexAttribute.

In theory the crash might be reproduced with VoiceOver too, but for some reason we were not able to reproduce it that way without also applying the patch for bug 1177640.

[Describe test coverage new/current, TreeHerder]:

No automated tests were made since our framework can not currently verify platform-specific accessibility code.

I (reporter) was able to reproduce the crash with XCode Accessibility Inspector
using attachment 8667978 [details] and verified that it is fixed by the patch.

Marco Zehe (QA) was also able to reproduce the crash with VoiceOver and the Bugzilla "attachments" table, but only when the patch for bug 1177640 is applied. Marco verified that the crash is fixed by the patch.

Read access violations happen in two different "for" loops (non-e10s and e10s respectively). Only the non-e10s case has been tested.

[Risks and why]:

Low risk, the two loops are only executed when an assistive technology such as VoiceOver requests the index of a table row via the NSAccessibilityIndexAttribute value. Note that before bug 744790, HTML tables were not exposed at all to assistive technologies and even after bug 744790 the NSAccessibilityIndexAttribute value is still totally wrong. So the only thing that this patch can do besides fixing a potential crash is to make Gecko return the correct value for NSAccessibilityIndexAttribute.

[String/UUID change made/needed]:

none
Attachment #8667963 - Flags: approval-mozilla-beta?
Attachment #8667963 - Flags: approval-mozilla-aurora?
Comment on attachment 8667963 [details] [diff] [review]
Patch

Approved for aurora and beta uplift; crash fix, low risk
Attachment #8667963 - Flags: approval-mozilla-beta?
Attachment #8667963 - Flags: approval-mozilla-beta+
Attachment #8667963 - Flags: approval-mozilla-aurora?
Attachment #8667963 - Flags: approval-mozilla-aurora+
Assignee

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [please land attachment 8667963 into aurora & beta branches]
Assignee

Updated

4 years ago
Keywords: checkin-needed
Whiteboard: [please land attachment 8667963 into aurora & beta branches]
Given that this is a sec-moderate and atm there is not dot release planned since 41.0.1 went live last week, this will be a wontfix for 41.
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+]
Alias: CVE-2015-7192
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.