1324608 - RtpStreamId RTP header extension indicates incorrect header length

Status: RESOLVED FIXED

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

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

When serializing the RtpStreamId header extension the length field should be (length of id)-1 as per rfc5285 section 4.2[0].
Additionally the length of the field should be checked to ensure that it will fit in 16 bytes.

Serialization occurs RtpSender::BuildRIDExtension[1]

[0] https://tools.ietf.org/html/rfc5285#section-4.2
[1] https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_sender.cc?q=BuildRIDExtension&redirect_type=direct#

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Bug 1324608: restrict RID len.

Review commit: https://reviewboard.mozilla.org/r/99850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99850/

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8820355 [details]
Bug 1324608: restrict RID len.

https://reviewboard.mozilla.org/r/99850/#review100294

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1435
(Diff revision 1)
>    }
>    size_t pos = 0;
>    // RID value is not null-terminated in header, so no +1
> -  const uint8_t len = strlen(rid_);
> -  data_buffer[pos++] = (id << 4) + len;
> +  uint8_t len = strlen(rid_);
> +  if (len > 16) {
> +    len = 16;

I am not sure if the proper behavior when the length is too long is to truncate or to fail (perhaps noisily).  I lean towards failure because truncated fields won't match the SDP anyways, and two RtpStreamIds could be truncated to the same value.

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1437
(Diff revision 1)
>    // RID value is not null-terminated in header, so no +1
> -  const uint8_t len = strlen(rid_);
> -  data_buffer[pos++] = (id << 4) + len;
> +  uint8_t len = strlen(rid_);
> +  if (len > 16) {
> +    len = 16;
> +  }
> +  data_buffer[pos++] = (id << 4) + (len - 1);

If len is 0, will this overflow?

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1437
(Diff revision 1)
>    // RID value is not null-terminated in header, so no +1
> -  const uint8_t len = strlen(rid_);
> -  data_buffer[pos++] = (id << 4) + len;
> +  uint8_t len = strlen(rid_);
> +  if (len > 16) {
> +    len = 16;
> +  }
> +  data_buffer[pos++] = (id << 4) + (len - 1);

If len is zero this will overflow.

Comment 3

[Nils Ohlmeier [:drno]]

Comment on attachment 8820355 [details]
Bug 1324608: restrict RID len.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99850/diff/1-2/

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8820355 [details]
Bug 1324608: restrict RID len.

https://reviewboard.mozilla.org/r/99850/#review100308

r+, with optional suggestion.

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1435
(Diff revision 2)
>    }
>    size_t pos = 0;
>    // RID value is not null-terminated in header, so no +1
>    const uint8_t len = strlen(rid_);
> -  data_buffer[pos++] = (id << 4) + len;
> +  if (len > 16 || len == 0) {
> +    LOG(LS_ERROR) << "Failed to add RID header because of unsupported RID"

If you want you could log the length.  It is up to you.

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8820355 [details]
Bug 1324608: restrict RID len.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99850/diff/2-3/

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8820355 [details]
Bug 1324608: restrict RID len.

https://reviewboard.mozilla.org/r/99850/#review100376

Looks good.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41bd7439928e
restrict RID len. r=ng

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41bd7439928e