Closed
Bug 1296942
Opened 8 years ago
Closed 8 years ago
Crash in InvalidArrayIndex_CRASH mozilla::a11y::AccessibleWrap::GetXPAccessibleFor()
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
VERIFIED
FIXED
mozilla51
People
(Reporter: alice0775, Assigned: tbsaunde)
References
Details
(Keywords: crash, csectype-bounds, sec-high)
Crash Data
Attachments
(1 file)
1.08 KB,
patch
|
davidb
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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 )
Reporter | ||
Comment 1•8 years ago
|
||
[Tracking Requested - why for this release]:
tracking-firefox51:
--- → ?
Comment 2•8 years ago
|
||
David, this looks like an accessibility crash, can you find somebody to look at this?
Flags: needinfo?(dbolter)
Updated•8 years ago
|
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()
Comment 3•8 years ago
|
||
The first crash in comment 0 has: ElementAt(aIndex = 1, aLength = 1) The second has: ElementAt(aIndex = 420, aLength = 417)
Comment 4•8 years ago
|
||
Trevor agreed to take a look.
Assignee: nobody → tbsaunde+mozbugs
Flags: needinfo?(dbolter)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8783630 -
Flags: review?(dbolter)
Comment 6•8 years ago
|
||
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+
Comment 7•8 years ago
|
||
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
Assignee | ||
Comment 8•8 years ago
|
||
(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.
Assignee | ||
Comment 9•8 years ago
|
||
(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.
Updated•8 years ago
|
Assignee | ||
Comment 10•8 years ago
|
||
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?
Updated•8 years ago
|
Attachment #8783630 -
Flags: sec-approval? → sec-approval+
Updated•8 years ago
|
Assignee | ||
Comment 11•8 years ago
|
||
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?
Updated•8 years ago
|
Attachment #8783630 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/e2a5214ebb0f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•8 years ago
|
Group: dom-core-security → core-security-release
Updated•8 years ago
|
status-firefox-esr45:
--- → disabled
Updated•8 years ago
|
Flags: qe-verify+
Comment 14•8 years ago
|
||
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
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
•