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 29c59f7f583fa46c3430b1d46b5c739f8867369a..16ceeff7394c6b0d2df898dc473fa409726be733 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>
@@ -82,6 +84,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 = 3;
+const uint8_t kTbMinProtocolVersionMajor = 0;
+const uint8_t kTbMinProtocolVersionMinor = 2;
+
 void FreeX509Stack(STACK_OF(X509)* ptr) {
   sk_X509_pop_free(ptr, X509_free);
 }
@@ -243,6 +254,9 @@ class SSLClientSocketOpenSSL::SSLContext {
     SSL_CTX_set_session_cache_mode(
         ssl_ctx_.get(), SSL_SESS_CACHE_CLIENT | SSL_SESS_CACHE_NO_INTERNAL);
     SSL_CTX_sess_set_new_cb(ssl_ctx_.get(), NewSessionCallback);
+
+    if (!RegisterTokenBindingExtensionCallbacks(ssl_ctx_.get()))
+      NOTREACHED();
   }
 
   static int ClientCertRequestCallback(SSL* ssl, void* arg) {
@@ -431,6 +445,8 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       cert_verifier_(context.cert_verifier),
       cert_transparency_verifier_(context.cert_transparency_verifier),
       channel_id_service_(context.channel_id_service),
+      tb_was_negotiated_(false),
+      tb_negotiated_param_(TB_PARAM_ECDSAP256),
       ssl_(NULL),
       transport_bio_(NULL),
       transport_(transport_socket.Pass()),
@@ -607,6 +623,7 @@ void SSLClientSocketOpenSSL::Disconnect() {
   npn_proto_.clear();
 
   channel_id_sent_ = false;
+  tb_was_negotiated_ = false;
   session_pending_ = false;
   certificate_verified_ = false;
   channel_id_request_.Cancel();
@@ -704,6 +721,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);
@@ -1103,6 +1122,11 @@ 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_get_extms_support(ssl_))
+    return ERR_SSL_PROTOCOL_ERROR;
+
   // SSL handshake is completed. If NPN wasn't negotiated, see if ALPN was.
   if (npn_status_ == kNextProtoUnsupported) {
     const uint8_t* alpn_proto = NULL;
@@ -2065,6 +2089,9 @@ std::string SSLClientSocketOpenSSL::GetSessionCacheKey() const {
 }
 
 bool SSLClientSocketOpenSSL::IsRenegotiationAllowed() const {
+  if (tb_was_negotiated_)
+    return false;
+
   if (npn_status_ == kNextProtoUnsupported)
     return ssl_config_.renego_allowed_default;
 
@@ -2165,4 +2192,152 @@ 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;
+}
+
+namespace {
+CBB* CBB_new(size_t initial_capacity) {
+  CBB* ret;
+  ret = reinterpret_cast<CBB*>(OPENSSL_malloc(sizeof(CBB)));
+  if (ret == NULL)
+    return NULL;
+  if (!CBB_init(ret, initial_capacity))
+    OPENSSL_free(ret);
+  return ret;
+}
+
+void CBB_free(CBB* cbb) {
+  if (cbb == NULL)
+    return;
+  CBB_cleanup(cbb);
+  OPENSSL_free(cbb);
+}
+}

[davidben, 2015/11/04 00:40:36]

What's wrong with

class ScopedCBB {
 public:
  ScopedCBB() { CBB_zero(&cbb_); }
  ~ScopedCBB() { CBB_cleanup(&cbb_); }

  CBB* get() { return cbb_; }
 private:
  CBB cbb_;
  DISALLOW_COPY_AND_ASSIGN(ScopedCBB);
};

Shorter, more efficient, and that's how the API is meant to be used. I
specifically added CBB_zero so that it would be scopable without complications.
(CBB_cleanup is defined to be valid in the zero state.)

At the least, it's probably best to not define these OpenSSL-style, so you don't
collide with OpenSSL names later. Also no reason you can't use new/delete or
something or assume malloc doesn't fail.

[nharper, 2015/11/04 02:28:04]

Nothing's wrong with it. I thought you were suggesting using
crypto::ScopedOpenSSL; writing this short ScopedCBB class seems like it's
shorter and simpler.

+
+using ScopedCBB = crypto::ScopedOpenSSL<CBB, CBB_free>;
+
+int SSLClientSocketOpenSSL::TokenBindingAdd(const uint8_t** out,
+                                            size_t* out_len,
+                                            int* out_alert_value) {
+  if (!ssl_config_.token_binding_enabled ||
+      ssl_config_.token_binding_params.empty()) {
+    return 0;
+  }
+  ScopedCBB output(CBB_new(7));
+  CBB parameters_list;
+  int retval = -1;
+  if (!CBB_add_u8(output.get(), kTbProtocolVersionMajor) ||
+      !CBB_add_u8(output.get(), kTbProtocolVersionMinor) ||
+      !CBB_add_u8_length_prefixed(output.get(), &parameters_list)) {
+    return 0;
+  }
+  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]))
+      return 0;
+  }
+  // |*out| will be freed by TokenBindingFreeCallback.
+  if (!CBB_finish(output.get(), const_cast<uint8_t**>(out), out_len))
+    return 0;
+
+  retval = 1;
+
+  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) {
+  if (completed_connect_) {
+    // Token Binding may only be negotiated on the initial handshake.
+    *out_alert_value = SSL_AD_ILLEGAL_PARAMETER;
+    return 0;
+  }
+
+  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) ||
+      !CBS_get_u8_length_prefixed(&extension, &parameters_list) ||
+      !CBS_get_u8(&parameters_list, &param) || CBS_len(&extension) > 0 ||
+      CBS_len(&parameters_list) > 0) {

[davidben, 2015/11/04 00:40:36]

Super nitpicky nit: I think it's mildly better to check CBS_len of
parameters_list before extension, that way you assert empty in stack order.

[nharper, 2015/11/04 02:28:04]

Done.

+    *out_alert_value = SSL_AD_DECODE_ERROR;
+    return 0;
+  }
+  // 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;
+  }
+  // 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;
+  }
+
+  for (size_t i = 0; i < ssl_config_.token_binding_params.size(); ++i) {
+    if (param == ssl_config_.token_binding_params[i]) {
+      tb_negotiated_param_ = ssl_config_.token_binding_params[i];
+      tb_was_negotiated_ = true;
+      return 1;
+    }
+  }

[davidben, 2015/11/04 00:40:36]

Optional: std::find I think works here too. Might be cleaner? Your call.

[nharper, 2015/11/04 02:28:04]

std::find works, but the code is just as long (or short). I'd prefer to leave it
as is. (The rest of this method is much more C-like than C++-like.)

+
+  *out_alert_value = SSL_AD_ILLEGAL_PARAMETER;
+  return 0;
+}
+
+// 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) {

[davidben, 2015/11/04 00:40:35]

Most of the other FooCallback wrappers were static methods on the SSLContext
which then (like you're doing here) forward to the SSLClientSocketOpenSSL. You
can move Register up into SSLContext too and probably just inline it.

[nharper, 2015/11/04 02:28:04]

I inlined Register in SSLContext. Are you also suggesting I move the static
callback methods to SSLContext?

[davidben, 2015/11/04 17:40:13]

Yeah. Doesn't really matter, but it's more consistent with the rest. SSLContext
can call private methods of SSLClientSocketOpenSSL just fine.

[nharper, 2015/11/04 19:43:18]

I moved them into SSLContext to go with the rest of the callbacks there.

+  DCHECK_EQ(extension_value, kTbExtNum);
+  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_EQ(extension_value, kTbExtNum);
+  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_EQ(extension_value, kTbExtNum);
+  SSLClientSocketOpenSSL* socket =
+      SSLClientSocketOpenSSL::SSLContext::GetInstance()->GetClientSocketFromSSL(
+          ssl);
+  return socket->TokenBindingParse(contents, contents_len, out_alert_value);
+}
+
 }  // namespace net