Crash in DataChannels/sctp timer loop when far-end is being debugged with GDB

RESOLVED FIXED in Firefox 34

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

({crash, csectype-uaf, sec-critical})

Trunk
mozilla36
x86_64
Linux
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox34+ fixed, firefox35+ fixed, firefox36+ fixed, firefox-esr3134+ fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [webrtc-uplift][adv-main34+][adv-esr31.3+], crash signature)

Attachments

(1 attachment)

When debugging DataChannels, if one side is stalled (in gdb) for a long time, the other side may crash in a sctp-timer-processing routing accessing freed memory.

Crashes in user_sctp_timer_iterate.c:81:
if (c->c_time <= ticks)
with c = 0xa5a5a5a5a5a5a5a5

Suspicion is that in processing one timer (and it drops to lock to process the timer) it freed another item on the list.  Michael Tuexen thinks he knows the actual cause.  Several people have seen this (and I'm fairly sure I've hit it before but never got a useful stack from it).

sec-crit because it uses the freed-memory-pointer to load a function pointer and call it.

Likely affects multiple releases as we haven't changed the SCTP library recently.
For the record, CR with FF34:
https://crash-stats.mozilla.com/report/index/e6def404-4f1b-43bc-b4d0-301192141008
Crash Signature: [@ user_sctp_timer_iterate ]
Assignee: rjesup → tuexen
No longer depends on: 1079729
See Also: → 1079729
This code hasn't changed pretty much since initial landing.
There's a fix in upstream (likely should update the library on m-c), and I can cherry-pick the fix for uplifts.
(In reply to Randell Jesup [:jesup] from comment #3)
> There's a fix in upstream (likely should update the library on m-c), and I
> can cherry-pick the fix for uplifts.

Has this upstream fix been uplifted to our repo yet?  We're nearing the end of a cycle so that would be good to have.
Flags: needinfo?(rjesup)
Michael, any updates on this before our cycle ends?
Flags: needinfo?(tuexen)
Not sure what you are asking for. I fixed the bug upstream. Integrating that fix
is normally done by Randell.
Flags: needinfo?(tuexen)
Al: sorry, I wanted to build a more-upliftable patch than a full library import.  I started working on that again last night when I realized it had fallen off the radar.  I hope to land one today (and import a new lib to 36)
Flags: needinfo?(rjesup)
Michael, the bug is assigned to you, which is why I was asking you the question. We saw this during critsmash and I assumed the assignee was the person to ask for status.

Randell, thanks.
Ahh, OK. So maybe the assignment is suboptimal...
Assignee: tuexen → rjesup
Comment on attachment 8521747 [details] [diff] [review]
update iteration code from upstream

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

imported rev 9038 from upstream, updated our moz.build file to match and remove the user_sctp_timer_iterate.c
Imported as a single patch, it applies cleanly
Attachment #8521747 - Flags: review?(tuexen)
Comment on attachment 8521747 [details] [diff] [review]
update iteration code from upstream

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very unlikely

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? Pretty much all

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?  Should apply directly

How likely is this patch to cause regressions; how much testing does it need? fairly unlikely to regress, as this makes it use the existing Mac kernel timer loop, and this upstream patch is basically a clean import.

I've done local testing where I freeze one side using the testcase in See Also here and two browsers.  no problems in multiple tests.
Attachment #8521747 - Flags: sec-approval?
Attachment #8521747 - Flags: approval-mozilla-esr31?
Attachment #8521747 - Flags: approval-mozilla-beta?
Attachment #8521747 - Flags: approval-mozilla-aurora?
Comment on attachment 8521747 [details] [diff] [review]
update iteration code from upstream

Many approvals given! Go!
Attachment #8521747 - Flags: sec-approval?
Attachment #8521747 - Flags: sec-approval+
Attachment #8521747 - Flags: approval-mozilla-esr31?
Attachment #8521747 - Flags: approval-mozilla-esr31+
Attachment #8521747 - Flags: approval-mozilla-beta?
Attachment #8521747 - Flags: approval-mozilla-beta+
Attachment #8521747 - Flags: approval-mozilla-aurora?
Attachment #8521747 - Flags: approval-mozilla-aurora+
Actually: Tuexen wrote the upstream patch; I imported it, so I really should be the reviewer.
Attachment #8521747 - Flags: review?(tuexen) → review+
https://hg.mozilla.org/mozilla-central/rev/153148514f22
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Duplicate of this bug: 1099388
Whiteboard: [webrtc-uplift] → [webrtc-uplift][adv-main34+][adv-esr31.3+]
Going to mark this as qe‑verify- as this was tested via bug # 1099388 comment # 7. I tried reproducing the issue with the test case from bug # 1099388 using the instructions from bug # 1099388 comment # 4 without any luck. I let the test case run for more than 2 hours on my asan build without any crashes.

Using the information from comment #0, I wasn't sure how to proceed with testing as the issue was discovered while debugging DataChannels.

If there's anything else that QE can do here, please let me know and I'll gladly take another look!
Flags: qe-verify-
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.