838319 - WebRTC STUN crash [@mbslen]

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: 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

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Someone needs to look at this (ekr/abr/jib)

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

any word on a testcase?

Comment 3

[Christoph Diehl [:posidron]]

Attachment: 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

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Christoph: If there are additional things needed to run the testcase, could you indicate what's needed here?  Thanks

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: don't assert() on bad STUN input credentials

proposed patch assuming this is just 'evil data on the wire causes assert()' (to be verified when I can run the testcase)

Comment 7

[Eric Rescorla (:ekr)]

dan, didn't you have a patch here?

Comment 8

[Christoph Diehl [:posidron]]

(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

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Good memory; I had forgotten about <https://bugzilla.mozilla.org/show_bug.cgi?id=819825>.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: don't assert() on bad STUN input credentials; accept utf8 for UTF-8

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

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).

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28b7fa5d890b

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/28b7fa5d890b

Comment 15

[Henri Sivonen (:hsivonen)]

Filed follow-up: bug 860727.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa388678f6a9