generic-library/webrtc
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Prevent JS from bypassing RTP data channel bandwidth limitation.

Browse Source 
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}
This commit is contained in:
Peter Thatcher 2015-06-24 15:31:25 -07:00
parent 8d3e489d01
commit c0c3a865f4
2 changed files with 39 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
talk/app/webrtc/webrtcsdp.cc
View File

@ -1274,16 +1274,7 @@ void BuildMediaDescription(const ContentInfo* content_info,
// 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 @@ 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);
}
}

48
talk/app/webrtc/webrtcsdp_unittest.cc
View File

@ -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<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 @@ 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<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));
}