When SDES is used, pass pre-shared key to media transport.

This allows to use secure, end to end communication if SDES cryptos are
passed. MediaTransport can use a derived key to secure its own
communication.

Bug: webrtc:9719
Change-Id: If1a20b136b3b4af0cb24f10b52fc5ce1eb31daa2
Reviewed-on: https://webrtc-review.googlesource.com/c/108504
Commit-Queue: Peter Slatala <psla@webrtc.org>
Reviewed-by: Seth Hampson <shampson@webrtc.org>
Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
Reviewed-by: Benjamin Wright <benwright@webrtc.org>
Reviewed-by: Bjorn Mellem <mellem@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25452}

api/media_transport_interface.h

@@ -48,6 +48,8 @@ struct MediaTransportSettings final {
   bool is_caller;
 
   // Must be set if a pre-shared key is used for the call.
+  // TODO(bugs.webrtc.org/9944): This should become zero buffer in the distant
+  // future.
   absl::optional<std::string> pre_shared_key;
 };
 

api/test/fake_media_transport.h

@@ -12,6 +12,7 @@
 #define API_TEST_FAKE_MEDIA_TRANSPORT_H_
 
 #include <memory>
+#include <string>
 #include <utility>
 
 #include "absl/memory/memory.h"
@@ -25,7 +26,8 @@ namespace webrtc {
 // could unit test audio / video integration.
 class FakeMediaTransport : public MediaTransportInterface {
  public:
-  explicit FakeMediaTransport(bool is_caller) : is_caller_(is_caller) {}
+  explicit FakeMediaTransport(const MediaTransportSettings& settings)
+      : settings_(settings) {}
   ~FakeMediaTransport() = default;
 
   RTCError SendAudioFrame(uint64_t channel_id,
@@ -46,14 +48,17 @@ class FakeMediaTransport : public MediaTransportInterface {
   void SetReceiveAudioSink(MediaTransportAudioSinkInterface* sink) override {}
   void SetReceiveVideoSink(MediaTransportVideoSinkInterface* sink) override {}
 
-  // Returns true if fake media trasport was created as a caller.
-  bool is_caller() const { return is_caller_; }
+  // Returns true if fake media transport was created as a caller.
+  bool is_caller() const { return settings_.is_caller; }
+  absl::optional<std::string> pre_shared_key() const {
+    return settings_.pre_shared_key;
+  }
 
   void SetTargetTransferRateObserver(
       webrtc::TargetTransferRateObserver* observer) override {}
 
  private:
-  const bool is_caller_;
+  const MediaTransportSettings settings_;
 };
 
 // Fake media transport factory creates fake media transport.
@@ -66,9 +71,17 @@ class FakeMediaTransportFactory : public MediaTransportFactory {
       rtc::PacketTransportInternal* packet_transport,
       rtc::Thread* network_thread,
       bool is_caller) override {
-    std::unique_ptr<MediaTransportInterface> media_transport =
-        absl::make_unique<FakeMediaTransport>(is_caller);
+    MediaTransportSettings settings;
+    settings.is_caller = is_caller;
+    return CreateMediaTransport(packet_transport, network_thread, settings);
+  }
 
+  RTCErrorOr<std::unique_ptr<MediaTransportInterface>> CreateMediaTransport(
+      rtc::PacketTransportInternal* packet_transport,
+      rtc::Thread* network_thread,
+      const MediaTransportSettings& settings) override {
+    std::unique_ptr<MediaTransportInterface> media_transport =
+        absl::make_unique<FakeMediaTransport>(settings);
     return std::move(media_transport);
   }
 };

pc/jseptransportcontroller.cc

@@ -15,8 +15,10 @@
 #include <utility>
 
 #include "p2p/base/port.h"
+#include "pc/srtpfilter.h"
 #include "rtc_base/bind.h"
 #include "rtc_base/checks.h"
+#include "rtc_base/key_derivation.h"
 #include "rtc_base/thread.h"
 
 using webrtc::SdpType;
@@ -940,16 +942,82 @@ RTCError JsepTransportController::MaybeCreateJsepTransport(
         CreateDtlsTransport(content_info.name, /*rtcp =*/true);
   }
 
+  absl::optional<cricket::CryptoParams> selected_crypto_for_media_transport;
+  if (content_info.media_description() &&
+      !content_info.media_description()->cryptos().empty()) {
+    // Order of cryptos is deterministic (rfc4568, 5.1.1), so we just select the
+    // first one (in fact the first one should be the most preferred one.) We
+    // ignore the HMAC size, as media transport crypto settings currently don't
+    // expose HMAC size, nor crypto protocol for that matter.
+    selected_crypto_for_media_transport =
+        content_info.media_description()->cryptos()[0];
+  }
+
   if (config_.media_transport_factory != nullptr) {
-    auto media_transport_result =
-        config_.media_transport_factory->CreateMediaTransport(
-            rtp_dtls_transport->ice_transport(), network_thread_,
-            /*is_caller=*/local);
+    if (!selected_crypto_for_media_transport.has_value()) {
+      RTC_LOG(LS_WARNING) << "a=cryto line was not found in the offer. Most "
+                             "likely you did not enable SDES. "
+                             "Make sure to pass config.enable_dtls_srtp=false "
+                             "to RTCConfiguration. "
+                             "Cannot continue with media transport. Falling "
+                             "back to RTP. is_local="
+                          << local;
 
-    // TODO(sukhanov): Proper error handling.
-    RTC_CHECK(media_transport_result.ok());
+      // Remove media_transport_factory from config, because we don't want to
+      // use it on the subsequent call (for the other side of the offer).
+      config_.media_transport_factory = nullptr;
+    } else {
+      // Note that we ignore here lifetime and length.
+      // In fact we take those bits (inline, lifetime and length) and keep it as
+      // part of key derivation.
+      //
+      // Technically, we are also not following rfc4568, which requires us to
+      // send and answer with the key that we chose. In practice, for media
+      // transport, the current approach should be sufficient (we take the key
+      // that sender offered, and caller assumes we will use it. We are not
+      // signaling back that we indeed used it.)
+      std::unique_ptr<rtc::KeyDerivation> key_derivation =
+          rtc::KeyDerivation::Create(rtc::KeyDerivationAlgorithm::HKDF_SHA256);
+      const std::string label = "MediaTransportLabel";
+      constexpr int kDerivedKeyByteSize = 32;
 
-    media_transport = std::move(media_transport_result.value());
+      int key_len, salt_len;
+      if (!rtc::GetSrtpKeyAndSaltLengths(
+              rtc::SrtpCryptoSuiteFromName(
+                  selected_crypto_for_media_transport.value().cipher_suite),
+              &key_len, &salt_len)) {
+        RTC_CHECK(false) << "Cannot set up secure media transport";
+      }
+      rtc::ZeroOnFreeBuffer<uint8_t> raw_key(key_len + salt_len);
+
+      cricket::SrtpFilter::ParseKeyParams(
+          selected_crypto_for_media_transport.value().key_params,
+          raw_key.data(), raw_key.size());
+      absl::optional<rtc::ZeroOnFreeBuffer<uint8_t>> key =
+          key_derivation->DeriveKey(
+              raw_key,
+              /*salt=*/nullptr,
+              rtc::ArrayView<const uint8_t>(
+                  reinterpret_cast<const uint8_t*>(label.data()), label.size()),
+              kDerivedKeyByteSize);
+
+      // We want to crash the app if we don't have a key, and not silently fall
+      // back to the unsecure communication.
+      RTC_CHECK(key.has_value());
+      MediaTransportSettings settings;
+      settings.is_caller = local;
+      settings.pre_shared_key =
+          std::string(reinterpret_cast<const char*>(key.value().data()),
+                      key.value().size());
+      auto media_transport_result =
+          config_.media_transport_factory->CreateMediaTransport(
+              rtp_dtls_transport->ice_transport(), network_thread_, settings);
+
+      // TODO(sukhanov): Proper error handling.
+      RTC_CHECK(media_transport_result.ok());
+
+      media_transport = std::move(media_transport_result.value());
+    }
   }
 
   // TODO(sukhanov): Do not create RTP/RTCP transports if media transport is

pc/jseptransportcontroller_unittest.cc

@@ -41,6 +41,20 @@ static const char kDataMid1[] = "data1";
 
 namespace webrtc {
 
+namespace {
+
+// Media transport factory requires crypto settings to be present in order to
+// create media transport.
+void AddCryptoSettings(cricket::SessionDescription* description) {
+  for (auto& content : description->contents()) {
+    content.media_description()->AddCrypto(cricket::CryptoParams(
+        /*t=*/0, std::string(rtc::CS_AES_CM_128_HMAC_SHA1_80),
+        "inline:YUJDZGVmZ2hpSktMbW9QUXJzVHVWd3l6MTIzNDU2", ""));
+  }
+}
+
+}  // namespace
+
 class FakeTransportFactory : public cricket::TransportFactoryInterface {
  public:
   std::unique_ptr<cricket::IceTransportInternal> CreateIceTransport(
@@ -380,6 +394,8 @@ TEST_F(JsepTransportControllerTest, GetMediaTransportInCaller) {
   config.media_transport_factory = &fake_media_transport_factory;
   CreateJsepTransportController(config);
   auto description = CreateSessionDescriptionWithoutBundle();
+  AddCryptoSettings(description.get());
+
   EXPECT_TRUE(transport_controller_
                   ->SetLocalDescription(SdpType::kOffer, description.get())
                   .ok());
@@ -391,6 +407,7 @@ TEST_F(JsepTransportControllerTest, GetMediaTransportInCaller) {
 
   // After SetLocalDescription, media transport should be created as caller.
   EXPECT_TRUE(media_transport->is_caller());
+  EXPECT_TRUE(media_transport->pre_shared_key().has_value());
 
   // Return nullptr for non-existing mids.
   EXPECT_EQ(nullptr, transport_controller_->GetMediaTransport(kVideoMid2));
@@ -404,6 +421,7 @@ TEST_F(JsepTransportControllerTest, GetMediaTransportInCallee) {
   config.media_transport_factory = &fake_media_transport_factory;
   CreateJsepTransportController(config);
   auto description = CreateSessionDescriptionWithoutBundle();
+  AddCryptoSettings(description.get());
   EXPECT_TRUE(transport_controller_
                   ->SetRemoteDescription(SdpType::kOffer, description.get())
                   .ok());
@@ -415,11 +433,70 @@ TEST_F(JsepTransportControllerTest, GetMediaTransportInCallee) {
 
   // After SetRemoteDescription, media transport should be created as callee.
   EXPECT_FALSE(media_transport->is_caller());
+  EXPECT_TRUE(media_transport->pre_shared_key().has_value());
 
   // Return nullptr for non-existing mids.
   EXPECT_EQ(nullptr, transport_controller_->GetMediaTransport(kVideoMid2));
 }
 
+TEST_F(JsepTransportControllerTest, GetMediaTransportIsNotSetIfNoSdes) {
+  FakeMediaTransportFactory fake_media_transport_factory;
+  JsepTransportController::Config config;
+
+  config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyNegotiate;
+  config.media_transport_factory = &fake_media_transport_factory;
+  CreateJsepTransportController(config);
+  auto description = CreateSessionDescriptionWithoutBundle();
+  EXPECT_TRUE(transport_controller_
+                  ->SetRemoteDescription(SdpType::kOffer, description.get())
+                  .ok());
+
+  EXPECT_EQ(nullptr, transport_controller_->GetMediaTransport(kAudioMid1));
+
+  // Even if we set local description with crypto now (after the remote offer
+  // was set), media transport won't be provided.
+  auto description2 = CreateSessionDescriptionWithoutBundle();
+  AddCryptoSettings(description2.get());
+  EXPECT_TRUE(transport_controller_
+                  ->SetLocalDescription(SdpType::kAnswer, description2.get())
+                  .ok());
+
+  EXPECT_EQ(nullptr, transport_controller_->GetMediaTransport(kAudioMid1));
+}
+
+TEST_F(JsepTransportControllerTest,
+       AfterSettingAnswerTheSameMediaTransportIsReturned) {
+  FakeMediaTransportFactory fake_media_transport_factory;
+  JsepTransportController::Config config;
+
+  config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyNegotiate;
+  config.media_transport_factory = &fake_media_transport_factory;
+  CreateJsepTransportController(config);
+  auto description = CreateSessionDescriptionWithoutBundle();
+  AddCryptoSettings(description.get());
+  EXPECT_TRUE(transport_controller_
+                  ->SetRemoteDescription(SdpType::kOffer, description.get())
+                  .ok());
+
+  FakeMediaTransport* media_transport = static_cast<FakeMediaTransport*>(
+      transport_controller_->GetMediaTransport(kAudioMid1));
+  EXPECT_NE(nullptr, media_transport);
+  EXPECT_TRUE(media_transport->pre_shared_key().has_value());
+
+  // Even if we set local description with crypto now (after the remote offer
+  // was set), media transport won't be provided.
+  auto description2 = CreateSessionDescriptionWithoutBundle();
+  AddCryptoSettings(description2.get());
+
+  RTCError result = transport_controller_->SetLocalDescription(
+      SdpType::kAnswer, description2.get());
+  EXPECT_TRUE(result.ok()) << result.message();
+
+  // Media transport did not change.
+  EXPECT_EQ(media_transport,
+            transport_controller_->GetMediaTransport(kAudioMid1));
+}
+
 TEST_F(JsepTransportControllerTest, SetIceConfig) {
   CreateJsepTransportController(JsepTransportController::Config());
   auto description = CreateSessionDescriptionWithoutBundle();

pc/peerconnection_media_unittest.cc

@@ -1092,6 +1092,9 @@ TEST_P(PeerConnectionMediaTest, MediaTransportPropagatedToVoiceEngine) {
   // Setup PeerConnection to use media transport.
   config.use_media_transport = true;
 
+  // Force SDES.
+  config.enable_dtls_srtp = false;
+
   auto caller = CreatePeerConnectionWithAudioVideo(config);
   auto callee = CreatePeerConnectionWithAudioVideo(config);
 

pc/srtpfilter.h

@@ -78,6 +78,10 @@ class SrtpFilter {
 
   bool ResetParams();
 
+  static bool ParseKeyParams(const std::string& params,
+                             uint8_t* key,
+                             size_t len);
+
   absl::optional<int> send_cipher_suite() { return send_cipher_suite_; }
   absl::optional<int> recv_cipher_suite() { return recv_cipher_suite_; }
 
@@ -104,10 +108,6 @@ class SrtpFilter {
 
   bool ApplyRecvParams(const CryptoParams& recv_params);
 
-  static bool ParseKeyParams(const std::string& params,
-                             uint8_t* key,
-                             size_t len);
-
   enum State {
     ST_INIT,                    // SRTP filter unused.
     ST_SENTOFFER,               // Offer with SRTP parameters sent.