Add a flag to actively reset the SRTP parameters
Add a new flag to RtcConfiguration. By setting that flag to true, the
SRTP parameters will be reset whenever the DTLS transports are reset
after every offer/answer negotiation.
The flag is added to Android and Objc wrapper as well.
This should only be used as a workaround for the linked bug, if the
application knows that the other party is affected (for instance,
using a version number).
TBR=sakal@webrtc.org, denicija@webrtc.org
Bug: chromium:835958
Change-Id: I6db025e1c69bf83e1b1908f7df4627430db9920c
Reviewed-on: https://webrtc-review.googlesource.com/83101
Commit-Queue: Zhi Huang <zhihuang@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23587}diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h
index ab9f458..757811e 100644
--- a/api/peerconnectioninterface.h
+++ b/api/peerconnectioninterface.h
@@ -577,6 +577,13 @@
// For all other users, specify kUnifiedPlan.
SdpSemantics sdp_semantics = SdpSemantics::kPlanB;
+ // Actively reset the SRTP parameters whenever the DTLS transports
+ // underneath are reset for every offer/answer negotiation.
+ // This is only intended to be a workaround for crbug.com/835958
+ // WARNING: This would cause RTP/RTCP packets decryption failure if not used
+ // correctly. This flag will be deprecated soon. Do not rely on it.
+ bool active_reset_srtp_params = false;
+
//
// Don't forget to update operator== if adding something.
//
diff --git a/pc/dtlssrtptransport.cc b/pc/dtlssrtptransport.cc
index a452d97..e31b2f5 100644
--- a/pc/dtlssrtptransport.cc
+++ b/pc/dtlssrtptransport.cc
@@ -39,7 +39,10 @@
// When using DTLS-SRTP, we must reset the SrtpTransport every time the
// DtlsTransport changes and wait until the DTLS handshake is complete to set
// the newly negotiated parameters.
- if (IsSrtpActive() && rtp_dtls_transport != rtp_dtls_transport_) {
+ // If |active_reset_srtp_params_| is true, intentionally reset the SRTP
+ // parameter even though the DtlsTransport may not change.
+ if (IsSrtpActive() && (rtp_dtls_transport != rtp_dtls_transport_ ||
+ active_reset_srtp_params_)) {
ResetParams();
}
diff --git a/pc/dtlssrtptransport.h b/pc/dtlssrtptransport.h
index c24034a..a2d7aad 100644
--- a/pc/dtlssrtptransport.h
+++ b/pc/dtlssrtptransport.h
@@ -53,6 +53,12 @@
"Set SRTP keys for DTLS-SRTP is not supported.");
}
+ // If |active_reset_srtp_params_| is set to be true, the SRTP parameters will
+ // be reset whenever the DtlsTransports are reset.
+ void SetActiveResetSrtpParams(bool active_reset_srtp_params) {
+ active_reset_srtp_params_ = active_reset_srtp_params;
+ }
+
private:
bool IsDtlsActive();
bool IsDtlsConnected();
@@ -84,6 +90,8 @@
// The encrypted header extension IDs.
rtc::Optional<std::vector<int>> send_extension_ids_;
rtc::Optional<std::vector<int>> recv_extension_ids_;
+
+ bool active_reset_srtp_params_ = false;
};
} // namespace webrtc
diff --git a/pc/dtlssrtptransport_unittest.cc b/pc/dtlssrtptransport_unittest.cc
index e1da462..cfe817f 100644
--- a/pc/dtlssrtptransport_unittest.cc
+++ b/pc/dtlssrtptransport_unittest.cc
@@ -510,3 +510,51 @@
// errors when a packet with a duplicate SRTCP index is received.
SendRecvRtcpPackets();
}
+
+// Tests that RTCP packets can be sent and received if both sides actively reset
+// the SRTP parameters with the |active_reset_srtp_params_| flag.
+TEST_F(DtlsSrtpTransportTest, ActivelyResetSrtpParams) {
+ auto rtp_dtls1 = rtc::MakeUnique<FakeDtlsTransport>(
+ "audio", cricket::ICE_CANDIDATE_COMPONENT_RTP);
+ auto rtcp_dtls1 = rtc::MakeUnique<FakeDtlsTransport>(
+ "audio", cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+ auto rtp_dtls2 = rtc::MakeUnique<FakeDtlsTransport>(
+ "audio", cricket::ICE_CANDIDATE_COMPONENT_RTP);
+ auto rtcp_dtls2 = rtc::MakeUnique<FakeDtlsTransport>(
+ "audio", cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+
+ MakeDtlsSrtpTransports(rtp_dtls1.get(), rtcp_dtls1.get(), rtp_dtls2.get(),
+ rtcp_dtls2.get(), /*rtcp_mux_enabled=*/true);
+ CompleteDtlsHandshake(rtp_dtls1.get(), rtp_dtls2.get());
+ CompleteDtlsHandshake(rtcp_dtls1.get(), rtcp_dtls2.get());
+
+ // Send some RTCP packets, causing the SRTCP index to be incremented.
+ SendRecvRtcpPackets();
+
+ // Only set the |active_reset_srtp_params_| flag to be true one side.
+ dtls_srtp_transport1_->SetActiveResetSrtpParams(true);
+ // Set RTCP transport to null to trigger the SRTP parameters update.
+ dtls_srtp_transport1_->SetDtlsTransports(rtp_dtls1.get(), nullptr);
+ dtls_srtp_transport2_->SetDtlsTransports(rtp_dtls2.get(), nullptr);
+
+ // Sending some RTCP packets.
+ size_t rtcp_len = sizeof(kRtcpReport);
+ size_t packet_size = rtcp_len + 4 + kRtpAuthTagLen;
+ rtc::Buffer rtcp_packet_buffer(packet_size);
+ rtc::CopyOnWriteBuffer rtcp_packet(kRtcpReport, rtcp_len, packet_size);
+ int prev_received_packets = transport_observer2_.rtcp_count();
+ ASSERT_TRUE(dtls_srtp_transport1_->SendRtcpPacket(
+ &rtcp_packet, rtc::PacketOptions(), cricket::PF_SRTP_BYPASS));
+ // The RTCP packet is not exepected to be received because the SRTP parameters
+ // are only reset on one side and the SRTCP index is out of sync.
+ EXPECT_EQ(prev_received_packets, transport_observer2_.rtcp_count());
+
+ // Set the flag to be true on the other side.
+ dtls_srtp_transport2_->SetActiveResetSrtpParams(true);
+ // Set RTCP transport to null to trigger the SRTP parameters update.
+ dtls_srtp_transport1_->SetDtlsTransports(rtp_dtls1.get(), nullptr);
+ dtls_srtp_transport2_->SetDtlsTransports(rtp_dtls2.get(), nullptr);
+
+ // RTCP packets flow is expected to work just fine.
+ SendRecvRtcpPackets();
+}
diff --git a/pc/jseptransport.cc b/pc/jseptransport.cc
index ab785af..adce4af 100644
--- a/pc/jseptransport.cc
+++ b/pc/jseptransport.cc
@@ -330,6 +330,15 @@
std::string(desc.str()));
}
+void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) {
+ if (dtls_srtp_transport_) {
+ RTC_LOG(INFO)
+ << "Setting active_reset_srtp_params of DtlsSrtpTransport to: "
+ << active_reset_srtp_params;
+ dtls_srtp_transport_->SetActiveResetSrtpParams(active_reset_srtp_params);
+ }
+}
+
void JsepTransport::SetLocalIceParameters(IceTransportInternal* ice_transport) {
RTC_DCHECK(ice_transport);
RTC_DCHECK(local_description_);
diff --git a/pc/jseptransport.h b/pc/jseptransport.h
index 290ac4a..392f861 100644
--- a/pc/jseptransport.h
+++ b/pc/jseptransport.h
@@ -173,6 +173,8 @@
const rtc::RTCCertificate* certificate,
const rtc::SSLFingerprint* fingerprint) const;
+ void SetActiveResetSrtpParams(bool active_reset_srtp_params);
+
private:
bool SetRtcpMux(bool enable, webrtc::SdpType type, ContentSource source);
diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc
index 2d3eeb9..52519c0 100644
--- a/pc/jseptransportcontroller.cc
+++ b/pc/jseptransportcontroller.cc
@@ -379,6 +379,24 @@
}
}
+void JsepTransportController::SetActiveResetSrtpParams(
+ bool active_reset_srtp_params) {
+ if (!network_thread_->IsCurrent()) {
+ network_thread_->Invoke<void>(RTC_FROM_HERE, [=] {
+ SetActiveResetSrtpParams(active_reset_srtp_params);
+ });
+ return;
+ }
+
+ RTC_LOG(INFO)
+ << "Updating the active_reset_srtp_params for JsepTransportController: "
+ << active_reset_srtp_params;
+ config_.active_reset_srtp_params = active_reset_srtp_params;
+ for (auto& kv : jsep_transports_by_name_) {
+ kv.second->SetActiveResetSrtpParams(active_reset_srtp_params);
+ }
+}
+
std::unique_ptr<cricket::DtlsTransportInternal>
JsepTransportController::CreateDtlsTransport(const std::string& transport_name,
bool rtcp) {
@@ -478,6 +496,8 @@
dtls_srtp_transport->SetDtlsTransports(rtp_dtls_transport,
rtcp_dtls_transport);
+ dtls_srtp_transport->SetActiveResetSrtpParams(
+ config_.active_reset_srtp_params);
return dtls_srtp_transport;
}
diff --git a/pc/jseptransportcontroller.h b/pc/jseptransportcontroller.h
index dee05fa..b678a09 100644
--- a/pc/jseptransportcontroller.h
+++ b/pc/jseptransportcontroller.h
@@ -78,6 +78,7 @@
// Used to inject the ICE/DTLS transports created externally.
cricket::TransportFactoryInterface* external_transport_factory = nullptr;
Observer* transport_observer = nullptr;
+ bool active_reset_srtp_params = false;
RtcEventLog* event_log = nullptr;
};
@@ -155,6 +156,8 @@
bool initial_offerer() const { return initial_offerer_ && *initial_offerer_; }
+ void SetActiveResetSrtpParams(bool active_reset_srtp_params);
+
// All of these signals are fired on the signaling thread.
// If any transport failed => failed,
diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index 093e72d..cf56c35 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -693,6 +693,7 @@
webrtc::TurnCustomizer* turn_customizer;
SdpSemantics sdp_semantics;
rtc::Optional<rtc::AdapterType> network_preference;
+ bool active_reset_srtp_params;
};
static_assert(sizeof(stuff_being_tested_for_equality) == sizeof(*this),
"Did you add something to RTCConfiguration and forget to "
@@ -739,7 +740,8 @@
ice_regather_interval_range == o.ice_regather_interval_range &&
turn_customizer == o.turn_customizer &&
sdp_semantics == o.sdp_semantics &&
- network_preference == o.network_preference;
+ network_preference == o.network_preference &&
+ active_reset_srtp_params == o.active_reset_srtp_params;
}
bool PeerConnectionInterface::RTCConfiguration::operator!=(
@@ -938,6 +940,7 @@
#if defined(ENABLE_EXTERNAL_AUTH)
config.enable_external_auth = true;
#endif
+ config.active_reset_srtp_params = configuration.active_reset_srtp_params;
transport_controller_.reset(new JsepTransportController(
signaling_thread(), network_thread(), port_allocator_.get(), config));
transport_controller_->SignalIceConnectionState.connect(
@@ -2841,7 +2844,6 @@
bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
RTCError* error) {
TRACE_EVENT0("webrtc", "PeerConnection::SetConfiguration");
-
if (IsClosed()) {
RTC_LOG(LS_ERROR) << "SetConfiguration: PeerConnection is closed.";
return SafeSetError(RTCErrorType::INVALID_STATE, error);
@@ -2879,6 +2881,8 @@
configuration.stun_candidate_keepalive_interval;
modified_config.turn_customizer = configuration.turn_customizer;
modified_config.network_preference = configuration.network_preference;
+ modified_config.active_reset_srtp_params =
+ configuration.active_reset_srtp_params;
if (configuration != modified_config) {
RTC_LOG(LS_ERROR) << "Modifying the configuration in an unsupported way.";
return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
@@ -2937,6 +2941,12 @@
transport_controller_->SetIceConfig(ParseIceConfig(modified_config));
+ if (configuration_.active_reset_srtp_params !=
+ modified_config.active_reset_srtp_params) {
+ transport_controller_->SetActiveResetSrtpParams(
+ modified_config.active_reset_srtp_params);
+ }
+
configuration_ = modified_config;
return SafeSetError(RTCErrorType::NONE, error);
}
diff --git a/sdk/android/api/org/webrtc/PeerConnection.java b/sdk/android/api/org/webrtc/PeerConnection.java
index b9e31e1..2f9adcf 100644
--- a/sdk/android/api/org/webrtc/PeerConnection.java
+++ b/sdk/android/api/org/webrtc/PeerConnection.java
@@ -457,6 +457,10 @@
// This is an optional wrapper for the C++ webrtc::TurnCustomizer.
@Nullable public TurnCustomizer turnCustomizer;
+ // Actively reset the SRTP parameters whenever the DTLS transports underneath are reset for
+ // every offer/answer negotiation.This is only intended to be a workaround for crbug.com/835958
+ public boolean activeResetSrtpParams;
+
// TODO(deadbeef): Instead of duplicating the defaults here, we should do
// something to pick up the defaults from C++. The Objective-C equivalent
// of RTCConfiguration does that.
@@ -495,6 +499,7 @@
enableDtlsSrtp = null;
networkPreference = AdapterType.UNKNOWN;
sdpSemantics = SdpSemantics.PLAN_B;
+ activeResetSrtpParams = false;
}
@CalledByNative("RTCConfiguration")
@@ -682,6 +687,11 @@
SdpSemantics getSdpSemantics() {
return sdpSemantics;
}
+
+ @CalledByNative("RTCConfiguration")
+ boolean getActiveResetSrtpParams() {
+ return activeResetSrtpParams;
+ }
};
private final List<MediaStream> localStreams = new ArrayList<>();
diff --git a/sdk/android/src/jni/pc/peerconnection.cc b/sdk/android/src/jni/pc/peerconnection.cc
index 225443f..722a785 100644
--- a/sdk/android/src/jni/pc/peerconnection.cc
+++ b/sdk/android/src/jni/pc/peerconnection.cc
@@ -232,6 +232,8 @@
rtc_config->network_preference =
JavaToNativeNetworkPreference(jni, j_network_preference);
rtc_config->sdp_semantics = JavaToNativeSdpSemantics(jni, j_sdp_semantics);
+ rtc_config->active_reset_srtp_params =
+ Java_RTCConfiguration_getActiveResetSrtpParams(jni, j_rtc_config);
}
rtc::KeyType GetRtcConfigKeyType(JNIEnv* env,
diff --git a/sdk/objc/Framework/Classes/PeerConnection/RTCConfiguration.mm b/sdk/objc/Framework/Classes/PeerConnection/RTCConfiguration.mm
index e318a9f..4bd0450 100644
--- a/sdk/objc/Framework/Classes/PeerConnection/RTCConfiguration.mm
+++ b/sdk/objc/Framework/Classes/PeerConnection/RTCConfiguration.mm
@@ -45,6 +45,7 @@
@synthesize iceRegatherIntervalRange = _iceRegatherIntervalRange;
@synthesize sdpSemantics = _sdpSemantics;
@synthesize turnCustomizer = _turnCustomizer;
+@synthesize activeResetSrtpParams = _activeResetSrtpParams;
- (instancetype)init {
// Copy defaults.
@@ -99,6 +100,7 @@
}
_sdpSemantics = [[self class] sdpSemanticsForNativeSdpSemantics:config.sdp_semantics];
_turnCustomizer = config.turn_customizer;
+ _activeResetSrtpParams = config.active_reset_srtp_params;
}
return self;
}
@@ -106,7 +108,7 @@
- (NSString *)description {
static NSString *formatString =
@"RTCConfiguration: "
- @"{\n%@\n%@\n%@\n%@\n%@\n%@\n%@\n%@\n%d\n%d\n%d\n%d\n%d\n%d\n%d\n%@\n%@\n%d\n%d\n}\n";
+ @"{\n%@\n%@\n%@\n%@\n%@\n%@\n%@\n%@\n%d\n%d\n%d\n%d\n%d\n%d\n%d\n%@\n%@\n%d\n%d\n%d\n}\n";
return [NSString
stringWithFormat:formatString,
@@ -128,7 +130,8 @@
_iceCheckMinInterval,
_iceRegatherIntervalRange,
_disableLinkLocalNetworks,
- _maxIPv6Networks];
+ _maxIPv6Networks,
+ _activeResetSrtpParams];
}
#pragma mark - Private
@@ -194,6 +197,7 @@
if (_turnCustomizer) {
nativeConfig->turn_customizer = _turnCustomizer;
}
+ nativeConfig->active_reset_srtp_params = _activeResetSrtpParams ? true : false;
return nativeConfig.release();
}
diff --git a/sdk/objc/Framework/Headers/WebRTC/RTCConfiguration.h b/sdk/objc/Framework/Headers/WebRTC/RTCConfiguration.h
index dd8f364..b203b28 100644
--- a/sdk/objc/Framework/Headers/WebRTC/RTCConfiguration.h
+++ b/sdk/objc/Framework/Headers/WebRTC/RTCConfiguration.h
@@ -162,6 +162,12 @@
*/
@property(nonatomic, assign) RTCSdpSemantics sdpSemantics;
+/** Actively reset the SRTP parameters when the DTLS transports underneath are
+ * changed after offer/answer negotiation. This is only intended to be a
+ * workaround for crbug.com/835958
+ */
+@property(nonatomic, assign) BOOL activeResetSrtpParams;
+
- (instancetype)init;
@end
diff --git a/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm b/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm
index 686bf2d..63617cb 100644
--- a/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm
+++ b/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm
@@ -49,6 +49,7 @@
config.continualGatheringPolicy =
RTCContinualGatheringPolicyGatherContinually;
config.shouldPruneTurnPorts = YES;
+ config.activeResetSrtpParams = YES;
RTCMediaConstraints *contraints = [[RTCMediaConstraints alloc] initWithMandatoryConstraints:@{}
optionalConstraints:nil];
@@ -87,6 +88,7 @@
newConfig.iceBackupCandidatePairPingInterval);
EXPECT_EQ(config.continualGatheringPolicy, newConfig.continualGatheringPolicy);
EXPECT_EQ(config.shouldPruneTurnPorts, newConfig.shouldPruneTurnPorts);
+ EXPECT_EQ(config.activeResetSrtpParams, newConfig.activeResetSrtpParams);
}
@end