heplify icon indicating copy to clipboard operation
heplify copied to clipboard

Potential bug

Open systemcrash opened this issue 4 years ago • 4 comments

https://github.com/sipcapture/heplify/blob/1b2a80825bf4ef051337a8a746285346c7d992c3/decoder/correlator.go#L53

// Minimum RTCP port length of "m=audio 1000" = 12

Potentially, this is wrong. This assumes that only non-privileged ports are used by the declaring host. If the host declaring the SDP has port number 8 available to listen, the string is shorter. I have seen these in the wild.

Consider

// Minimum RTCP port length of "m=audio 8" = 9

i.e.

	if posRestPort := bytes.Index(restPort, []byte(" RTP")); posRestPort >= 9 {

I think.

systemcrash avatar May 15 '20 15:05 systemcrash

Same assumption goes for:

https://github.com/sipcapture/heplify/blob/1b2a80825bf4ef051337a8a746285346c7d992c3/decoder/correlator.go#L42

systemcrash avatar May 15 '20 15:05 systemcrash

These standards do not mandate port ranges or string lengths for the m= or a= row: https://tools.ietf.org/html/rfc4566#section-5.14

https://tools.ietf.org/html/rfc2327#page-19

Also, the assumption is that the media type is audio. Possible types are: https://tools.ietf.org/html/rfc2327#page-19

The first sub-field is the media type.  Currently defined media are
     "audio", "video", "application", "data" and "control", though this
     list may be extended as new communication modalities emerge (e.g.,
     telepresense).

I have seen text. So data or text = length 4.

making:

// Minimum RTCP port length of "m=data 8" = 8

systemcrash avatar May 15 '20 15:05 systemcrash

But this is not a safe assumption. a media type of z is theoretically possible ;)

systemcrash avatar May 15 '20 15:05 systemcrash

Yeah could be improved.

negbie avatar May 15 '20 17:05 negbie