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.

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).

Bug: chromium:835958
Change-Id: Ifb4b99f68dc272507728ab59c07627f0d1b9c605
Reviewed-on: https://webrtc-review.googlesource.com/81642
Commit-Queue: Zhi Huang <zhihuang@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23570}

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/jseptransportcontroller.cc

@@ -478,6 +478,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

@@ -77,6 +77,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;
   };
 
   // The ICE related events are signaled on the |signaling_thread|.

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!=(
@@ -937,6 +939,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(