From 63a176b34f5fced24b7f8c80b4221436c2a3fce3 Mon Sep 17 00:00:00 2001 From: "Piotr (Peter) Slatala" Date: Fri, 25 Jan 2019 08:25:33 -0800 Subject: [PATCH] Do not modify media transport config when falling back to RTP It is possible that media transport is re-set by the caller, but once disabled it should stay disabled. it's possible to fail this check the check in JsepTransportController::SetMediaTransportFactory in such case. We should also change the caller to not invoke SetMediaTransportFactory multiple times (with the same value), but I'll leave it as an excercise to someone else :) Bug: webrtc:9719 Change-Id: Ideea8a50d863edf4ef59e594a78c74bb9aba5aa7 Reviewed-on: https://webrtc-review.googlesource.com/c/119911 Reviewed-by: Steve Anton Reviewed-by: Bjorn Mellem Commit-Queue: Peter Slatala Cr-Commit-Position: refs/heads/master@{#26411} --- pc/jsep_transport_controller.cc | 137 ++++++++++++++++---------------- pc/jsep_transport_controller.h | 6 ++ 2 files changed, 76 insertions(+), 67 deletions(-) diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index a7d1fd7e91..084cec5a34 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -433,7 +433,7 @@ JsepTransportController::CreateDtlsTransport( RTC_DCHECK(network_thread_->IsCurrent()); std::unique_ptr dtls; - if (config_.media_transport_factory) { + if (is_media_transport_factory_enabled_ && config_.media_transport_factory) { dtls = absl::make_unique( std::move(ice), config_.crypto_options); } else if (config_.external_transport_factory) { @@ -947,6 +947,13 @@ JsepTransportController::MaybeCreateMediaTransport( const cricket::ContentInfo& content_info, bool local, cricket::IceTransportInternal* ice_transport) { + if (!is_media_transport_factory_enabled_) { + return nullptr; + } + if (config_.media_transport_factory == nullptr) { + return nullptr; + } + absl::optional selected_crypto_for_media_transport; if (content_info.media_description() && !content_info.media_description()->cryptos().empty()) { @@ -958,75 +965,71 @@ JsepTransportController::MaybeCreateMediaTransport( content_info.media_description()->cryptos()[0]; } - if (config_.media_transport_factory != nullptr) { - 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; + 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; - // 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 key_derivation = - rtc::KeyDerivation::Create(rtc::KeyDerivationAlgorithm::HKDF_SHA256); - const std::string label = "MediaTransportLabel"; - constexpr int kDerivedKeyByteSize = 32; - - 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 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> key = - key_derivation->DeriveKey( - raw_key, - /*salt=*/nullptr, - rtc::ArrayView( - reinterpret_cast(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(key.value().data()), - key.value().size()); - settings.event_log = config_.event_log; - auto media_transport_result = - config_.media_transport_factory->CreateMediaTransport( - ice_transport, network_thread_, settings); - - // TODO(sukhanov): Proper error handling. - RTC_CHECK(media_transport_result.ok()); - - return media_transport_result.MoveValue(); - } + // 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). + is_media_transport_factory_enabled_ = false; + return nullptr; } - return nullptr; + // 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 key_derivation = + rtc::KeyDerivation::Create(rtc::KeyDerivationAlgorithm::HKDF_SHA256); + const std::string label = "MediaTransportLabel"; + constexpr int kDerivedKeyByteSize = 32; + + 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 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> key = + key_derivation->DeriveKey( + raw_key, + /*salt=*/nullptr, + rtc::ArrayView( + reinterpret_cast(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(key.value().data()), key.value().size()); + settings.event_log = config_.event_log; + auto media_transport_result = + config_.media_transport_factory->CreateMediaTransport( + ice_transport, network_thread_, settings); + + // TODO(sukhanov): Proper error handling. + RTC_CHECK(media_transport_result.ok()); + + return media_transport_result.MoveValue(); } RTCError JsepTransportController::MaybeCreateJsepTransport( diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index e160be3cfc..08dea61414 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -358,6 +358,12 @@ class JsepTransportController : public sigslot::has_slots<> { cricket::IceGatheringState ice_gathering_state_ = cricket::kIceGatheringNew; Config config_; + // Determines if Config::media_transport_factory should be used + // to create a media transport. (when falling back to RTP this may be false). + // This is a prerequisite, but is not sufficient to create media transport + // (the factory needs to be provided in the config, and config must allow for + // media transport). + bool is_media_transport_factory_enabled_ = true; const cricket::SessionDescription* local_desc_ = nullptr; const cricket::SessionDescription* remote_desc_ = nullptr; absl::optional initial_offerer_;