Closed Bug 1296942 Opened 3 years ago Closed 3 years ago

Crash in InvalidArrayIndex_CRASH mozilla::a11y::AccessibleWrap::GetXPAccessibleFor()

Categories

(Core :: Disability Access APIs, defect, critical)

51 Branch
x86
Windows 10
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- disabled
firefox49 --- disabled
firefox-esr45 --- disabled
firefox50 --- verified
firefox51 + verified

People

(Reporter: alice0775, Assigned: tbsaunde)

References

Details

(Keywords: crash, csectype-bounds, sec-high)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-feeeb195-012e-46f0-aee2-80b4c2160821.
=============================================================

I can reproduce(same operation, but on different profile) the crash.

bp-feeeb195-012e-46f0-aee2-80b4c2160821
bp-32de7ab7-8ca2-4481-8826-257072160821

Multiple click [shuffle] icon of Youtube Play List ( https://www.youtube.com/watch?v=M6mmv0sGYOM&list=PUxVlUVEiwY43qLhCB7kMBIw )
[Tracking Requested - why for this release]:
David, this looks like an accessibility crash, can you find somebody to look at this?
Flags: needinfo?(dbolter)
Depends on: 1297102
Group: dom-core-security
Component: XPCOM → Disability Access APIs
Keywords: csectype-bounds
Summary: Crash in InvalidArrayIndex_CRASH → Crash in InvalidArrayIndex_CRASH mozilla::a11y::AccessibleWrap::GetXPAccessibleFor()
The first crash in comment 0 has:
  ElementAt(aIndex = 1, aLength = 1)
The second has:
  ElementAt(aIndex = 420, aLength = 417)
Trevor agreed to take a look.
Assignee: nobody → tbsaunde+mozbugs
Flags: needinfo?(dbolter)
Comment on attachment 8783630 [details] [diff] [review]
AccessibleWrap::GetXPAccessibleFor() should check there is a child at an index before returning it

Review of attachment 8783630 [details] [diff] [review]:
-----------------------------------------------------------------

Can you add a comment about why this check is needed in the proxy case (and not the other)?
Attachment #8783630 - Flags: review?(dbolter) → review+
I'm going to mark this sec-high because it looks kind of bad. Adjust as appropriate. How far back is this underlying bug (not the actual crash here which just affects Nightly)?
Keywords: sec-high
(In reply to David Bolter [:davidb] from comment #6)
> Comment on attachment 8783630 [details] [diff] [review]
> AccessibleWrap::GetXPAccessibleFor() should check there is a child at an
> index before returning it
> 
> Review of attachment 8783630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you add a comment about why this check is needed in the proxy case (and
> not the other)?

its just a difference in what ProxyAccessible ChildAt() and Accessible ChildAt() expect about their argument.  ProxyAccessible expects it is valid, and Accessible doesn't.  Maybe ProxyAccessible should be changed, but that's a question for another day I think.
(In reply to Andrew McCreight [:mccr8] from comment #7)
> I'm going to mark this sec-high because it looks kind of bad. Adjust as
> appropriate. How far back is this underlying bug (not the actual crash here
> which just affects Nightly)?

that makes sense, its been there for several releases, but this code shouldn't be enabled by default anywhere.
Comment on attachment 8783630 [details] [diff] [review]
AccessibleWrap::GetXPAccessibleFor() should check there is a child at an index before returning it

[Security approval request comment]
How easily could an exploit be constructed based on the patch?its basically equivelent to a standardish UAF.  Any exploit would to some degree depend on some OS code running though.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?its pretty clear its an out of bounds array access.  how trigger that is perhaps less clear.

Which older supported branches are affected by this flaw?all of them though this code shouldn't be enabled by default anywhere.  especially beta or release.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?it should backport easily, and be low risk.

How likely is this patch to cause regressions; how much testing does it need?it should be low risk, it just adds a bounds check and returns null instead of a random value.
Attachment #8783630 - Flags: sec-approval?
Attachment #8783630 - Flags: sec-approval? → sec-approval+
Comment on attachment 8783630 [details] [diff] [review]
AccessibleWrap::GetXPAccessibleFor() should check there is a child at an index before returning it

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8783630 - Flags: approval-mozilla-aurora?
Attachment #8783630 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/e2a5214ebb0f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Group: dom-core-security → core-security-release
Confirming that Firefox no longer crashes using the STR from the description on Windows 10 x64 and x86 with Fx 50 RC (build ID 20161101104304) and Latest 51.0a2 Aurora (build ID 20161102004003).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: cornel.ionce
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.