Prevent JS from bypassing RTP data channel bandwidth limitation.
Normally the RTP data channel is capped at 30kbps, but by mangling the
SDP string, one could get around this limitation. With this fix,
SdpDeserialize will return an error if it detects this condition.
BUG=280726
R=pthatcher@webrtc.org
Review URL: https://codereview.webrtc.org/1196403004.
Cr-Commit-Position: refs/heads/master@{#9499}
diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc
index aaf9d71..9d64795 100644
--- a/talk/app/webrtc/webrtcsdp.cc
+++ b/talk/app/webrtc/webrtcsdp.cc
@@ -1274,16 +1274,7 @@
// RFC 4566
// b=AS:<bandwidth>
- // 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 @@
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 @@
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 2bd6f6d..b657768 100644
--- a/talk/app/webrtc/webrtcsdp_unittest.cc
+++ b/talk/app/webrtc/webrtcsdp_unittest.cc
@@ -1698,12 +1698,7 @@
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, 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<DataContentDescription*>(
- 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 @@
&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<DataContentDescription*>(
+ 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));
}
