Add support for GCM cipher suites from RFC 7714.

webrtc/pc/mediasession.cc

Index: webrtc/pc/mediasession.cc
diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc
index ea0eaa26836055c1a7c88f94898f2654894bfca1..e40b4646115b8ca4979033708275d00abc36dc43 100644
--- a/webrtc/pc/mediasession.cc
+++ b/webrtc/pc/mediasession.cc
@@ -36,11 +36,13 @@ static const uint32_t kMaxSctpSid = 1023;
 namespace {
 const char kInline[] = "inline:";
 
-void GetSupportedCryptoSuiteNames(void (*func)(std::vector<int>*),
+void GetSupportedCryptoSuiteNames(void (*func)(const rtc::CryptoOptions&,
+                                      std::vector<int>*),
+                                  const rtc::CryptoOptions& crypto_options,
                                   std::vector<std::string>* names) {
 #ifdef HAVE_SRTP
   std::vector<int> crypto_suites;
-  func(&crypto_suites);
+  func(crypto_options, &crypto_suites);
   for (const auto crypto : crypto_suites) {
     names->push_back(rtc::SrtpCryptoSuiteToName(crypto));
   }
@@ -82,10 +84,19 @@ static bool IsMediaContentOfType(const ContentInfo* content,
 
 static bool CreateCryptoParams(int tag, const std::string& cipher,
                                CryptoParams *out) {
+  int key_len;
+  int salt_len;
+  if (!rtc::GetSrtpKeyAndSaltLengths(
+      rtc::SrtpCryptoSuiteFromName(cipher), &key_len, &salt_len)) {
+    return false;
+  }
+
+  int master_key_base64_len = (key_len + salt_len) * 4 / 3;

[mattdr, 2016/05/06 22:34:14]

This is bad.

E.g.
SRTP_AEAD_AES_256_GCM has key_len = 32 bytes and salt_len = 12 bytes. Total is
44 bytes, which requires at least 59 characters to encode in base64 (not
including padding), but we round down to 58.

https://tools.ietf.org/html/rfc4568#section-6.1

   If the length (after being decoded
   from base64) does not match that specified for the crypto-suite, the
   crypto attribute in question MUST be considered invalid.  Each master
   key and salt MUST be a cryptographically random number and MUST be
   unique to the entire SDP message.  When base64 decoding the key and
   salt, padding characters (i.e., one or two "=" at the end of the
   base64-encoded data) are discarded (see [RFC3548] for details).
   Base64 encoding assumes that the base64 encoding input is an integral
   number of octets.  If a given crypto-suite requires the use of a
   concatenated key and salt with a length that is not an integral
   number of octets, said crypto-suite MUST define a padding scheme that
   results in the base64 input being an integral number of octets.  For
   example, if the length defined were 250 bits, then 6 padding bits
   would be needed, which could be defined to be the last 6 bits in a
   256 bit input.

Much more troubling -- the unit tests *cover* AES-256-GCM but *pass*. Something
is wrong with the parsing code (SrtpFilter::ParseKeyParams) or its callers if
this didn't blow up.

[joachim, 2016/05/09 23:21:40]

Right, and sorry for missing this :-( I now changed it to generate binary random
data with the required length and then encode to base64.
The reason why this was not caught by my added tests is that for DTLS-SRTP the
key is extracted from the underlying channel (see "BaseChannel::SetupDtlsSrtp")
which directly returns the binary key.

Also none of my added tests enabled Gcm for cases where keys are read from a SDP
(this codepath is only executed for such keys).

I added "WebRtcSessionTest.VerifyCryptoParamsInSDPGcm" to check the length of
encoded keys in the SDP but I'm not sure if this is the best place to verify
this or if another test should be added somewhere else.

Other tests were added to srtpfilter_unittest.cc together with a fix in
"SrtpFilter::ApplyParams".

+
   std::string key;
-  key.reserve(SRTP_MASTER_KEY_BASE64_LEN);
+  key.reserve(master_key_base64_len);
 
-  if (!rtc::CreateRandomString(SRTP_MASTER_KEY_BASE64_LEN, &key)) {
+  if (!rtc::CreateRandomString(master_key_base64_len, &key)) {
     return false;
   }
   out->tag = tag;
@@ -146,63 +157,80 @@ bool FindMatchingCrypto(const CryptoParamsVec& cryptos,
   return false;
 }
 
-// For audio, HMAC 32 is prefered because of the low overhead.
-void GetSupportedAudioCryptoSuites(std::vector<int>* crypto_suites) {
+// For audio, HMAC 32 is prefered over HMAC 80 because of the low overhead.
+void GetSupportedAudioCryptoSuites(const rtc::CryptoOptions& crypto_options,
+    std::vector<int>* crypto_suites) {
 #ifdef HAVE_SRTP
+  if (crypto_options.enable_gcm_crypto_suites) {
+    crypto_suites->push_back(rtc::SRTP_AEAD_AES_256_GCM);

[mattdr, 2016/05/06 22:47:52]

Judging by the comments above, I'm inferring these ciphers are listed in
decreasing order of priority, i.e. we'll prefer AES-256-GCM to AES-128-GCM.

Is there an RFC or other explicit recommendation on which we should prefer?

[joachim, 2016/05/09 23:21:41]

No particular reason from my side. I didn't find anything special recommendation
regarding SRTP and GCM, but Mozilla prioritizes AES-256 over -128 for the
webserver setup
(https://wiki.mozilla.org/Security/Server_Side_TLS#Modern_compatibility), so I
decided to use the same ordering here.

+    crypto_suites->push_back(rtc::SRTP_AEAD_AES_128_GCM);
+  }
   crypto_suites->push_back(rtc::SRTP_AES128_CM_SHA1_32);
   crypto_suites->push_back(rtc::SRTP_AES128_CM_SHA1_80);
 #endif
 }
 
-void GetSupportedAudioCryptoSuiteNames(
+void GetSupportedAudioCryptoSuiteNames(const rtc::CryptoOptions& crypto_options,
     std::vector<std::string>* crypto_suite_names) {
   GetSupportedCryptoSuiteNames(GetSupportedAudioCryptoSuites,
-                               crypto_suite_names);
+                               crypto_options, crypto_suite_names);
 }
 
-void GetSupportedVideoCryptoSuites(std::vector<int>* crypto_suites) {
-  GetDefaultSrtpCryptoSuites(crypto_suites);
+void GetSupportedVideoCryptoSuites(const rtc::CryptoOptions& crypto_options,
+    std::vector<int>* crypto_suites) {
+  GetDefaultSrtpCryptoSuites(crypto_options, crypto_suites);
 }
 
-void GetSupportedVideoCryptoSuiteNames(
+void GetSupportedVideoCryptoSuiteNames(const rtc::CryptoOptions& crypto_options,
     std::vector<std::string>* crypto_suite_names) {
   GetSupportedCryptoSuiteNames(GetSupportedVideoCryptoSuites,
-                               crypto_suite_names);
+                               crypto_options, crypto_suite_names);
 }
 
-void GetSupportedDataCryptoSuites(std::vector<int>* crypto_suites) {
-  GetDefaultSrtpCryptoSuites(crypto_suites);
+void GetSupportedDataCryptoSuites(const rtc::CryptoOptions& crypto_options,
+    std::vector<int>* crypto_suites) {
+  GetDefaultSrtpCryptoSuites(crypto_options, crypto_suites);
 }
 
-void GetSupportedDataCryptoSuiteNames(
+void GetSupportedDataCryptoSuiteNames(const rtc::CryptoOptions& crypto_options,
     std::vector<std::string>* crypto_suite_names) {
   GetSupportedCryptoSuiteNames(GetSupportedDataCryptoSuites,
-                               crypto_suite_names);
+                               crypto_options, crypto_suite_names);
 }
 
-void GetDefaultSrtpCryptoSuites(std::vector<int>* crypto_suites) {
+void GetDefaultSrtpCryptoSuites(const rtc::CryptoOptions& crypto_options,
+    std::vector<int>* crypto_suites) {
 #ifdef HAVE_SRTP
+  if (crypto_options.enable_gcm_crypto_suites) {
+    crypto_suites->push_back(rtc::SRTP_AEAD_AES_256_GCM);
+    crypto_suites->push_back(rtc::SRTP_AEAD_AES_128_GCM);
+  }
   crypto_suites->push_back(rtc::SRTP_AES128_CM_SHA1_80);
 #endif
 }
 
-void GetDefaultSrtpCryptoSuiteNames(
+void GetDefaultSrtpCryptoSuiteNames(const rtc::CryptoOptions& crypto_options,
     std::vector<std::string>* crypto_suite_names) {
-  GetSupportedCryptoSuiteNames(GetDefaultSrtpCryptoSuites, crypto_suite_names);
+  GetSupportedCryptoSuiteNames(GetDefaultSrtpCryptoSuites,
+                               crypto_options, crypto_suite_names);
 }
 
-// For video support only 80-bit SHA1 HMAC. For audio 32-bit HMAC is
-// tolerated unless bundle is enabled because it is low overhead. Pick the
-// crypto in the list that is supported.
+// Support any GCM cipher (if enabled through options). For video support only
+// 80-bit SHA1 HMAC. For audio 32-bit HMAC is tolerated unless bundle is enabled
+// because it is low overhead.
+// Pick the crypto in the list that is supported.
 static bool SelectCrypto(const MediaContentDescription* offer,
                          bool bundle,
+                         const rtc::CryptoOptions& crypto_options,
                          CryptoParams *crypto) {
   bool audio = offer->type() == MEDIA_TYPE_AUDIO;
   const CryptoParamsVec& cryptos = offer->cryptos();
 
   for (CryptoParamsVec::const_iterator i = cryptos.begin();
        i != cryptos.end(); ++i) {
-    if (rtc::CS_AES_CM_128_HMAC_SHA1_80 == i->cipher_suite ||
+    if ((crypto_options.enable_gcm_crypto_suites &&
+        rtc::IsGcmCryptoSuiteName(i->cipher_suite)) ||

[mattdr, 2016/05/06 22:34:14]

one more space on this line to align with the begin of the paren expression
above

[joachim, 2016/05/09 23:21:40]

Done.

+        rtc::CS_AES_CM_128_HMAC_SHA1_80 == i->cipher_suite ||
         (rtc::CS_AES_CM_128_HMAC_SHA1_32 == i->cipher_suite && audio &&
          !bundle)) {
       return CreateCryptoParams(i->tag, i->cipher_suite, crypto);
@@ -1067,7 +1095,7 @@ static bool CreateMediaContentAnswer(
 
   if (sdes_policy != SEC_DISABLED) {
     CryptoParams crypto;
-    if (SelectCrypto(offer, bundle_enabled, &crypto)) {
+    if (SelectCrypto(offer, bundle_enabled, options.crypto_options, &crypto)) {
       if (current_cryptos) {
         FindMatchingCrypto(*current_cryptos, crypto, &crypto);
       }
@@ -1583,7 +1611,7 @@ bool MediaSessionDescriptionFactory::AddAudioContentForOffer(
 
   std::unique_ptr<AudioContentDescription> audio(new AudioContentDescription());
   std::vector<std::string> crypto_suites;
-  GetSupportedAudioCryptoSuiteNames(&crypto_suites);
+  GetSupportedAudioCryptoSuiteNames(options.crypto_options, &crypto_suites);
   if (!CreateMediaContentOffer(
           options,
           audio_codecs,
@@ -1643,7 +1671,7 @@ bool MediaSessionDescriptionFactory::AddVideoContentForOffer(
 
   std::unique_ptr<VideoContentDescription> video(new VideoContentDescription());
   std::vector<std::string> crypto_suites;
-  GetSupportedVideoCryptoSuiteNames(&crypto_suites);
+  GetSupportedVideoCryptoSuiteNames(options.crypto_options, &crypto_suites);
   if (!CreateMediaContentOffer(
           options,
           video_codecs,
@@ -1719,7 +1747,7 @@ bool MediaSessionDescriptionFactory::AddDataContentForOffer(
     data->set_protocol(
         secure_transport ? kMediaProtocolDtlsSctp : kMediaProtocolSctp);
   } else {
-    GetSupportedDataCryptoSuiteNames(&crypto_suites);
+    GetSupportedDataCryptoSuiteNames(options.crypto_options, &crypto_suites);
   }
 
   if (!CreateMediaContentOffer(