From c2bbe4b952f6130a84fc8cd7b6bed4f0beb05770 Mon Sep 17 00:00:00 2001 From: Manashi Sarkar Date: Thu, 28 Sep 2023 14:37:22 +0000 Subject: [PATCH] Revert "Enable SRTP GCM ciphers by default" This reverts commit d8633868b34dc1d841f0a9fd1afe2bc22aa8bde6. Reason for revert: Breaks downstream project. Original change's description: > Enable SRTP GCM ciphers by default > > Bug: webrtc:15178 > Change-Id: I0216ce8f194fffc820723d82b9c04a76573c2f4f > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/305381 > Reviewed-by: Harald Alvestrand > Commit-Queue: Philipp Hancke > Reviewed-by: Victor Boivie > Cr-Commit-Position: refs/heads/main@{#40828} Bug: webrtc:15178 Change-Id: I88433e899cb4b705eafa3fceff3edc520629f603 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/321863 Owners-Override: Artem Titov Auto-Submit: Manashi Sarkar Commit-Queue: Artem Titov Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Reviewed-by: Artem Titov Cr-Commit-Position: refs/heads/main@{#40832} --- api/crypto/crypto_options.cc | 7 +++++++ api/crypto/crypto_options.h | 7 ++++++- api/peer_connection_interface.h | 2 +- pc/media_session_unittest.cc | 25 +++++++++++++------------ 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/api/crypto/crypto_options.cc b/api/crypto/crypto_options.cc index 6fe0540319..22c5dd464b 100644 --- a/api/crypto/crypto_options.cc +++ b/api/crypto/crypto_options.cc @@ -23,6 +23,13 @@ CryptoOptions::CryptoOptions(const CryptoOptions& other) { CryptoOptions::~CryptoOptions() {} +// static +CryptoOptions CryptoOptions::NoGcm() { + CryptoOptions options; + options.srtp.enable_gcm_crypto_suites = false; + return options; +} + std::vector CryptoOptions::GetSupportedDtlsSrtpCryptoSuites() const { std::vector crypto_suites; // Note: kSrtpAes128CmSha1_80 is what is required to be supported (by diff --git a/api/crypto/crypto_options.h b/api/crypto/crypto_options.h index b38ea27a42..317995134a 100644 --- a/api/crypto/crypto_options.h +++ b/api/crypto/crypto_options.h @@ -25,6 +25,11 @@ struct RTC_EXPORT CryptoOptions { CryptoOptions(const CryptoOptions& other); ~CryptoOptions(); + // Helper method to return an instance of the CryptoOptions with GCM crypto + // suites disabled. This method should be used instead of depending on current + // default values set by the constructor. + static CryptoOptions NoGcm(); + // Returns a list of the supported DTLS-SRTP Crypto suites based on this set // of crypto options. std::vector GetSupportedDtlsSrtpCryptoSuites() const; @@ -36,7 +41,7 @@ struct RTC_EXPORT CryptoOptions { struct Srtp { // Enable GCM crypto suites from RFC 7714 for SRTP. GCM will only be used // if both sides enable it. - bool enable_gcm_crypto_suites = true; + bool enable_gcm_crypto_suites = false; // If set to true, the (potentially insecure) crypto cipher // kSrtpAes128CmSha1_32 will be included in the list of supported ciphers diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index e2f284e25e..37dcfbbc27 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -1493,7 +1493,7 @@ class RTC_EXPORT PeerConnectionFactoryInterface rtc::SSLProtocolVersion ssl_max_version = rtc::SSL_PROTOCOL_DTLS_12; // Sets crypto related options, e.g. enabled cipher suites. - CryptoOptions crypto_options = {}; + CryptoOptions crypto_options = CryptoOptions::NoGcm(); }; // Set the options to be used for subsequently created PeerConnections. diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index aaf2183f0e..753e8cd9c7 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -284,7 +284,6 @@ static const char* kMediaProtocolsDtls[] = { // default changes. static const char* kDefaultSrtpCryptoSuite = kCsAesCm128HmacSha1_80; static const char* kDefaultSrtpCryptoSuiteGcm = kCsAeadAes256Gcm; -static const uint8_t kDefaultCryptoSuiteSize = 3U; // These constants are used to make the code using "AddMediaDescriptionOptions" // more readable. @@ -623,8 +622,9 @@ class MediaSessionDescriptionFactoryTest : public ::testing::Test { ASSERT_TRUE(video_media_desc); EXPECT_TRUE(CompareCryptoParams(audio_media_desc->cryptos(), video_media_desc->cryptos())); - ASSERT_CRYPTO(audio_media_desc, offer ? kDefaultCryptoSuiteSize : 1U, - kDefaultSrtpCryptoSuite); + EXPECT_EQ(1u, audio_media_desc->cryptos().size()); + EXPECT_EQ(kDefaultSrtpCryptoSuite, + audio_media_desc->cryptos()[0].crypto_suite); // Verify the selected crypto is one from the reference audio // media content. @@ -819,7 +819,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioOffer) { EXPECT_EQ(0U, acd->first_ssrc()); // no sender is attached. EXPECT_EQ(kAutoBandwidth, acd->bandwidth()); // default bandwidth (auto) EXPECT_TRUE(acd->rtcp_mux()); // rtcp-mux defaults on - ASSERT_CRYPTO(acd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite); + ASSERT_CRYPTO(acd, 1U, kDefaultSrtpCryptoSuite); EXPECT_EQ(cricket::kMediaProtocolSavpf, acd->protocol()); } @@ -844,14 +844,14 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateVideoOffer) { EXPECT_EQ(0U, acd->first_ssrc()); // no sender is attached EXPECT_EQ(kAutoBandwidth, acd->bandwidth()); // default bandwidth (auto) EXPECT_TRUE(acd->rtcp_mux()); // rtcp-mux defaults on - ASSERT_CRYPTO(acd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite); + ASSERT_CRYPTO(acd, 1U, kDefaultSrtpCryptoSuite); EXPECT_EQ(cricket::kMediaProtocolSavpf, acd->protocol()); EXPECT_EQ(MEDIA_TYPE_VIDEO, vcd->type()); EXPECT_EQ(f1_.video_sendrecv_codecs(), vcd->codecs()); EXPECT_EQ(0U, vcd->first_ssrc()); // no sender is attached EXPECT_EQ(kAutoBandwidth, vcd->bandwidth()); // default bandwidth (auto) EXPECT_TRUE(vcd->rtcp_mux()); // rtcp-mux defaults on - ASSERT_CRYPTO(vcd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite); + ASSERT_CRYPTO(vcd, 1U, kDefaultSrtpCryptoSuite); EXPECT_EQ(cricket::kMediaProtocolSavpf, vcd->protocol()); } @@ -1298,6 +1298,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioAnswerGcm) { f1_.set_secure(SEC_ENABLED); f2_.set_secure(SEC_ENABLED); MediaSessionOptions opts = CreatePlanBMediaSessionOptions(); + opts.crypto_options.srtp.enable_gcm_crypto_suites = true; std::unique_ptr offer = f1_.CreateOfferOrError(opts, nullptr).MoveValue(); ASSERT_TRUE(offer.get()); @@ -2474,11 +2475,11 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateMultiStreamVideoOffer) { EXPECT_EQ(kAutoBandwidth, acd->bandwidth()); // default bandwidth (auto) EXPECT_TRUE(acd->rtcp_mux()); // rtcp-mux defaults on - ASSERT_CRYPTO(acd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite); + ASSERT_CRYPTO(acd, 1U, kDefaultSrtpCryptoSuite); EXPECT_EQ(MEDIA_TYPE_VIDEO, vcd->type()); EXPECT_EQ(f1_.video_sendrecv_codecs(), vcd->codecs()); - ASSERT_CRYPTO(vcd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite); + ASSERT_CRYPTO(vcd, 1U, kDefaultSrtpCryptoSuite); const StreamParamsVec& video_streams = vcd->streams(); ASSERT_EQ(1U, video_streams.size()); @@ -2511,9 +2512,9 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateMultiStreamVideoOffer) { EXPECT_EQ(acd->codecs(), updated_acd->codecs()); EXPECT_EQ(vcd->type(), updated_vcd->type()); EXPECT_EQ(vcd->codecs(), updated_vcd->codecs()); - ASSERT_CRYPTO(updated_acd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite); + ASSERT_CRYPTO(updated_acd, 1U, kDefaultSrtpCryptoSuite); EXPECT_TRUE(CompareCryptoParams(acd->cryptos(), updated_acd->cryptos())); - ASSERT_CRYPTO(updated_vcd, kDefaultCryptoSuiteSize, kDefaultSrtpCryptoSuite); + ASSERT_CRYPTO(updated_vcd, 1U, kDefaultSrtpCryptoSuite); EXPECT_TRUE(CompareCryptoParams(vcd->cryptos(), updated_vcd->cryptos())); const StreamParamsVec& updated_audio_streams = updated_acd->streams(); @@ -3880,8 +3881,8 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCryptoDtls) { ASSERT_TRUE(audio_media_desc); video_media_desc = offer->GetContentDescriptionByName("video"); ASSERT_TRUE(video_media_desc); - EXPECT_EQ(kDefaultCryptoSuiteSize, audio_media_desc->cryptos().size()); - EXPECT_EQ(kDefaultCryptoSuiteSize, video_media_desc->cryptos().size()); + EXPECT_EQ(1u, audio_media_desc->cryptos().size()); + EXPECT_EQ(1u, video_media_desc->cryptos().size()); audio_trans_desc = offer->GetTransportDescriptionByName("audio"); ASSERT_TRUE(audio_trans_desc);