Do not use HTTP/2 without adequate security.

net/socket/ssl_client_socket.cc

Index: net/socket/ssl_client_socket.cc
diff --git a/net/socket/ssl_client_socket.cc b/net/socket/ssl_client_socket.cc
index a52e6a3c052c70eba239aa57594c8a390816fc56..49bcdff210e9ed53b783bbed28209e8bf4a2ec20 100644
--- a/net/socket/ssl_client_socket.cc
+++ b/net/socket/ssl_client_socket.cc
@@ -11,6 +11,7 @@
 #include "net/base/connection_type_histograms.h"
 #include "net/base/host_port_pair.h"
 #include "net/ssl/channel_id_service.h"
+#include "net/ssl/ssl_cipher_suite_names.h"
 #include "net/ssl/ssl_config_service.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 
@@ -234,8 +235,25 @@ bool SSLClientSocket::IsChannelIDEnabled(
 }
 
 // static
+bool SSLClientSocket::IsSecurityAdequateForHTTP2(
+    const SSLConfig& ssl_config,
+    const std::vector<uint16>& cipher_suites) {
+  if (ssl_config.version_max < SSL_PROTOCOL_VERSION_TLS1_2) {
+    return false;
+  }
+  for (std::vector<uint16>::const_iterator it = cipher_suites.begin();

[Ryan Hamilton, 2014/12/10 21:11:41]

for (uint16 cipher : cipher_suites) {
}

[Bence, 2014/12/11 16:50:49]

Done.

+       it != cipher_suites.end(); ++it) {
+    if (IsSecureTLSCipherSuite(*it)) {

[Ryan Hamilton, 2014/12/10 21:11:41]

Just to confirm, this is the right check, yes?

[Ryan Sleevi, 2014/12/10 22:53:06]

Yes

+      return true;
+    }
+  }
+  return false;
+}
+
+// static
 std::vector<uint8_t> SSLClientSocket::SerializeNextProtos(
-    const std::vector<std::string>& next_protos) {
+    const std::vector<std::string>& next_protos,
+    bool advertise_http2) {
   // Do a first pass to determine the total length.
   size_t wire_length = 0;
   for (std::vector<std::string>::const_iterator i = next_protos.begin();
@@ -248,6 +266,13 @@ std::vector<uint8_t> SSLClientSocket::SerializeNextProtos(
       LOG(WARNING) << "Ignoring empty NPN/ALPN protocol";
       continue;
     }
+    if (!advertise_http2) {

[davidben, 2014/12/10 21:29:57]

I'm mildly unhappy that the SSL layer needs to know whether about HTTP/2's
cipher suite requirements (rather than thinking ALPN tokens are opaque strings
and have a higher-level layer deal), but checking cipher suites at any other
layer would be extremely messy. So this is fine.

If we were to care about that, I'd say to not bother with checking
disabled_cipher_suites. That only ever gets set via command-line flag or
enterprise policy it seems.

[Ryan Sleevi, 2014/12/10 22:53:06]

It's almost like you were at the last IETF meeting ;)

On a more practical matter, does it make sense to have the SSLClientSocket
provide some form of callback after this socket has been configured, to let
applications choose the ALPN strings then? It seems like it'd be a little weird
to implement given the ConnectJob layering (for example, AFAIK, the
SSLClientSocket connect job won't have been bound yet to the HTTP
ConnectJob/Request layer, since the pools are in the way, right?), but it might
allow setting policy elsewhere.

Alternatively, having the higher layer pass in a policy selector that can choose
the ALPN strings for a given socket. For now, we might just give the policy
selector the effective set of enabled ciphersuites and TLS versions, in the
future we might pass additional details.

WDYT?
Yes, although it still feels weird to request something from the client we know
we won't support. This is the same argument MSFT has made on the server side.

[Ryan Hamilton, 2014/12/11 19:59:05]

We thought about this when bnc and I were brainstorming this. I'm totally with
you that it seems like that would be a "better" layering, but as you say, I'm
not at all sure what we would callback into? I guess we could add some sort of
NextProtoVectorSanitizerCallback to the SSLSocketParams which could implement
this logic? I'm not sure that the cleaner layering would be worth the extra
plumbing, but I could be convinced otherwise. What do you think?

+      const NextProto proto = NextProtoFromString(*i);
+      if (kProtoSPDY4MinimumVersion <= proto &&
+          proto <= kProtoSPDY4MaximumVersion) {

[Ryan Hamilton, 2014/12/10 21:11:41]

consider adding an IsHttp2NextProto(NextProto next_proto) method since this
logic is now in two places and it's a bit opaque.

[Bence, 2014/12/11 16:50:49]

It will not be in two places after rebasing on https://crrev.com/794563003,
please revisit if you still prefer factoring into its own function.  Also note
that once HTTP/2 is finalized, there will be a singe |proto == kProtoSPDY4|
here.

+        continue;
+      }
+    }
     wire_length += i->size();
     wire_length++;
   }
@@ -259,6 +284,13 @@ std::vector<uint8_t> SSLClientSocket::SerializeNextProtos(
        i != next_protos.end(); i++) {
     if (i->size() == 0 || i->size() > 255)
       continue;
+    if (!advertise_http2) {
+      const NextProto proto = NextProtoFromString(*i);
+      if (kProtoSPDY4MinimumVersion <= proto &&
+          proto <= kProtoSPDY4MaximumVersion) {
+        continue;
+      }
+    }
     wire_protos.push_back(i->size());
     wire_protos.resize(wire_protos.size() + i->size());
     memcpy(&wire_protos[wire_protos.size() - i->size()],