Add IsAcceptableCipher, use instead of GetDefaultCipher.

webrtc/base/opensslstreamadapter.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 129 matching lines @@
   {0, NULL}
 };
 #endif  // #ifndef OPENSSL_IS_BORINGSSL
 
 #if defined(_MSC_VER)
 #pragma warning(push)
 #pragma warning(disable : 4309)
 #pragma warning(disable : 4310)
 #endif  // defined(_MSC_VER)
 
-// Default cipher used between OpenSSL/BoringSSL stream adapters.
-// This needs to be updated when the default of the SSL library changes.
-// static_cast<uint16_t> causes build warnings on windows platform.
-static int kDefaultSslCipher10 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_AES_256_CBC_SHA);
-static int kDefaultSslEcCipher10 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_ECDSA_WITH_AES_256_CBC_SHA);
-#ifdef OPENSSL_IS_BORINGSSL
-static int kDefaultSslCipher12 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256);
-static int kDefaultSslEcCipher12 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256);
-// Fallback cipher for DTLS 1.2 if hardware-accelerated AES-GCM is unavailable.
-
-#ifdef TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
-// This ciphersuite was added in boringssl 13414b3a..., changing the fallback
-// ciphersuite.  For compatibility during a transitional period, support old
-// boringssl versions.  TODO(torbjorng): Remove this.
-static int kDefaultSslCipher12NoAesGcm =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256);
-#else
-static int kDefaultSslCipher12NoAesGcm =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_CHACHA20_POLY1305_OLD);
-#endif
-
-#ifdef TLS1_CK_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
-// This ciphersuite was added in boringssl 13414b3a..., changing the fallback
-// ciphersuite.  For compatibility during a transitional period, support old
-// boringssl versions.  TODO(torbjorng): Remove this.
-static int kDefaultSslEcCipher12NoAesGcm =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256);
-#else
-static int kDefaultSslEcCipher12NoAesGcm =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_ECDSA_CHACHA20_POLY1305_OLD);
-#endif
-
-#else  // !OPENSSL_IS_BORINGSSL
-// OpenSSL sorts differently than BoringSSL, so the default cipher doesn't
-// change between TLS 1.0 and TLS 1.2 with the current setup.
-static int kDefaultSslCipher12 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_AES_256_CBC_SHA);
-static int kDefaultSslEcCipher12 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_ECDSA_WITH_AES_256_CBC_SHA);
-#endif  // OPENSSL_IS_BORINGSSL
-
 #if defined(_MSC_VER)
 #pragma warning(pop)
 #endif  // defined(_MSC_VER)
 
 //////////////////////////////////////////////////////////////////////
 // StreamBIO
 //////////////////////////////////////////////////////////////////////
 
 static int stream_write(BIO* h, const char* buf, int num);
 static int stream_read(BIO* h, char* buf, int size);
@@ skipping 935 matching lines @@
 }
 
 bool OpenSSLStreamAdapter::HaveExporter() {
 #ifdef HAVE_DTLS_SRTP
   return true;
 #else
   return false;
 #endif
 }
 
-int OpenSSLStreamAdapter::GetDefaultSslCipherForTest(SSLProtocolVersion version,
-                                                     KeyType key_type) {
+#define CDEF(X) \
+  { static_cast<uint16_t>(TLS1_CK_##X & 0xffff), "TLS_" #X }
+
+struct cipher_list {
+  uint16_t cipher;
+  const char* cipher_str;
+};
+
+// TODO(torbjorng): Perhaps add more cipher suites to these lists.
+static const cipher_list OK_RSA_ciphers[] = {
+  CDEF(ECDHE_RSA_WITH_AES_128_CBC_SHA),
+  CDEF(ECDHE_RSA_WITH_AES_256_CBC_SHA),
+  CDEF(ECDHE_RSA_WITH_AES_128_GCM_SHA256),
+#ifdef TLS1_CK_ECDHE_RSA_WITH_AES_256_GCM_SHA256
+  CDEF(ECDHE_RSA_WITH_AES_256_GCM_SHA256),
+#endif
+  CDEF(ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256),
+};
+
+static const cipher_list OK_ECDSA_ciphers[] = {
+  CDEF(ECDHE_ECDSA_WITH_AES_128_CBC_SHA),
+  CDEF(ECDHE_ECDSA_WITH_AES_256_CBC_SHA),
+  CDEF(ECDHE_ECDSA_WITH_AES_128_GCM_SHA256),
+#ifdef TLS1_CK_ECDHE_ECDSA_WITH_AES_256_GCM_SHA256
+  CDEF(ECDHE_ECDSA_WITH_AES_256_GCM_SHA256),
+#endif
+  CDEF(ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256),
+};
+
+bool OpenSSLStreamAdapter::IsAcceptableCipher(int cipher,
+                                              KeyType key_type) {

[davidben_webrtc, 2016/03/08 20:29:05]

What is the test actually trying to check? Are you trying to make sure the key
type was honored or...? Previously it seems you were somehow interested in the
protocol version, but this one isn't.

I'm asking because there may be more direct ways to check what you want. You
could assert on the actual protocol version, etc.

[torbjorng (webrtc), 2016/03/09 13:10:31]

Mainly I am trying to assert we got a reasonable cipher suite. Separating RSA
and ECDSA sanity checks the logic of our code (since we separate them there).

I think it makes sense to whitelist cipher suites like this, even at the price
of making our test code somewhat sensitive to boringssl default changes. Now the
only change we will need is to add a line or two in our whitelist. I'd be
interested if there is a better way.

You're right that the old code somewhat indirectly checked that the expected
protocol version was used. We should check that again, with a definitive check
(by means of SSL_version(const SSL *ssl), (which unfortunately is
BoringSSL-specific and we still pretend to support plain OpenSSL).

[davidben_webrtc, 2016/03/09 20:30:26]

I think it depends on what you're interested in checking. If you want to assert
something about the cipher suite, you're probably stuck with this. I suppose you
can query the certificate out of the SSL* and then check the key type there? I
dunno. I can get you an API to categorize ciphers one way or another, but it
won't work in OpenSSL.
SSL_version's been in OpenSSL since forever. :-)

[torbjorng (webrtc), 2016/03/10 21:29:03]

Thanks, let's consider that for the future, but stick with my solution for now.
(This CL was not planned, I need to move on.)

It is doubtful whether WebRTC can be compiled against plain OpenSSL. We
conditionalize for it, but none of the bots test that it still works, which
probably means it doesn't.
Right.  I realised that when I implemented the checks yesterday.

[davidben_webrtc, 2016/03/10 21:40:05]

Really? I've been under the impression you all need to support it. If not, that
would be wonderful!

   if (key_type == KT_RSA) {
-    switch (version) {
-      case SSL_PROTOCOL_TLS_10:
-      case SSL_PROTOCOL_TLS_11:
-        return kDefaultSslCipher10;
-      case SSL_PROTOCOL_TLS_12:
-      default:
-#ifdef OPENSSL_IS_BORINGSSL
-        if (EVP_has_aes_hardware()) {
-          return kDefaultSslCipher12;
-        } else {
-          return kDefaultSslCipher12NoAesGcm;
-        }
-#else  // !OPENSSL_IS_BORINGSSL
-        return kDefaultSslCipher12;
-#endif
+    for (const cipher_list &c : OK_RSA_ciphers) {
+      if (cipher == c.cipher)
+        return true;
     }
-  } else if (key_type == KT_ECDSA) {
-    switch (version) {
-      case SSL_PROTOCOL_TLS_10:
-      case SSL_PROTOCOL_TLS_11:
-        return kDefaultSslEcCipher10;
-      case SSL_PROTOCOL_TLS_12:
-      default:
-#ifdef OPENSSL_IS_BORINGSSL
-        if (EVP_has_aes_hardware()) {
-          return kDefaultSslEcCipher12;
-        } else {
-          return kDefaultSslEcCipher12NoAesGcm;
-        }
-#else  // !OPENSSL_IS_BORINGSSL
-        return kDefaultSslEcCipher12;
-#endif
+  }
+
+  if (key_type == KT_ECDSA) {
+    for (const cipher_list &c : OK_ECDSA_ciphers) {
+      if (cipher == c.cipher)
+        return true;
     }
-  } else {
-    RTC_NOTREACHED();
-    return kDefaultSslEcCipher12;
   }
+
+  // TODO(torbjorng): Remove before landing.
+  LOG(LS_ERROR) << "Attempted use of truly terrible cipher suite: "
+                << OpenSSLStreamAdapter::SslCipherSuiteToName(cipher) << "("
+                << cipher << ")";
+  return false;
+}
+
+bool OpenSSLStreamAdapter::IsAcceptableCipher(std::string cipher,
+                                              KeyType key_type) {
+  if (key_type == KT_RSA) {
+    for (const cipher_list &c : OK_RSA_ciphers) {
+      if (cipher == c.cipher_str)
+        return true;
+    }
+  }
+
+  if (key_type == KT_ECDSA) {
+    for (const cipher_list &c : OK_ECDSA_ciphers) {
+      if (cipher == c.cipher_str)
+        return true;
+    }
+  }
+
+  // TODO(torbjorng): Remove before landing.
+  LOG(LS_ERROR) << "Attempted use of truly terrible cipher suite: " << cipher;
+  return false;
 }
 
 }  // namespace rtc
 
 #endif  // HAVE_OPENSSL_SSL_H