Closed Bug 838319 Opened 8 years ago Closed 8 years ago

WebRTC STUN crash [@mbslen]

Categories

(Core :: WebRTC: Networking, defect, P3)

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox21 --- disabled
firefox22 + fixed
firefox23 --- fixed

People

(Reporter: posidron, Assigned: jesup)

References

Details

(Keywords: crash, testcase, Whiteboard: [WebRTC],[blocking-webrtc+][qa-])

Crash Data

Attachments

(3 files, 1 obsolete file)

Attached file callstack
This crash occured while fuzzing STUN. Am working on building a testcase scenario for easy reproducing.


media/mtransport/third_party/nICEr/src/util/mbslen.c:115

114    if (nbytes == (size_t)-1)   /* should never happen */ {
115        assert(0);
116	   ABORT(R_INTERNAL);


Message:

'\x01\x01\xd2\\\xc5\xcb\x18\x1d\xd8\x01\xf1\x13q\x933\xa6\x8e\xb4\xa6C\x00\x01\xe5\x08\x00\x01\xe6\xb8M\t\x1b\xfb\x00\x04\x00\x08\x00\x01\r\x96\x17\x15\x96y\x88\x05\x00\x08\x00\x01\r\x97\x00\x00\x00\x00s \x00\x08\x00\x01#s\x88\xc2U\xe6\x80"\x00(Vovida.or\xe0 0.96 mozilla.org 6bvc.amzn1 \x00'

Tested with m-i changeset: 120856:66efdc5f9355
Crash Signature: [@ mbslen]
Someone needs to look at this (ekr/abr/jib)
Assignee: nobody → ekr
Whiteboard: [WebRTC],[blocking-webrtc+]
Assignee: ekr → adam
Priority: -- → P3
any word on a testcase?
Flags: needinfo?(cdiehl)
Attached file testcase
1- Run testcase.py
2- NSPR_LOG_MODULES=signaling:5 R_LOG_LEVEL=9 R_LOG_DESTINATION=stderr obj-ff64-asan-opt/dist/NightlyDebug.app/Contents/MacOS/firefox stun.html
3- Click the button
Flags: needinfo?(cdiehl)
Keywords: testcase
The testcase is incomplete:

$ python testcase.py 
Traceback (most recent call last):
  File "testcase.py", line 4, in <module>
    from twisted.internet.protocol import DatagramProtocol
ImportError: No module named twisted.internet.protocol
Assignee: adam → rjesup
Christoph: If there are additional things needed to run the testcase, could you indicate what's needed here?  Thanks
Flags: needinfo?(cdiehl)
proposed patch assuming this is just 'evil data on the wire causes assert()' (to be verified when I can run the testcase)
dan, didn't you have a patch here?
Flags: needinfo?(dmose)
(In reply to Randell Jesup [:jesup] from comment #5)
> Christoph: If there are additional things needed to run the testcase, could
> you indicate what's needed here?  Thanks

You need the twisted library.

sudo pip install Twisted
Flags: needinfo?(cdiehl)
Good memory; I had forgotten about <https://bugzilla.mozilla.org/show_bug.cgi?id=819825>.
Flags: needinfo?(dmose)
Attachment #732895 - Attachment is obsolete: true
That patch is in (and the testcase here can be run by using easy_install instead of pip).

So the problem is actually caused when you combine that with running the browser with LC_ALL=en_US.UTF-8

With a default/unset LC_ALL on my Fedora 17 machine, it doesn't fail - setlocale() returns "en_US.utf8".  Since locale affects mbrlen() via the LC_CTYPE, this is possible as we're spewing binary into a UTF-8 string, and my default locale doesn't let it actually test the string (since it searches for UTF-8, not utf8).

In this case, I'll state that my my patch which removes the asserts is reasonable - error on bad/unparsable STUN data, but don't kill the browser.  However, it wasn't actually running these checks in a lot of cases, so I've modified it to accept utf8 or utf-8 (either case) as utf8 is a common-if-incorrect (apparently) variation.
Attachment #733057 - Flags: review?(adam)
Comment on attachment 733057 [details] [diff] [review]
don't assert() on bad STUN input credentials; accept utf8 for UTF-8

Re-vectoring review to dmose per ekr's suggestion (abr is off until monday).

annevk was good with conflating UTF8 and UTF-8 (per IRC).
Attachment #733057 - Flags: review?(adam) → review?(dmose)
Attachment #733057 - Flags: review?(dmose) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/28b7fa5d890b
Whiteboard: [WebRTC],[blocking-webrtc+] → [WebRTC],[blocking-webrtc+][webrtc-uplift]
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/28b7fa5d890b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Keywords: verifyme
Keywords: verifyme
Whiteboard: [WebRTC],[blocking-webrtc+][webrtc-uplift] → [WebRTC],[blocking-webrtc+][webrtc-uplift][qa-]
Filed follow-up: bug 860727.
Comment on attachment 733057 [details] [diff] [review]
don't assert() on bad STUN input credentials; accept utf8 for UTF-8

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Certain bad input will cause a browser crash

Testing completed (on m-c, etc.): On m-c

Risk to taking this patch (and alternatives if risky): almost none.  Mostluy removing asserts, and making sure we don't bypass checks for to-long strings per the STUN spec.

String or IDL/UUID changes made by this patch: none
Attachment #733057 - Flags: approval-mozilla-aurora?
Attachment #733057 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/aa388678f6a9
Whiteboard: [WebRTC],[blocking-webrtc+][webrtc-uplift][qa-] → [WebRTC],[blocking-webrtc+][qa-]
Target Milestone: mozilla23 → mozilla22
You need to log in before you can comment on or make changes to this bug.