Closed Bug 1185157 Opened 5 years ago Closed 5 years ago

Assertion failures in accessibility IPC

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- disabled
firefox40 --- disabled
firefox42 --- fixed
b2g-master --- fixed

People

(Reporter: mccr8, Assigned: tbsaunde)

References

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [e10s-only][post-critsmash-triage][adv-main42+])

Attachments

(1 file)

This is a fairly common IPC assertion crash on Nightly. We're inside mozilla::layout::PVsyncChild::OnMessageReceived(), which calls nsRefreshDriver::Tick(), which calls mozilla::a11y::NotificationController::WillRefresh(), which then calls mozilla::a11y::PDocAccessibleChild::SendTextChangeEvent() or SendHideEvent(), which tries to send a message. This is on the main thread in the content process. I'm not sure if the issue is the nested IPC or whether a11y IPC is supposed to run no another thread or what.

Here are some example crash reports:
https://crash-stats.mozilla.com/report/index/9aef00e4-cefb-476a-8cc3-525fd2150717
https://crash-stats.mozilla.com/report/index/74205d66-570b-49bf-84a0-8a9a72150717
https://crash-stats.mozilla.com/report/index/46415e7d-cede-4614-a630-d34b02150716
https://crash-stats.mozilla.com/report/index/30a44a30-bb8b-479f-89c1-f05bf2150716
Summary: IPC assertions in → Assertion failures in accessibility IPC
So, my understanding is that it assumed WillRefresh will call js, so I believe it *should* be safe for a11y to do this.

In the last one of those crashes we crashed on the line calling AssertWorkerThread() so my guess would be some how part of the ipc code is running on the wrong thread (or at least thinks it is).

I'm not that familiar with ipc code maybe Billm has ideas?
Flags: needinfo?(wmccloskey)
I think that the IPC channel for ipcDoc is probably dead at this point, so the thread assertion is probably a red herring. DocAccessibleChild (and DocAccessibleParent as well) need to override the ActorDestroy method and ensure that no IPC messages are sent after that point (using a flag or something). IPDL requires this.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #2)
> I think that the IPC channel for ipcDoc is probably dead at this point, so
> the thread assertion is probably a red herring. DocAccessibleChild (and
> DocAccessibleParent as well) need to override the ActorDestroy method and

DocAccessibleParent does.

> ensure that no IPC messages are sent after that point (using a flag or
> something). IPDL requires this.

I'm not really sure how this happens in normal operation, but its easy enough to deal with.  That is I could see this happening when we kill a child process, but otherwise I would expect that no events are fired on a DocAccessible after the point where we shut down its actor, but I might be wrong about that.
Bill: does the fatal assert save us from any bad consequences, or might there be other paths that would slip through with a reference to a dead object?
Flags: needinfo?(wmccloskey)
The assertion helps, but I don't think it's a guarantee against exploitability. A hacker would just have to set the worker ID to the correct value, which is probably very easy to guess.
Flags: needinfo?(wmccloskey)
Comment on attachment 8637437 [details] [diff] [review]
make sure we don't send an event to a destroyed ipc document

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

I'm not comfortable enough with IPDL workings to catch anything wrong with this.
Attachment #8637437 - Flags: review?(lorien) → review?(wmccloskey)
Attachment #8637437 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/c966f178765a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee: nobody → tbsaunde+mozbugs
How exploitable do you think this will be? Is this just during shutdown when we're not running any user code or can it happen at other times? Should we backport this to other branches?
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Andrew McCreight [:mccr8] from comment #9)
> How exploitable do you think this will be? Is this just during shutdown when
> we're not running any user code or can it happen at other times? Should we
> backport this to other branches?

It only matters with e10s, and we have / will disable a11y with e10s on all existing release branches so I don't see any point in backporting.

How exploitable is it? I'm not really sure, I suspect it only happens when tabs are closed so it may well be no content js is running, and can't force this to happen but I don't really know.
Flags: needinfo?(tbsaunde+mozbugs)
Thanks of the explanation.
Whiteboard: [e10s-only]
Err... thanks for the explanation.
Group: core-security → core-security-release
Whiteboard: [e10s-only] → [e10s-only][post-critsmash-triage]
Whiteboard: [e10s-only][post-critsmash-triage] → [e10s-only][post-critsmash-triage][adv-main42+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.