Closed Bug 673757 Opened 13 years ago Closed 13 years ago

Out-of-bounds array access in nsDocAccessible::CacheChildrenInSubtree

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla8
Tracking Status
firefox5 --- unaffected
firefox6 --- affected
firefox7 --- fixed
firefox8 --- fixed
status2.0 --- unaffected
status1.9.2 --- unaffected

People

(Reporter: jruderman, Assigned: surkov)

References

Details

(4 keywords, Whiteboard: [sg:critical?][qa-])

Crash Data

Attachments

(3 files, 3 obsolete files)

1. Enable accessibility, e.g. by pasting the following into the js console:

Components.classes["@mozilla.org/accessibilityService;1"]
      .getService(Components.interfaces.nsIAccessibleRetrieval);

2. Load the testcase.

Result: "ASSERTION: invalid array index" in nsTArray::ElementAt (nb: NOT SafeElementAt) followed by a crash [@ nsAccessNode::IsContent].

Both are called by nsDocAccessible::CacheChildrenInSubtree, so I assume that's closer to where the bug is.

In debug builds it usually crashes near 0x0, but in opt builds it's clearly exploitable (bp-aad90a7b-4402-40f4-bd6d-92a4d2110724).
Attached file stack traces
Attached patch patch (obsolete) — Splinter Review
the problem is the invalidation list processing alters the cached tree while its subtree is created.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #548011 - Flags: review?(trev.saunders)
Attached patch patch2 (obsolete) — Splinter Review
previous one is wrong (one of early versions)
Attachment #548011 - Attachment is obsolete: true
Attachment #548011 - Flags: review?(trev.saunders)
Attachment #548012 - Flags: review?(trev.saunders)
regression from bug 659633 which bared problem, mozilla 7 and mozilla 6 are affected
Attached patch patch3 (obsolete) — Splinter Review
updated patch summary and name
Attachment #548012 - Attachment is obsolete: true
Attachment #548012 - Flags: review?(trev.saunders)
Attachment #548026 - Flags: review?(trev.saunders)
Attached patch patch4Splinter Review
patch3 was wrong one, sorry for the spam
Attachment #548026 - Attachment is obsolete: true
Attachment #548026 - Flags: review?(trev.saunders)
Attachment #548027 - Flags: review?(trev.saunders)
Comment on attachment 548027 [details] [diff] [review]
patch4

while doing a quick debug to understand this I noticed that xul menu buttons can cause accessibles to be removed while in CacheChildren() called by  CacheChildrenInSubtree() we should  look for other cases of this behavior, but this patch clearly gets rid of the worst way of this happening, and generally makes things better so r=me
Attachment #548027 - Flags: review?(trev.saunders) → review+
Comment on attachment 548027 [details] [diff] [review]
patch4

requesting approval for aurora and beta per comment 3 saying they're effected and its possibly exploitable.    I'm not sure  how easy this would be to weaponize.  since I suspect the only control of the address isthrough masaging the heap, and even if you can control the address I'm not sure the crash is trying to run code / get a vfunc from the address.  However I'm willing to believe with a bit of work its  likely doable.  SO I think we want to ake this on aurora and beta.
Attachment #548027 - Flags: approval-mozilla-beta?
Attachment #548027 - Flags: approval-mozilla-aurora?
landed http://hg.mozilla.org/mozilla-central/rev/4f38df646524
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
OS: Linux → All
Hardware: x86_64 → All
(In reply to comment #7)
> Comment on attachment 548027 [details] [diff] [review] [review]
> patch4
> 
> while doing a quick debug to understand this I noticed that xul menu buttons
> can cause accessibles to be removed while in CacheChildren() called by 
> CacheChildrenInSubtree()

Can you give me a stack because at quick glance I don't see how it can happen?
(In reply to comment #10)
> (In reply to comment #7)
> > Comment on attachment 548027 [details] [diff] [review] [review] [review]
> > patch4
> > 
> > while doing a quick debug to understand this I noticed that xul menu buttons
> > can cause accessibles to be removed while in CacheChildren() called by 
> > CacheChildrenInSubtree()
> 
> Can you give me a stack because at quick glance I don't see how it can
> happen?

I was talking about xul/nsXULFormcontrolAccesible.cpp:214
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
OS: All → Linux
Hardware: All → x86_64
Resolution: FIXED → ---
Target Milestone: mozilla8 → ---
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago13 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(In reply to comment #11)

> I was talking about xul/nsXULFormcontrolAccesible.cpp:214

I assumed this line confuses you but my question isn't answered because this method doesn't change the tree explicitly, so that's why I asked for a stack.
(In reply to comment #12)
> (In reply to comment #11)
> 
> > I was talking about xul/nsXULFormcontrolAccesible.cpp:214
> 
> I assumed this line confuses you but my question isn't answered because this
> method doesn't change the tree explicitly, so that's why I asked for a stack.

OK
I support the approval requests for Mozilla-Aurora and Mozilla-Beta. Comment #4 gives the explanation and bug that affects this one.
Comment on attachment 548027 [details] [diff] [review]
patch4

We will not be taking this in beta as we are concerned about the risk. As this was internally found and the crashes are low we will wait for normal uplift to the beta channel
Attachment #548027 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Alexander Surkov, would it be reasonable to back out the patch that exposed this rather than taking the rather large fix here?
(In reply to comment #16)
> Alexander Surkov, would it be reasonable to back out the patch that exposed
> this rather than taking the rather large fix here?

I wouldn't back out bug 659633 because of Firefox bad reputation as a slow browser. For example, per report in bug 659633 AddBlock Plus extension makes Firefox hang for several seconds, also that bug can affect on pure Firefox in bookmarks dialog and about:config, i.e. whenever XUL tree is used and accessibility is enabled occasionally, also Thunderbird is affected.
Blocks: 659633
Keywords: regression
Whiteboard: [sg:critical] → [sg:critical?]
Comment on attachment 548027 [details] [diff] [review]
patch4

I don't think we want this kind of change landing this late in beta. We should either back out the change that opened up this crasher and security hole or we should live with it.
Attachment #548027 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
According to the reports, this crash is happening at our Mozilla Japan site.
https://crash-stats.mozilla.com/report/index/eb6fa35a-637d-4657-b25f-5a2632110819

I hope this would get fixed in Beta = Firefox 7.
Comment on attachment 548027 [details] [diff] [review]
patch4

Kohei sent email to release drivers asking us to re-consider this because it crashes at mozilla.jp and may crash at mozilla.com: 

"We've heard some users are encountering crash on the Mozilla Japan site (mozilla.jp) with Firefox 6 and came down to this bug.... As the portion of our WAI-ARIA implementation has been ported to mozilla.com, the same crash potentially occur on mozilla.com."

I'm reverting the minus back to a question mark for aurora and beta so Release Drivers will re-evaluate.
Attachment #548027 - Flags: approval-mozilla-beta?
Attachment #548027 - Flags: approval-mozilla-beta-
Attachment #548027 - Flags: approval-mozilla-aurora?
Attachment #548027 - Flags: approval-mozilla-aurora-
Comment on attachment 548027 [details] [diff] [review]
patch4

Un-asking for aurora approval since this fix is already in current aurora (FF8) -- am I correct in doing this?

Alexander, Trevor, if you can please add opinions here on potential risks taking this patch in beta.
Attachment #548027 - Flags: approval-mozilla-aurora?
(In reply to David Bolter [:davidb] from comment #22)

> Alexander, Trevor, if you can please add opinions here on potential risks
> taking this patch in beta.

It's potentially risky because it touches core stuffs (accessible tree creation/invalidation). The same time I'm sure the patch is correct. The patch was on trunk and aurora for a while and there's no known regressions caused by it. That makes me think we won't get any regressions until we release (because a11y doesn't have strong alpha/beta testing community, basically all we have is QA), so my thumb up to port it to beta to make release closer.
Attachment #548027 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Landed on mozilla-beta: http://hg.mozilla.org/releases/mozilla-beta/rev/455584906d41 (sorry I messed up a bit during qrefresh to add the approval info, accidentally omitted the bug number :( ).
Crash Signature: [@ nsDocAccessible::CacheChildrenInSubtree] [@ nsAccessNode::IsContent] → [@ nsDocAccessible::CacheChildrenInSubtree ] [@ nsAccessNode::IsContent ]
qa- as no QA fix verification needed
Whiteboard: [sg:critical?] → [sg:critical?][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.