Implement Token Binding negotiation TLS extension

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index c3af8b84d83e62340efb535f64a5e2d93da2ac86..afa322b26f8b4fb10d9070300cd401435d5b3930 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -9,7 +9,9 @@
 
 #include <errno.h>
 #include <openssl/bio.h>
+#include <openssl/bytestring.h>
 #include <openssl/err.h>
+#include <openssl/evp.h>
 #include <openssl/mem.h>
 #include <openssl/ssl.h>
 #include <string.h>
@@ -79,6 +81,15 @@ const char kDefaultSupportedNPNProtocol[] = "http/1.1";
 // Default size of the internal BoringSSL buffers.
 const int KDefaultOpenSSLBufferSize = 17 * 1024;
 
+// TLS extension number use for Token Binding.
+const unsigned int kTbExtNum = 30033;
+
+// Token Binding ProtocolVersions supported.
+const uint8_t kTbProtocolVersionMajor = 0;
+const uint8_t kTbProtocolVersionMinor = 2;
+const uint8_t kTbMinProtocolVersionMajor = 0;
+const uint8_t kTbMinProtocolVersionMinor = 2;
+
 void FreeX509Stack(STACK_OF(X509)* ptr) {
   sk_X509_pop_free(ptr, X509_free);
 }
@@ -255,6 +266,10 @@ class SSLClientSocketOpenSSL::SSLContext {
         SSL_CTX_set_keylog_bio(ssl_ctx_.get(), bio);
       }
     }
+
+    if (!RegisterTokenBindingExtensionCallbacks(ssl_ctx_.get())) {
+      NOTREACHED();
+    }

[davidben, 2015/10/15 21:52:08]

Nit: No curlies, even though I prefer them. :-(

[nharper, 2015/10/20 22:52:18]

Done.

   }
 
   static int ClientCertRequestCallback(SSL* ssl, void* arg) {
@@ -704,6 +719,8 @@ bool SSLClientSocketOpenSSL::GetSSLInfo(SSLInfo* ssl_info) {
   ssl_info->client_cert_sent =
       ssl_config_.send_client_cert && ssl_config_.client_cert.get();
   ssl_info->channel_id_sent = channel_id_sent_;
+  ssl_info->token_binding_negotiated = tb_was_negotiated_;
+  ssl_info->token_binding_key_param = tb_negotiated_param_;
   ssl_info->pinning_failure_log = pinning_failure_log_;
 
   AddSCTInfoToSSLInfo(ssl_info);
@@ -1102,6 +1119,12 @@ int SSLClientSocketOpenSSL::DoHandshakeComplete(int result) {
     return ERR_SSL_FALLBACK_BEYOND_MINIMUM_VERSION;
   }
 
+  // Check that if token binding was negotiated, then extended master secret
+  // must also be negotiated.
+  if (tb_was_negotiated_ && !ssl_->session->extended_master_secret) {

[davidben, 2015/10/15 21:52:08]

This check also doesn't work unless we're forbidding renego (which I would
prefer we do). In that case, you want to tweak IsRenegotiationAllowed().

[davidben, 2015/10/15 21:52:08]

Use SSL_get_extms_support. (BoringSSL will internally ensure that all the EMS
rules around session resumption check out.)

https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#S...

[nharper, 2015/10/20 22:52:18]

Done.

[nharper, 2015/10/20 22:52:18]

I've modified IsRenegotiationAllowed to return false if TB is negotiated.

+    return ERR_SSL_PROTOCOL_ERROR;
+  }

[davidben, 2015/10/15 21:52:09]

Nit: No curlies, even though I prefer them. :-(

[nharper, 2015/10/20 22:52:19]

Done.

+
   // SSL handshake is completed. If NPN wasn't negotiated, see if ALPN was.
   if (npn_status_ == kNextProtoUnsupported) {
     const uint8_t* alpn_proto = NULL;
@@ -1118,6 +1141,8 @@ int SSLClientSocketOpenSSL::DoHandshakeComplete(int result) {
   RecordChannelIDSupport(channel_id_service_, channel_id_sent_,
                          ssl_config_.channel_id_enabled,
                          crypto::ECPrivateKey::IsSupported());
+  RecordTokenBindingSupport(ssl_config_, channel_id_service_,
+                            tb_was_negotiated_);
 
   // Only record OCSP histograms if OCSP was requested.
   if (ssl_config_.signed_cert_timestamps_enabled ||
@@ -2161,4 +2186,137 @@ void SSLClientSocketOpenSSL::OnPrivateKeySignComplete(
   PumpReadWriteEvents();
 }
 
+// static
+bool SSLClientSocketOpenSSL::RegisterTokenBindingExtensionCallbacks(
+    SSL_CTX* ssl_ctx) {
+  return SSL_CTX_add_client_custom_ext(
+             ssl_ctx, kTbExtNum, &TokenBindingAddCallback,
+             &TokenBindingFreeCallback, nullptr, &TokenBindingParseCallback,
+             nullptr) != 0;
+}
+
+int SSLClientSocketOpenSSL::TokenBindingAdd(const uint8_t** out,
+                                            size_t* out_len,
+                                            int* out_alert_value) {
+  if (ssl_config_.token_binding_params.empty()) {
+    return 0;
+  }
+  CBB output, parameters_list;

[davidben, 2015/10/15 21:52:08]

CBB_zero(&output); otherwise CBB_cleanup might act on an uninitialized one. Any
reason not to use the scoper? goto and C++ sometimes interact weirdly when you
declare variables across the point you're goto'ing across.

(I guess, strictly speaking, CBB_init can't fail because mallocs don't fail. But
if you're checking, we probably ought to zero it properly.)

[nharper, 2015/10/20 22:52:18]

I've changed it to use a scoper. There isn't one that already exists (that I
could find) for CBB, which meant I had to define a CBB_new and CBB_free to wrap
around CBB_init and CBB_cleanup.

+  int retval = -1;
+  if (!CBB_init(&output, 7))

[davidben, 2015/10/15 21:52:09]

Nit: You can || this with the next block.

[nharper, 2015/10/20 22:52:19]

This block is gone now.

+    goto end;
+  if (!CBB_add_u8(&output, kTbProtocolVersionMajor) ||
+      !CBB_add_u8(&output, kTbProtocolVersionMinor) ||
+      !CBB_add_u8_length_prefixed(&output, &parameters_list)) {
+    goto end;
+  }
+  for (size_t i = 0; i < ssl_config_.token_binding_params.size(); ++i) {
+    if (!CBB_add_u8(&parameters_list, ssl_config_.token_binding_params[i])) {
+      goto end;
+    }

[davidben, 2015/10/15 21:52:09]

Nit: No curlies, even though I prefer them. :-(

[nharper, 2015/10/20 22:52:18]

Done.

+  }

[davidben, 2015/10/15 21:52:09]

Optional: Since this was confusing before, perhaps a comment:

// |*out| will be freed by TokenBindingFreeCallback.

[nharper, 2015/10/20 22:52:19]

Done.

+  if (!CBB_finish(&output, const_cast<uint8_t**>(out), out_len))
+    goto end;
+
+  retval = 1;
+
+end:
+  CBB_cleanup(&output);
+  if (retval == -1)
+    *out_alert_value = SSL_AD_INTERNAL_ERROR;
+  return retval;
+}
+
+int SSLClientSocketOpenSSL::TokenBindingParse(const uint8_t* contents,
+                                              size_t contents_len,
+                                              int* out_alert_value) {

[davidben, 2015/10/15 21:52:08]

So, this method may get called on a renego, and that it mucks with
tb_negotiated_param_ and such is going to be a problem.

I'm not sure what the renego semantics of this thing are supposed to be at all,
but supposing we go with the only sane option (renegos are forbidden if you TB),
this should have:

if (completed_connect_) {
  // Token Binding may only be negotiated on the initial handshake.
  *out_alert_value = SSL_AD_ILLEGAL_PARAMETER;
  return 0;
}

[nharper, 2015/10/20 22:52:18]

Done.

+  CBS extension;
+  CBS_init(&extension, contents, contents_len);
+
+  CBS parameters_list;
+  uint8_t version_major, version_minor, param;
+  if (!CBS_get_u8(&extension, &version_major) ||
+      !CBS_get_u8(&extension, &version_minor) ||
+      version_major != kTbProtocolVersionMajor ||
+      version_minor != kTbProtocolVersionMinor ||
+      !CBS_get_u8_length_prefixed(&extension, &parameters_list) ||
+      !CBS_get_u8(&parameters_list, &param) || CBS_len(&extension) > 0) {
+    *out_alert_value = SSL_AD_DECODE_ERROR;
+    return 0;
+  }
+  bool valid_param = false;
+  for (size_t i = 0; i < ssl_config_.token_binding_params.size(); ++i) {
+    if (param == ssl_config_.token_binding_params[i]) {
+      tb_negotiated_param_ = TokenBindingParam(param);

[davidben, 2015/10/15 21:52:08]

static_cast<TokenBindingParam>(param).

That or just do ssl_config_.token_binding_params[i] which is already of the
right type. :-)

[nharper, 2015/10/20 22:52:19]

Done.

+      valid_param = true;
+      break;
+    }
+  }
+
+  // The server-negotiated version must be less than or equal to our version.
+  if (version_major > kTbProtocolVersionMajor ||
+      (version_minor > kTbProtocolVersionMinor &&
+       version_major == kTbProtocolVersionMajor)) {
+    *out_alert_value = SSL_AD_ILLEGAL_PARAMETER;
+    return 0;
+  }

[davidben, 2015/10/15 21:52:09]

This seems unnecessary now that you check equality.

[nharper, 2015/10/20 22:52:18]

That was a goof of checking equality for version_major/minor earlier.

+
+  // Although the server returns the negotiated key param in a list, it must
+  // only send one element in that list.
+  if (CBS_len(&parameters_list) > 0 || !valid_param) {

[davidben, 2015/10/15 21:52:08]

Optional: I would actually put the CBS_len check up into the parsing bit. That
seems quite legitimately an encoding error anyway. That TLS extensions some of
the time (but not always) use the same syntax in both directions to the
detriment of the server is really just weird, IMO.

Then you can put tb_was_negotiated_ = true into line 2251, early return and do
away with valid_param.

[nharper, 2015/10/20 22:52:19]

I moved the version checks to before the check on line 2251, to do away with
valid_param and early return. I also moved the CBS_len check for parameters_list
up with the rest of the parsing. A result of this (moving the version checks) is
we're a little lenient in parsing a response from a server with an old version
(one we don't support) that also didn't send a valid param in that we will
silently continue without token binding instead of setting an alert value.

+    *out_alert_value = SSL_AD_ILLEGAL_PARAMETER;
+    return 0;
+  }
+
+  // If the version the server negotiated is older than we support, don't fail
+  // parsing the extension, but also don't set |negotiated_|.
+  if (version_major < kTbMinProtocolVersionMajor ||
+      (version_minor < kTbMinProtocolVersionMinor &&
+       version_major == kTbMinProtocolVersionMajor)) {
+    return 1;
+  }

[davidben, 2015/10/15 21:52:09]

This seems unnecessary now that you check equality.

[nharper, 2015/10/20 22:52:18]

Ditto.

+
+  tb_was_negotiated_ = true;
+  return 1;
+}
+
+// static
+int SSLClientSocketOpenSSL::TokenBindingAddCallback(
+    SSL* ssl,
+    unsigned int extension_value,
+    const uint8_t** out,
+    size_t* out_len,
+    int* out_alert_value,
+    void* add_arg) {
+  DCHECK(extension_value == kTbExtNum);

[davidben, 2015/10/15 21:52:08]

DCHECK_EQ

[nharper, 2015/10/20 22:52:18]

Done.

+  SSLClientSocketOpenSSL* socket =
+      SSLClientSocketOpenSSL::SSLContext::GetInstance()->GetClientSocketFromSSL(
+          ssl);
+  return socket->TokenBindingAdd(out, out_len, out_alert_value);
+}
+
+// static
+void SSLClientSocketOpenSSL::TokenBindingFreeCallback(SSL* ssl,
+                                                      unsigned extension_value,
+                                                      const uint8_t* out,
+                                                      void* add_arg) {
+  DCHECK(extension_value == kTbExtNum);

[davidben, 2015/10/15 21:52:09]

DCHECK_EQ

[nharper, 2015/10/20 22:52:19]

Done.

+  OPENSSL_free(const_cast<unsigned char*>(out));
+}
+
+// static
+int SSLClientSocketOpenSSL::TokenBindingParseCallback(
+    SSL* ssl,
+    unsigned int extension_value,
+    const uint8_t* contents,
+    size_t contents_len,
+    int* out_alert_value,
+    void* parse_arg) {
+  DCHECK(extension_value == kTbExtNum);

[davidben, 2015/10/15 21:52:09]

DCHECK_EQ

[nharper, 2015/10/20 22:52:18]

Done.

+  SSLClientSocketOpenSSL* socket =
+      SSLClientSocketOpenSSL::SSLContext::GetInstance()->GetClientSocketFromSSL(
+          ssl);
+  return socket->TokenBindingParse(contents, contents_len, out_alert_value);
+}
+
 }  // namespace net