790929 - WebRTC crash [@sdp_build_attr_from_str]

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

The console shows the following before the crash happens:

!!! Real PeerConnection constructor called OMG !!!
!!! {94dd7f47-e489-a648-a1d0-a9317f8d4bc0} : calling initialize
!!! Queue for {94dd7f47-e489-a648-a1d0-a9317f8d4bc0} is currently: []
!!! Queue for {94dd7f47-e489-a648-a1d0-a9317f8d4bc0} is currently: []
!!! mozPeerConnection constructor called [object Window @ 0x1544f2980 (native @ 0x14e633d00)]
Delivering PeerConnection ICE callback 
!!! {94dd7f47-e489-a648-a1d0-a9317f8d4bc0} : onStateChange called: 2
!!! ICE waiting...
!!! in executeNext: !!! Queue for {94dd7f47-e489-a648-a1d0-a9317f8d4bc0} is currently: []
!!! {94dd7f47-e489-a648-a1d0-a9317f8d4bc0} : createOffer called
!!! {94dd7f47-e489-a648-a1d0-a9317f8d4bc0} : calling createOffer
!!! Queue for {94dd7f47-e489-a648-a1d0-a9317f8d4bc0} is currently: []
!!! {94dd7f47-e489-a648-a1d0-a9317f8d4bc0} : createOffer returned
!!! {94dd7f47-e489-a648-a1d0-a9317f8d4bc0} : onCreateOfferSuccess called

Click "Crash me" in the testcase to reproduce with a ASan enabled Alder build.

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 2

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

Attachment: Error out on SDP buffer too small

Comment 3

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

Comment on attachment 661166 [details] [diff] [review]
Error out on SDP buffer too small

If you don't error out, on the next attr the pointer is past the end, and the len (u16) is 6xxxx.
sdp_main.c is actually ok, but added comments.

FYI to cristoph - as this is code not in m-c yet, once it's fixed, should it be opened?  The main reason not to would be that this is imported code, so sec flaws (in the original code at least) would also exist in any Cisco product using it....  (Ethan, you might want to note this for internal Cisco people; CCSIP has/will get a lot of bugfixes relevant to the original source - though they might need to deal with them as reports and not patches, due to licensing issues.)

Comment 4

[Ethan Hugg [:ehugg]]

Comment on attachment 661166 [details] [diff] [review]
Error out on SDP buffer too small

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

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +215,1 @@
>              }

Is there a reason we wouldn't want to do this instead?  Seems we are checking the same value twice.

result = sdp_attr[attr_p->type].build_func(sdp_p, attr_p,ptr, (u16)(endbuf_p - *ptr));

/* If we ran out of buffer space, we must error out */
if (endbuf_p - *ptr <= 0)
  return (SDP_POTENTIAL_SDP_OVERFLOW);

if (result == SDP_SUCCESS) {

Comment 5

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

Attachment: Error out on SDP buffer too small

Good point; switched and also corrected indent level to match the file

Comment 6

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

http://hg.mozilla.org/projects/alder/rev/a08a4ac425dc