Adding a restriction for legal RID values.
According to the spec, RID values should be constrained to only
alpha-numeric values. This was not enforced in our implementation to
allow for more flexibility.
It has been brought to our attention that some values that we currently
consider legal (such as the '~', '=' ';' characters) might cause confusion
with the simulcast syntax that uses these characters to indicate other
meanings.
What's worse, is that some characters, when used in RIDs (such as
\u{1f937} \u{1f4a9} and \u{1f926}) cause uncontrollable laughter for some
users which might also be a health hazard.
This change resolves these issues by restricting RIDs to alpha-numeric.
Bug: webrtc:10491
Change-Id: I16e262c87525d0289764beacd098e1525a355463
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/132061
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Amit Hilbuch <amithi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27499}diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index a913f42..48c864b1 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -208,6 +208,7 @@
"../logging:rtc_event_log_impl_output",
"../media:rtc_data",
"../media:rtc_media_base",
+ "../modules/rtp_rtcp:rtp_rtcp_format",
"../p2p:rtc_p2p",
"../rtc_base",
"../rtc_base:checks",
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 656e21d..6e05d54 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -24,11 +24,14 @@
#include "api/jsep_session_description.h"
#include "api/media_stream_proxy.h"
#include "api/media_stream_track_proxy.h"
+#include "api/rtc_error.h"
+#include "api/rtp_parameters.h"
#include "api/uma_metrics.h"
#include "call/call.h"
#include "logging/rtc_event_log/ice_logger.h"
#include "logging/rtc_event_log/output/rtc_event_log_output_file.h"
#include "logging/rtc_event_log/rtc_event_log.h"
+#include "media/base/rid_description.h"
#include "media/sctp/sctp_transport.h"
#include "pc/audio_rtp_receiver.h"
#include "pc/audio_track.h"
@@ -1547,6 +1550,14 @@
"RIDs must be provided for either all or none of the send encodings.");
}
+ if (num_rids > 0 && absl::c_any_of(init.send_encodings,
+ [](const RtpEncodingParameters& encoding) {
+ return !IsLegalRsidName(encoding.rid);
+ })) {
+ LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
+ "Invalid RID value provided.");
+ }
+
if (absl::c_any_of(init.send_encodings,
[](const RtpEncodingParameters& encoding) {
return encoding.ssrc.has_value();
diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc
index 3efc6cb..7618ce2 100644
--- a/pc/peer_connection_simulcast_unittest.cc
+++ b/pc/peer_connection_simulcast_unittest.cc
@@ -15,6 +15,8 @@
#include "api/audio_codecs/builtin_audio_decoder_factory.h"
#include "api/audio_codecs/builtin_audio_encoder_factory.h"
#include "api/create_peerconnection_factory.h"
+#include "api/media_types.h"
+#include "api/rtc_error.h"
#include "api/rtp_transceiver_interface.h"
#include "api/uma_metrics.h"
#include "api/video_codecs/builtin_video_decoder_factory.h"
@@ -254,6 +256,16 @@
EXPECT_EQ(RTCErrorType::INVALID_PARAMETER, error.error().type());
}
+// Validates that an error is returned when illegal RIDs are supplied.
+TEST_F(PeerConnectionSimulcastTests, ChecksForIllegalRidValues) {
+ auto pc_wrapper = CreatePeerConnectionWrapper();
+ auto pc = pc_wrapper->pc();
+ auto layers = CreateLayers({"f", "h", "~q"}, true);
+ auto init = CreateTransceiverInit(layers);
+ auto error = pc->AddTransceiver(cricket::MEDIA_TYPE_VIDEO, init);
+ EXPECT_EQ(RTCErrorType::INVALID_PARAMETER, error.error().type());
+}
+
// Validates that a single RID is removed from the encoding layer.
TEST_F(PeerConnectionSimulcastTests, SingleRidIsRemovedFromSessionDescription) {
auto pc = CreatePeerConnectionWrapper();
diff --git a/pc/sdp_serializer.cc b/pc/sdp_serializer.cc
index 482dd07..7ebaffd 100644
--- a/pc/sdp_serializer.cc
+++ b/pc/sdp_serializer.cc
@@ -16,6 +16,7 @@
#include "absl/algorithm/container.h"
#include "api/jsep.h"
+#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
#include "rtc_base/checks.h"
#include "rtc_base/string_encode.h"
#include "rtc_base/string_to_number.h"
@@ -328,6 +329,10 @@
return ParseError("Invalid RID Description format. Too many arguments.");
}
+ if (!IsLegalRsidName(tokens[0])) {
+ return ParseError("Invalid RID value: " + tokens[0] + ".");
+ }
+
if (tokens[1] != kSendDirection && tokens[1] != kReceiveDirection) {
return ParseError("Invalid RID direction. Supported values: send / recv.");
}
diff --git a/pc/sdp_serializer_unittest.cc b/pc/sdp_serializer_unittest.cc
index e655f22..726d01d 100644
--- a/pc/sdp_serializer_unittest.cc
+++ b/pc/sdp_serializer_unittest.cc
@@ -469,6 +469,10 @@
"1 send pt=",
"1 send pt=abc",
"1 recv ;;",
+ "~1 recv",
+ "1$2 send",
+ "1=2 send",
+ "1* send",
};
INSTANTIATE_TEST_SUITE_P(RidDescriptionDeserializationErrors,
