838380 - WebRTC STUN crash [@nr_stun_form_error_response]

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

This crash occurred while fuzzing STUN responses.

465     res->header.type = NR_STUN_TYPE(request_method, NR_CLASS_ERROR_RESPONSE);
466     res->header.magic_cookie = req->header.magic_cookie;
467     memcpy(&res->header.id, &req->header.id, sizeof(res->header.id));
468
469     /* during development we should never see 500s (hopefully not in deployment either) */
470     assert(number != 500);


'\x01\x01\x00\\\xc5\xcbN\x1d\xc7\xbb\x9c\xf8[\x08_\x10\x13T\xc5(\x00\x01\x00\x08\x00\x01\xd4\x8eM\t\x1b\xfb\x00\x04\x00\x08\x00\x01\r\x96\x17\x15\x96y\x00\x05\x00\x08\x00\x01\r\x97\x00\x00\x00\x00\x80 \x00\x08\x00\x01\x11E\x88\xc2U\xe6\x80"\x00(Vovida.org 0.96 mozilla.org 6svc.amzn1 \x00'


Tested with m-i changeset: 120856:66efdc5f9355

Comment 1

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

Attachment: don't crash browser on server 500 error

Pretty obvious...

Comment 2

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

Comment on attachment 734081 [details] [diff] [review]
don't crash browser on server 500 error

calls with possible 500 sources are in media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c

Either if the error is 0, or if the server returned error 500.

dmose: If you don't feel comfortable reviewing this, reassign to ekr (or abr can look on monday).

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Removing asserts based on the receipt of data that could quite possible happen in the wild is a great plan.

A lot of times, though, when people write asserts like that, it's an indicator that the error-handling that follows the assert hasn't been thought out or is even known to be wrong.  I don't have time to read the code and convince myself that the error handling is sane, so I'm gonna pass back to ekr.

Comment 4

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

Comment on attachment 734081 [details] [diff] [review]
don't crash browser on server 500 error

ekr is busy this week; revectoring to abr

Comment 5

[Adam Roach [:abr]]

Comment on attachment 734081 [details] [diff] [review]
don't crash browser on server 500 error

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

r+ with the additional noted change.

::: media/mtransport/third_party/nICEr/src/stun/stun_build.c
@@ -474,5 @@
>      case 300:  str = "Try Alternate";       break;
>      case 400:  str = "Bad Request";         break;
>      case 401:  str = "Unauthorized";        break;
>      case 420:  str = "Unknown Attribute";   break;
>      case 438:  str = "Stale Nonce";         break;

Seven lines below this is an assert(0) that will fire if you get a response code that isn't in this switch statement. You'll want to remove it as well, since we don't want (e.g.) a server that returns "501" to trigger an assert either.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/db1d5109d172

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/db1d5109d172

Comment 8

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

Comment on attachment 734081 [details] [diff] [review]
don't crash browser on server 500 error

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Browser will assert/crash on a 500 server error from STUN server

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

Risk to taking this patch (and alternatives if risky): nil - removes an unwanted assert; the code handles errors already.

String or IDL/UUID changes made by this patch: none

Comment 9

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

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

Comment 10

[Scoobidiver (away)]

The m-c version where it landed is 23.

Comment 11

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

Sorry, I thought Target Milestone was supposed to reflect the earliest Milestone it's fixed in.  If not, I can go back and update all the Aurora patches I just landed.