diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc index aaf9d71940..9d647955d1 100644 --- a/talk/app/webrtc/webrtcsdp.cc +++ b/talk/app/webrtc/webrtcsdp.cc @@ -1274,16 +1274,7 @@ void BuildMediaDescription(const ContentInfo* content_info, // RFC 4566 // b=AS: - // We should always use the default bandwidth for RTP-based data - // channels. Don't allow SDP to set the bandwidth, because that - // would give JS the opportunity to "break the Internet". - // TODO(pthatcher): But we need to temporarily allow the SDP to control - // this for backwards-compatibility. Once we don't need that any - // more, remove this. - bool support_dc_sdp_bandwidth_temporarily = true; - if (media_desc->bandwidth() >= 1000 && - (media_type != cricket::MEDIA_TYPE_DATA || - support_dc_sdp_bandwidth_temporarily)) { + if (media_desc->bandwidth() >= 1000) { InitLine(kLineTypeSessionBandwidth, kApplicationSpecificMaximum, &os); os << kSdpDelimiterColon << (media_desc->bandwidth() / 1000); AddLine(os.str(), message); @@ -2249,17 +2240,6 @@ bool ParseMediaDescription(const std::string& message, if (!AddSctpDataCodec(data_desc, p)) return false; } - - // We should always use the default bandwidth for RTP-based data - // channels. Don't allow SDP to set the bandwidth, because that - // would give JS the opportunity to "break the Internet". - // TODO(pthatcher): But we need to temporarily allow the SDP to control - // this for backwards-compatibility. Once we don't need that any - // more, remove this. - bool support_dc_sdp_bandwidth_temporarily = true; - if (content.get() && !support_dc_sdp_bandwidth_temporarily) { - content->set_bandwidth(cricket::kAutoBandwidth); - } } else { LOG(LS_WARNING) << "Unsupported media type: " << line; continue; @@ -2517,6 +2497,17 @@ bool ParseContent(const std::string& message, if (!GetValueFromString(line, bandwidth, &b, error)) { return false; } + // We should never use more than the default bandwidth for RTP-based + // data channels. Don't allow SDP to set the bandwidth, because + // that would give JS the opportunity to "break the Internet". + // See: https://code.google.com/p/chromium/issues/detail?id=280726 + if (media_type == cricket::MEDIA_TYPE_DATA && IsRtp(protocol) && + b > cricket::kDataMaxBandwidth / 1000) { + std::ostringstream description; + description << "RTP-based data channels may not send more than " + << cricket::kDataMaxBandwidth / 1000 << "kbps."; + return ParseFailed(line, description.str(), error); + } media_desc->set_bandwidth(b * 1000); } } diff --git a/talk/app/webrtc/webrtcsdp_unittest.cc b/talk/app/webrtc/webrtcsdp_unittest.cc index 2bd6f6d668..b657768204 100644 --- a/talk/app/webrtc/webrtcsdp_unittest.cc +++ b/talk/app/webrtc/webrtcsdp_unittest.cc @@ -1698,12 +1698,7 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithDataChannelAndBandwidth) { std::string expected_sdp = kSdpString; expected_sdp.append(kSdpRtpDataChannelString); - // We want to test that serializing data content ignores bandwidth - // settings (it should always be the default). Thus, we don't do - // the following: - // TODO(pthatcher): We need to temporarily allow the SDP to control - // this for backwards-compatibility. Once we don't need that any - // more, remove this. + // Serializing data content shouldn't ignore bandwidth settings. InjectAfter("m=application 9 RTP/SAVPF 101\r\nc=IN IP4 0.0.0.0\r\n", "b=AS:100\r\n", &expected_sdp); @@ -2259,19 +2254,11 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannelAndNewPort) { } TEST_F(WebRtcSdpTest, DeserializeSdpWithRtpDataChannelsAndBandwidth) { - AddRtpDataChannel(); - JsepSessionDescription jdesc(kDummyString); - // We want to test that deserializing data content ignores bandwidth - // settings (it should always be the default). Thus, we don't do - // the following: - // TODO(pthatcher): We need to temporarily allow the SDP to control - // this for backwards-compatibility. Once we don't need that any - // more, remove this. - DataContentDescription* dcd = static_cast( - GetFirstDataContent(&desc_)->description); - dcd->set_bandwidth(100 * 1000); - ASSERT_TRUE(jdesc.Initialize(desc_.Copy(), kSessionId, kSessionVersion)); - + // We want to test that deserializing data content limits bandwidth + // settings (it should never be greater than the default). + // This should prevent someone from using unlimited data bandwidth through + // JS and "breaking the Internet". + // See: https://code.google.com/p/chromium/issues/detail?id=280726 std::string sdp_with_bandwidth = kSdpString; sdp_with_bandwidth.append(kSdpRtpDataChannelString); InjectAfter("a=mid:data_content_name\r\n", @@ -2279,8 +2266,27 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithRtpDataChannelsAndBandwidth) { &sdp_with_bandwidth); JsepSessionDescription jdesc_with_bandwidth(kDummyString); - EXPECT_TRUE( - SdpDeserialize(sdp_with_bandwidth, &jdesc_with_bandwidth)); + EXPECT_FALSE(SdpDeserialize(sdp_with_bandwidth, &jdesc_with_bandwidth)); +} + +TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannelsAndBandwidth) { + AddSctpDataChannel(); + JsepSessionDescription jdesc(kDummyString); + DataContentDescription* dcd = static_cast( + GetFirstDataContent(&desc_)->description); + dcd->set_bandwidth(100 * 1000); + ASSERT_TRUE(jdesc.Initialize(desc_.Copy(), kSessionId, kSessionVersion)); + + std::string sdp_with_bandwidth = kSdpString; + sdp_with_bandwidth.append(kSdpSctpDataChannelString); + InjectAfter("a=mid:data_content_name\r\n", + "b=AS:100\r\n", + &sdp_with_bandwidth); + JsepSessionDescription jdesc_with_bandwidth(kDummyString); + + // SCTP has congestion control, so we shouldn't limit the bandwidth + // as we do for RTP. + EXPECT_TRUE(SdpDeserialize(sdp_with_bandwidth, &jdesc_with_bandwidth)); EXPECT_TRUE(CompareSessionDescription(jdesc, jdesc_with_bandwidth)); }