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}

api/peerconnectioninterface.h

@@ -577,6 +577,13 @@ class PeerConnectionInterface : public rtc::RefCountInterface {
     // 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.
     //

pc/dtlssrtptransport.cc

@@ -39,7 +39,10 @@ void DtlsSrtpTransport::SetDtlsTransports(
   // 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();
   }
 

pc/dtlssrtptransport.h

@@ -53,6 +53,12 @@ class DtlsSrtpTransport : public SrtpTransport {
                     "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 @@ class DtlsSrtpTransport : public SrtpTransport {
   // 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

pc/dtlssrtptransport_unittest.cc

@@ -510,3 +510,51 @@ TEST_F(DtlsSrtpTransportTest, SrtpSessionNotResetWhenRtcpTransportRemoved) {
   // 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();
+}

pc/jseptransport.cc

@@ -330,6 +330,15 @@ webrtc::RTCError JsepTransport::VerifyCertificateFingerprint(
                           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_);

pc/jseptransport.h

@@ -173,6 +173,8 @@ class JsepTransport : public sigslot::has_slots<> {
       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);
 

pc/jseptransportcontroller.cc

@@ -379,6 +379,24 @@ void JsepTransportController::SetMetricsObserver(
   }
 }
 
+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 @@ JsepTransportController::CreateDtlsSrtpTransport(
 
   dtls_srtp_transport->SetDtlsTransports(rtp_dtls_transport,
                                          rtcp_dtls_transport);
+  dtls_srtp_transport->SetActiveResetSrtpParams(
+      config_.active_reset_srtp_params);
   return dtls_srtp_transport;
 }
 

pc/jseptransportcontroller.h

@@ -78,6 +78,7 @@ class JsepTransportController : public sigslot::has_slots<>,
     // 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 @@ class JsepTransportController : public sigslot::has_slots<>,
 
   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,

pc/peerconnection.cc

@@ -693,6 +693,7 @@ bool PeerConnectionInterface::RTCConfiguration::operator==(
     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 @@ bool PeerConnectionInterface::RTCConfiguration::operator==(
          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 @@ bool PeerConnection::Initialize(
 #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 @@ PeerConnectionInterface::RTCConfiguration PeerConnection::GetConfiguration() {
 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 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
       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 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
 
   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);
 }

sdk/android/api/org/webrtc/PeerConnection.java

@@ -457,6 +457,10 @@ public class PeerConnection {
     // 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 @@ public class PeerConnection {
       enableDtlsSrtp = null;
       networkPreference = AdapterType.UNKNOWN;
       sdpSemantics = SdpSemantics.PLAN_B;
+      activeResetSrtpParams = false;
     }
 
     @CalledByNative("RTCConfiguration")
@@ -682,6 +687,11 @@ public class PeerConnection {
     SdpSemantics getSdpSemantics() {
       return sdpSemantics;
     }
+
+    @CalledByNative("RTCConfiguration")
+    boolean getActiveResetSrtpParams() {
+      return activeResetSrtpParams;
+    }
   };
 
   private final List<MediaStream> localStreams = new ArrayList<>();

sdk/android/src/jni/pc/peerconnection.cc

@@ -232,6 +232,8 @@ void JavaToNativeRTCConfiguration(
   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,

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

sdk/objc/Framework/Headers/WebRTC/RTCConfiguration.h

@@ -162,6 +162,12 @@ RTC_EXPORT
  */
 @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

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