net: boost AES-GCM ciphers if the machine has AES-NI.

net/socket/nss_ssl_util.cc

Index: net/socket/nss_ssl_util.cc
diff --git a/net/socket/nss_ssl_util.cc b/net/socket/nss_ssl_util.cc
index 8547d4dc1cf493f167c4a2ebb731c809029870dc..48de03f6741157b64a846c962bd8dc9ce3343c74 100644
--- a/net/socket/nss_ssl_util.cc
+++ b/net/socket/nss_ssl_util.cc
@@ -13,6 +13,7 @@
 #include <string>
 
 #include "base/bind.h"
+#include "base/cpu.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
@@ -27,6 +28,59 @@
 #include "base/win/windows_version.h"
 #endif
 
+namespace {
+
+// CiphersRemove takes a zero-terminated array of cipher suite ids in
+// |to_remove| and sets every instance of them in |ciphers| to zero. It returns
+// true if it found and removed every element of |to_remove|. It assumes that
+// there are no duplicates in |ciphers| nor in |to_remove|.
+bool CiphersRemove(const uint16* to_remove, uint16* ciphers, size_t num) {
+  size_t num_remove, found = 0;
+
+  for (num_remove = 0; ; num_remove++) {
+    if (to_remove[num_remove] == 0)
+      break;
+  }
+
+  for (size_t i = 0; i < num; i++) {
+    for (size_t j = 0; j < num_remove; j++) {
+      if (to_remove[j] == ciphers[i]) {
+        ciphers[i] = 0;
+        found++;
+        break;
+      }
+    }
+  }

[wtc, 2013/11/22 01:14:16]

I think it is less work to reverse the nested for loops because the ciphers we
want to remove are likely to be in the top half of the |ciphers| array.

[agl, 2013/11/22 17:53:49]

Done.

+
+  return found == num_remove;
+}
+
+// CiphersCompact takes an array of cipher suite ids in |ciphers|, where some
+// entries are zero, and moves the entries so that all the non-zero elements
+// are compacted at the end of the array.
+size_t CiphersCompact(uint16* ciphers, size_t num) {
+  size_t j = num - 1;
+
+  for (size_t i = num - 1; i < num; i--) {

[wtc, 2013/11/22 01:14:16]

The i < num check looks wrong at first glance. You are relying on wrapping of an
unsigned integer type, right?

[agl, 2013/11/22 17:53:49]

Yes.

+    if (ciphers[i] == 0)
+      continue;
+    ciphers[j--] = ciphers[i];
+  }
+  return j+1;

[wtc, 2013/11/22 01:14:16]

Nit: the Style Guide recommends spaces around '+' and '-'.

Since the only caller of this function ignores the return value, this function
can return void.

[agl, 2013/11/22 17:53:49]

Done.

+}
+
+// CiphersCopy copies the zero-terminated array |in| to |out|.

[wtc, 2013/11/22 01:14:16]

Document the return value.

[agl, 2013/11/22 17:53:49]

Done.

+size_t CiphersCopy(const uint16* in, uint16* out) {
+  for (size_t i = 0; ; i++) {
+    if (in[i] == 0)
+      return i;
+    out[i] = in[i];
+  }
+}
+
+}  // anonymous namespace
+
+

[wtc, 2013/11/22 01:14:16]

Nit: delete one blank line

[agl, 2013/11/22 17:53:49]

Done.

 namespace net {
 
 class NSSSSLInitSingleton {
@@ -86,14 +140,64 @@ class NSSSSLInitSingleton {
     // Enable SSL.
     SSL_OptionSetDefault(SSL_SECURITY, PR_TRUE);
 
+    // Calculate the order of ciphers that we'll use for NSS sockets. (Note
+    // that, even if a cipher is specified in the ordering, it won't be
+    // included if it is disabled.)

[wtc, 2013/11/22 01:14:16]

"it won't be included if it is disabled" sounds wrong. Did you mean "it won't be
used if it is disabled"?

[agl, 2013/11/22 17:53:49]

I really did mean that, but it's clearly confusing. I've changed it to "Note
that, even if a cipher is specified in the ordering, it must still be enabled in
order to be included in a ClientHello."

+    //
+    // Our top preference cipher suites are either forward-secure AES-GCM or
+    // forward secure ChaCha20-Poly1305. If the local machine has AES-NI then

[wtc, 2013/11/22 01:14:16]

Nit: forward secure => forward secret (two occurrences)

[agl, 2013/11/22 17:53:49]

Done.

+    // we prefer AES-GCM, otherwise ChaCha20. The remainder of the cipher suite
+    // preference is inheriented from NSS. */
+    static const uint16 chacha_ciphers[] = {
+      0xcc14  /* TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305 */,

[wtc, 2013/11/22 01:14:16]

You should be able to use TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, etc. These
macros are defined in the NSS header sslproto.h, which we include.

[agl, 2013/11/22 17:53:49]

Done.

+      0xcc13  /* TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305 */,
+      0,
+    };
+    static const uint16 aes_gcm_ciphers[] = {
+      0xc02b  /* TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 */,
+      0xc02f  /* TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 */,
+      0x9e    /* TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 */,
+      0,
+    };
+    const PRUint16 *all_ciphers = SSL_GetImplementedCiphers();

[wtc, 2013/11/22 01:14:16]

Nit: put '*' next to the type. Also fix lines 171-172.

[agl, 2013/11/22 17:53:49]

Done.

+    num_ciphers_ = SSL_GetNumImplementedCiphers();
+    ciphers_.reset(new uint16[num_ciphers_]);
+    memcpy(ciphers_.get(), all_ciphers, sizeof(uint16)*num_ciphers_);
+
+    if (CiphersRemove(chacha_ciphers, ciphers_.get(), num_ciphers_) &&
+        CiphersRemove(aes_gcm_ciphers, ciphers_.get(), num_ciphers_)) {

[wtc, 2013/11/22 01:14:16]

Nit: We can pass the sizes of chacha_ciphers and aes_gcm_ciphers (excluding the
zero terminators) to CiphersRemove so that CiphersRemove doesn't need to
calculate the size of its |to_remove| input.

[agl, 2013/11/22 17:53:49]

By reordering the for loops in CiphersRemove, as you suggested, the function no
longer needs to calculate the length beforehand.

+      CiphersCompact(ciphers_.get(), num_ciphers_);
+
+      const uint16 *preference_ciphers = chacha_ciphers;
+      const uint16 *other_ciphers = aes_gcm_ciphers;
+      if (base::CPU().has_aesni()) {
+        preference_ciphers = aes_gcm_ciphers;
+        other_ciphers = chacha_ciphers;
+      }
+      unsigned i = CiphersCopy(preference_ciphers, ciphers_.get());
+      CiphersCopy(other_ciphers, &ciphers_[i]);
+    } else {
+      ciphers_.reset();
+      num_ciphers_ = 0;
+    }
+
     // All other SSL options are set per-session by SSLClientSocket and
     // SSLServerSocket.
   }
 
+  const uint16* GetNSSCipherOrder(size_t* out_length) {
+    *out_length = num_ciphers_;
+    return ciphers_.get();
+  }
+
   ~NSSSSLInitSingleton() {
     // Have to clear the cache, or NSS_Shutdown fails with SEC_ERROR_BUSY.
     SSL_ClearSessionCache();
   }
+
+ private:
+  scoped_ptr<uint16[]> ciphers_;
+  size_t num_ciphers_;
 };
 
 static base::LazyInstance<NSSSSLInitSingleton> g_nss_ssl_init_singleton =
@@ -112,6 +216,10 @@ void EnsureNSSSSLInit() {
   g_nss_ssl_init_singleton.Get();
 }
 
+const uint16* GetNSSCipherOrder(size_t* out_length) {
+  return g_nss_ssl_init_singleton.Get().GetNSSCipherOrder(out_length);
+}
+
 // Map a Chromium net error code to an NSS error code.
 // See _MD_unix_map_default_error in the NSS source
 // tree for inspiration.