Bug 1498068: fixed SRTP key length assertion for GCM 128 bit. r=mt
authorNils Ohlmeier [:drno] <drno@ohlmeier.org>
Tue, 16 Oct 2018 03:30:57 +0000
changeset 499943 62f5e5f579e7dddadf97483f4df5db093ae3e4eb
parent 499942 9d781037111bc5c77a643be0ae23188117a71cef
child 499944 3361fcc40ea294df9eed70a218c0eafe11bf60a7
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmt
bugs1498068
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1498068: fixed SRTP key length assertion for GCM 128 bit. r=mt Differential Revision: https://phabricator.services.mozilla.com/D8324
media/mtransport/SrtpFlow.cpp
media/mtransport/SrtpFlow.h
media/mtransport/test/transport_unittests.cpp
media/mtransport/transportlayerdtls.cpp
media/mtransport/transportlayerdtls.h
media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
--- a/media/mtransport/SrtpFlow.cpp
+++ b/media/mtransport/SrtpFlow.cpp
@@ -48,17 +48,17 @@ RefPtr<SrtpFlow> SrtpFlow::Create(int ci
 
   if (!key) {
     MOZ_MTLOG(ML_ERROR, "Null SRTP key specified");
     return nullptr;
   }
 
   if ((key_len > SRTP_MAX_KEY_LENGTH) ||
       (key_len < SRTP_MIN_KEY_LENGTH)) {
-    MOZ_MTLOG(ML_ERROR, "Invalid SRTP key length");
+    MOZ_ASSERT(false, "Invalid SRTP key length");
     return nullptr;
   }
 
   srtp_policy_t policy;
   memset(&policy, 0, sizeof(srtp_policy_t));
 
   // Note that we set the same cipher suite for RTP and RTCP
   // since any flow can only have one cipher suite with DTLS-SRTP
--- a/media/mtransport/SrtpFlow.h
+++ b/media/mtransport/SrtpFlow.h
@@ -10,23 +10,27 @@
 #include "mozilla/RefPtr.h"
 #include "nsISupportsImpl.h"
 #include "srtp.h"
 
 namespace mozilla {
 
 #define SRTP_ICM_MASTER_KEY_LENGTH 16
 #define SRTP_ICM_MASTER_SALT_LENGTH 14
-#define SRTP_ICM_MAX_MASTER_LEGNTH (SRTP_ICM_MASTER_KEY_LENGTH + SRTP_ICM_MASTER_SALT_LENGTH)
-#define SRTP_GCM_MASTER_KEY_LENGTH 32
+#define SRTP_ICM_MAX_MASTER_LENGTH (SRTP_ICM_MASTER_KEY_LENGTH + SRTP_ICM_MASTER_SALT_LENGTH)
+
+#define SRTP_GCM_MASTER_KEY_MIN_LENGTH 16
+#define SRTP_GCM_MASTER_KEY_MAX_LENGTH 32
 #define SRTP_GCM_MASTER_SALT_LENGTH 12
-#define SRTP_GCM_MAX_MASTER_LEGNTH (SRTP_GCM_MASTER_KEY_LENGTH + SRTP_GCM_MASTER_SALT_LENGTH)
 
-#define SRTP_MIN_KEY_LENGTH SRTP_ICM_MAX_MASTER_LEGNTH 
-#define SRTP_MAX_KEY_LENGTH SRTP_GCM_MAX_MASTER_LEGNTH 
+#define SRTP_GCM_MIN_MASTER_LENGTH (SRTP_GCM_MASTER_KEY_MIN_LENGTH + SRTP_GCM_MASTER_SALT_LENGTH)
+#define SRTP_GCM_MAX_MASTER_LENGTH (SRTP_GCM_MASTER_KEY_MAX_LENGTH + SRTP_GCM_MASTER_SALT_LENGTH)
+
+#define SRTP_MIN_KEY_LENGTH SRTP_GCM_MIN_MASTER_LENGTH 
+#define SRTP_MAX_KEY_LENGTH SRTP_GCM_MAX_MASTER_LENGTH 
 
 // SRTCP requires an auth tag *plus* a 4-byte index-plus-'E'-bit value (see
 // RFC 3711)
 #define SRTP_MAX_EXPANSION (SRTP_MAX_TRAILER_LEN+4)
 
 
 class SrtpFlow {
   ~SrtpFlow();
--- a/media/mtransport/test/transport_unittests.cpp
+++ b/media/mtransport/test/transport_unittests.cpp
@@ -530,23 +530,17 @@ class TransportTestPeer : public sigslot
 
       ASSERT_TRUE(NS_SUCCEEDED(res));
 
       mask <<= 1;
     }
   }
 
   void SetupSrtp() {
-    // this mimics the setup we do elsewhere
-    std::vector<uint16_t> srtp_ciphers;
-    srtp_ciphers.push_back(kDtlsSrtpAeadAes256Gcm);
-    srtp_ciphers.push_back(kDtlsSrtpAeadAes128Gcm);
-    srtp_ciphers.push_back(kDtlsSrtpAes128CmHmacSha1_80);
-    srtp_ciphers.push_back(kDtlsSrtpAes128CmHmacSha1_32);
-
+    std::vector<uint16_t> srtp_ciphers = TransportLayerDtls::GetDefaultSrtpCiphers();
     SetSrtpCiphers(srtp_ciphers);
  }
 
   void SetSrtpCiphers(std::vector<uint16_t>& srtp_ciphers) {
     ASSERT_TRUE(NS_SUCCEEDED(dtls_->SetSrtpCiphers(srtp_ciphers)));
   }
 
   void ConnectSocket_s(TransportTestPeer *peer) {
@@ -975,17 +969,16 @@ class TransportTest : public MtransportT
   }
 
   PRFileDesc *fds_[2];
   TransportTestPeer *p1_;
   TransportTestPeer *p2_;
   nsCOMPtr<nsIEventTarget> target_;
 };
 
-
 TEST_F(TransportTest, TestNoDtlsVerificationSettings) {
   ConnectSocketExpectFail();
 }
 
 static void DisableChaCha(TransportTestPeer* peer) {
   // On ARM, ChaCha20Poly1305 might be preferred; disable it for the tests that
   // want to check the cipher suite.  It doesn't matter which peer disables the
   // suite, disabling on either side has the same effect.
@@ -1010,18 +1003,18 @@ TEST_F(TransportTest, TestConnect) {
 TEST_F(TransportTest, TestConnectSrtp) {
   SetupSrtp();
   SetDtlsPeer();
   DisableChaCha(p2_);
   ConnectSocket();
 
   ASSERT_EQ(TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, p1_->cipherSuite());
 
-  // SRTP is on
-  ASSERT_EQ(kDtlsSrtpAeadAes256Gcm, p1_->srtpCipher());
+  // SRTP is on with default value
+  ASSERT_EQ(kDtlsSrtpAeadAes128Gcm, p1_->srtpCipher());
 }
 
 
 TEST_F(TransportTest, TestConnectDestroyFlowsMainThread) {
   SetDtlsPeer();
   ConnectSocket();
   DestroyPeerFlows();
 }
@@ -1403,30 +1396,39 @@ TEST_F(TransportTest, OnlyClientSendsSrt
   SetDtlsPeer();
   // This means that the server won't semd the extension as well.  The server
   // (p1) thinks that everything is OK.  The client (p2) notices the problem
   // after connecting and aborts.
   ConnectSocketExpectState(TransportLayer::TS_CLOSED,
                            TransportLayer::TS_ERROR);
 }
 
-TEST_F(TransportTest, TestSrtpFallback) {
-  std::vector<uint16_t> setA;
-  setA.push_back(kDtlsSrtpAeadAes256Gcm);
-  setA.push_back(kDtlsSrtpAes128CmHmacSha1_80);
+class TransportSrtpParameterTest : public TransportTest,
+                                   public ::testing::WithParamInterface<uint16_t> {
+};
+
+INSTANTIATE_TEST_CASE_P(SrtpParamInit,
+                        TransportSrtpParameterTest,
+                        ::testing::ValuesIn(TransportLayerDtls::GetDefaultSrtpCiphers()));
+
+TEST_P(TransportSrtpParameterTest, TestSrtpCiphersMismatchCombinations) {
+  uint16_t cipher = GetParam();
+  std::cerr << "Checking cipher: " << cipher << std::endl;
+
+  p1_->SetupSrtp();
+
   std::vector<uint16_t> setB;
-  setB.push_back(kDtlsSrtpAes128CmHmacSha1_80);
+  setB.push_back(cipher);
 
-  p1_->SetSrtpCiphers(setA);
   p2_->SetSrtpCiphers(setB);
   SetDtlsPeer();
   ConnectSocket();
 
-  ASSERT_EQ(kDtlsSrtpAes128CmHmacSha1_80, p1_->srtpCipher());
-  ASSERT_EQ(kDtlsSrtpAes128CmHmacSha1_80, p1_->srtpCipher());
+  ASSERT_EQ(cipher, p1_->srtpCipher());
+  ASSERT_EQ(cipher, p2_->srtpCipher());
 }
 
 // NSS doesn't support DHE suites on the server end.
 // This checks to see if we barf when that's the only option available.
 TEST_F(TransportTest, TestDheOnlyFails) {
   SetDtlsPeer();
 
   // p2_ is the client
--- a/media/mtransport/transportlayerdtls.cpp
+++ b/media/mtransport/transportlayerdtls.cpp
@@ -804,16 +804,29 @@ nsresult TransportLayerDtls::GetCipherSu
   if (rv != SECSuccess) {
     MOZ_MTLOG(ML_NOTICE, LAYER_INFO << "GetCipherSuite can't get channel info");
     return NS_ERROR_FAILURE;
   }
   *cipherSuite = info.cipherSuite;
   return NS_OK;
 }
 
+std::vector<uint16_t> TransportLayerDtls::GetDefaultSrtpCiphers() {
+  std::vector<uint16_t> ciphers;
+
+  ciphers.push_back(kDtlsSrtpAeadAes128Gcm);
+  // Since we don't support DTLS 1.3 or SHA384 ciphers (see bug 1312976)
+  // we don't really enough entropy to prefer this over 128 bit
+  ciphers.push_back(kDtlsSrtpAeadAes256Gcm);
+  ciphers.push_back(kDtlsSrtpAes128CmHmacSha1_80);
+  ciphers.push_back(kDtlsSrtpAes128CmHmacSha1_32);
+
+  return ciphers;
+}
+
 void TransportLayerDtls::StateChange(TransportLayer *layer, State state) {
   if (state <= state_) {
     MOZ_MTLOG(ML_ERROR, "Lower layer state is going backwards from ours");
     TL_SET_STATE(TS_ERROR);
     return;
   }
 
   switch (state) {
--- a/media/mtransport/transportlayerdtls.h
+++ b/media/mtransport/transportlayerdtls.h
@@ -82,16 +82,17 @@ class TransportLayerDtls final : public 
   nsresult SetVerificationDigest(const std::string digest_algorithm,
                                  const unsigned char *digest_value,
                                  size_t digest_len);
 
   nsresult GetCipherSuite(uint16_t* cipherSuite) const;
 
   nsresult SetSrtpCiphers(const std::vector<uint16_t>& ciphers);
   nsresult GetSrtpCipher(uint16_t *cipher) const;
+  static std::vector<uint16_t> GetDefaultSrtpCiphers();
 
   nsresult ExportKeyingMaterial(const std::string& label,
                                 bool use_context,
                                 const std::string& context,
                                 unsigned char *out,
                                 unsigned int outlen);
 
   // Transport layer overrides.
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ -733,21 +733,17 @@ PeerConnectionMedia::UpdateTransportFlow
     rv = dtls->SetVerificationDigest(ss.str(), &fingerprint.fingerprint[0],
                                      fingerprint.fingerprint.size());
     if (NS_FAILED(rv)) {
       CSFLogError(LOGTAG, "Could not set fingerprint");
       return rv;
     }
   }
 
-  std::vector<uint16_t> srtpCiphers;
-  srtpCiphers.push_back(kDtlsSrtpAeadAes256Gcm);
-  srtpCiphers.push_back(kDtlsSrtpAeadAes128Gcm);
-  srtpCiphers.push_back(kDtlsSrtpAes128CmHmacSha1_80);
-  srtpCiphers.push_back(kDtlsSrtpAes128CmHmacSha1_32);
+  std::vector<uint16_t> srtpCiphers = TransportLayerDtls::GetDefaultSrtpCiphers();
 
   rv = dtls->SetSrtpCiphers(srtpCiphers);
   if (NS_FAILED(rv)) {
     CSFLogError(LOGTAG, "Couldn't set SRTP ciphers");
     return rv;
   }
 
   // Always permits negotiation of the confidential mode.