Frédéric Wang (:fredw) Assignee
	Description • 9 years ago

      No description provided.

	Frédéric Wang (:fredw) Assignee
	Comment 3 • 9 years ago

This crashes when you select the third row accessible with the XCode Accessibility Inspector.

	Frédéric Wang (:fredw) Assignee
	Comment 4 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b123129dfc3f

	Carsten Book [:Tomcat]
	Comment 5 • 9 years ago

 https://hg.mozilla.org/integration/mozilla-inbound/rev/860c5decce24

	Carsten Book [:Tomcat]
	Comment 6 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/860c5decce24

	Frédéric Wang (:fredw) Assignee
	Comment 7 • 9 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.

	Frédéric Wang (:fredw) Assignee
	Comment 8 • 9 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

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 10 • 9 years ago

Comment on attachment 8667963 [details] [diff] [review]
Patch

Approved for aurora and beta uplift; crash fix, low risk

	Carsten Book [:Tomcat]
	Comment 11 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/216a745230dd

	Carsten Book [:Tomcat]
	Comment 12 • 9 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/6fcef6bbbbc5

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 13 • 9 years ago

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.