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 c4489fba65b04edab4e94d3c9b2089b98db5036e..c2f947f44bb16d0c2a0a6ffaa7b1bae47c8a2883 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>
@@ -260,6 +262,10 @@ class SSLClientSocketOpenSSL::SSLContext {
         SSL_CTX_set_keylog_bio(ssl_ctx_.get(), bio);
       }
     }
+
+    if (!TokenBindingExtension::RegisterCallbacks(ssl_ctx_.get())) {
+      DLOG(ERROR) << "Failed to register calllbacks for token binding";

[mattm, 2015/09/24 22:13:12]

dcheck?

[nharper, 2015/09/28 21:43:39]

Done.

+    }
   }
 
   static int ClientCertRequestCallback(SSL* ssl, void* arg) {
@@ -338,6 +344,7 @@ class SSLClientSocketOpenSSL::SSLContext {
   // TODO(davidben): Sessions should be invalidated on fatal
   // alerts. https://crbug.com/466352
   SSLClientSessionCacheOpenSSL session_cache_;
+  TokenBindingExtension token_binding_extension_;

[mattm, 2015/09/24 22:13:13]

unused?

[nharper, 2015/09/28 21:43:39]

It's used in several places below.

[mattm, 2015/09/28 23:07:25]

Those all look like references to
SSLClientSocketOpenSSL::token_binding_extension_, does anything use
SSLClientSocketOpenSSL::SSLContext::token_binding_extension_?

[nharper, 2015/09/28 23:18:33]

Sorry, I didn't notice that this was in SSLContext. It is indeed unused here.

 };
 
 const SSL_PRIVATE_KEY_METHOD
@@ -557,6 +564,11 @@ int SSLClientSocketOpenSSL::Connect(const CompletionCallback& callback) {
     return rv;
   }
 
+  // Set params for TokenBindingExtension object.
+  if (IsTokenBindingEnabled(ssl_config_, channel_id_service_)) {
+    token_binding_extension_.SetParams(&ssl_config_.token_binding_params);
+  }
+
   // Set SSL to client mode. Handshake happens in the loop below.
   SSL_set_connect_state(ssl_);
 
@@ -720,6 +732,7 @@ 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 = token_binding_extension_.WasNegotiated();
   ssl_info->pinning_failure_log = pinning_failure_log_;
 
   AddSCTInfoToSSLInfo(ssl_info);
@@ -1109,6 +1122,18 @@ 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 (token_binding_extension_.WasNegotiated() &&
+      !ssl_->session->extended_master_secret) {
+    return ERR_SSL_PROTOCOL_ERROR;
+  }
+
+  if (token_binding_extension_.WasNegotiated() && !channel_id_key_) {
+    GotoState(STATE_TOKEN_BINDING_LOOKUP);
+    return OK;
+  }
+
   // SSL handshake is completed. If NPN wasn't negotiated, see if ALPN was.
   if (npn_status_ == kNextProtoUnsupported) {
     const uint8_t* alpn_proto = NULL;
@@ -1187,6 +1212,29 @@ int SSLClientSocketOpenSSL::DoChannelIDLookupComplete(int result) {
   return OK;
 }
 
+int SSLClientSocketOpenSSL::DoTokenBindingLookup() {
+  net_log_.AddEvent(NetLog::TYPE_SSL_CHANNEL_ID_REQUESTED);

[mattm, 2015/09/24 22:13:12]

(I'm not sure why the netlogging for channel id is different in the openssl and
nss files.)

I think the channel_id_service lookups should have begin event
TYPE_SSL_GET_DOMAIN_BOUND_CERT (and a matching end event when the lookup
finishes).

Then there should be a new TYPE_SSL_TOKEN_BINDING_REQUESTED which is done when
the extension is negotiated even if channel_id_key_ is already initialized, and
a TOKEN_BINDING_PROVIDED when it's used 

[nharper, 2015/09/28 21:43:38]

(Both nss and openssl use TYPE_SSL_CHANNEL_ID_REQUESTED/PROVIDED; nss also calls
NetLog::BeginEvent/EndEventWithNetErrorCode for TYPE_SSL_GET_DOMAIN_BOUND_CERT.)

I added a new event TYPE_SSL_GET_TOKEN_BINDING_KEY and call BeginEvent and
EndEventWithNetErrorCode for it when we do the key lookup.

+  GotoState(STATE_TOKEN_BINDING_LOOKUP_COMPLETE);
+  return channel_id_service_->GetOrCreateChannelID(
+      host_and_port_.host(), &channel_id_key_,
+      base::Bind(&SSLClientSocketOpenSSL::OnHandshakeIOComplete,
+                 base::Unretained(this)),
+      &channel_id_request_);
+}
+
+int SSLClientSocketOpenSSL::DoTokenBindingLookupComplete(int result) {
+  if (result < 0)
+    return result;
+
+  if (!channel_id_key_) {
+    LOG(ERROR) << "Failed to import Channel ID.";
+    return ERR_CHANNEL_ID_IMPORT_FAILED;
+  }
+
+  GotoState(STATE_HANDSHAKE_COMPLETE);
+  return OK;
+}
+
 int SSLClientSocketOpenSSL::DoVerifyCert(int result) {
   DCHECK(!server_cert_chain_->empty());
   DCHECK(start_cert_verification_time_.is_null());
@@ -1417,6 +1465,13 @@ int SSLClientSocketOpenSSL::DoHandshakeLoop(int last_io_result) {
       case STATE_CHANNEL_ID_LOOKUP_COMPLETE:
         rv = DoChannelIDLookupComplete(rv);
         break;
+      case STATE_TOKEN_BINDING_LOOKUP:
+        DCHECK_EQ(OK, rv);
+        rv = DoTokenBindingLookup();
+        break;
+      case STATE_TOKEN_BINDING_LOOKUP_COMPLETE:
+        rv = DoTokenBindingLookupComplete(rv);
+        break;
       case STATE_VERIFY_CERT:
         DCHECK_EQ(OK, rv);
         rv = DoVerifyCert(rv);
@@ -2145,4 +2200,140 @@ void SSLClientSocketOpenSSL::OnPrivateKeySignComplete(
   PumpReadWriteEvents();
 }
 
+TokenBindingExtension::TokenBindingExtension()
+    : negotiated_(false), negotiated_param_(TokenBindingParam(0)) {}
+
+TokenBindingExtension::~TokenBindingExtension() {}
+
+bool TokenBindingExtension::WasNegotiated() const {
+  return negotiated_;
+}
+
+TokenBindingParam TokenBindingExtension::NegotiationResult() const {
+  return negotiated_param_;
+};
+
+void TokenBindingExtension::SetParams(std::vector<TokenBindingParam>* params) {

[mattm, 2015/09/24 22:13:12]

take a const ref

[nharper, 2015/09/28 21:43:38]

Done.

+  supported_params_.clear();
+  for (TokenBindingParam param : *params) {
+    supported_params_.push_back(param);
+  }
+};
+
+// static
+int TokenBindingExtension::RegisterCallbacks(SSL_CTX* ssl_ctx) {
+  return SSL_CTX_add_client_custom_ext(
+      ssl_ctx, kExtNum, &TokenBindingExtension::ClientAddCallback,
+      &TokenBindingExtension::ClientFreeCallback, nullptr,
+      &TokenBindingExtension::ClientParseCallback, nullptr);
+}
+
+int TokenBindingExtension::ClientAdd(SSL* s,
+                                     unsigned int ext_type,
+                                     const unsigned char** out,

[davidben, 2015/09/25 21:51:50]

Nit: uint8_t

[nharper, 2015/09/28 21:43:39]

Done. I've gone through and changed these signatures to match boringssl (I
originally copied the signatures, variable names included, from openssl).

+                                     size_t* outlen,
+                                     int* al) {

[mattm, 2015/09/24 22:13:12]

al?

[nharper, 2015/09/28 21:43:39]

out_alert_value. Renamed.

+  if (supported_params_.empty()) {
+    return 0;
+  }
+  if (ext_type != kExtNum) {

[mattm, 2015/09/24 22:13:12]

I'm not familiar enough with the boringssl api here, but should these ext_type
tests be DCHECKs?

[davidben, 2015/09/25 21:51:50]

Yeah, I think DCHECK is right. It shouldn't be called with the wrong one. (We
inherited the API from OpenSSL. I guess the idea is you might reuse the callback
for multiple xtns?)

[nharper, 2015/09/28 21:43:39]

Done.

+    return 0;
+  }
+  CBB output, parameters_list;
+  int retval = 0;
+  if (!CBB_init(&output, 7))
+    goto end;

[davidben, 2015/09/25 21:51:50]

Optional: Here's a scoper for you to save on the gotos.

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

  CBB* get() { return &cbb_; }

 private:
  CBB cbb_;

  DISALLOW_COPY_AND_ASSIGN(ScopedCBB);
};

[nharper, 2015/09/28 21:43:39]

Would it be appropriate to put this scoper in //crypto/scoped_openssl_types.h,
or should it go somewhere else?

+  if (!CBB_add_u8(&output, kProtocolVersionMajor) ||
+      !CBB_add_u8(&output, kProtocolVersionMinor) ||
+      !CBB_add_u8_length_prefixed(&output, &parameters_list)) {
+    goto end;
+  }
+  for (size_t i = 0; i < supported_params_.size(); ++i) {
+    if (!CBB_add_u8(&parameters_list, supported_params_[i])) {
+      goto end;
+    }
+  }
+  if (!CBB_finish(&output, const_cast<uint8_t**>(out), outlen))
+    goto end;
+
+  retval = 1;
+
+end:
+  CBB_cleanup(&output);
+  return retval;
+}
+
+int TokenBindingExtension::ClientParse(SSL* s,
+                                       unsigned int ext_type,
+                                       const unsigned char* in,

[davidben, 2015/09/25 21:51:50]

Nit: uint8_t

[nharper, 2015/09/28 21:43:39]

Done.

+                                       size_t inlen,
+                                       int* al) {
+  if (ext_type != kExtNum) {
+    return 0;
+  }
+  CBS in_ext;
+  CBS_init(&in_ext, in, inlen);
+
+  CBS parameters_list;
+  uint8_t version_major, version_minor, param;
+  if (!CBS_get_u8(&in_ext, &version_major) ||
+      !CBS_get_u8(&in_ext, &version_minor) ||
+      version_major != kProtocolVersionMajor ||
+      version_minor != kProtocolVersionMinor ||
+      !CBS_get_u8_length_prefixed(&in_ext, &parameters_list)) {

[davidben, 2015/09/25 21:51:50]

Also reject CBS_len(&in_ext) != 0 to reject extra bits.

[nharper, 2015/09/28 21:43:38]

Done.

+    return 0;

[davidben, 2015/09/25 21:51:50]

*al = SSL_AD_DECODE_ERROR;

(This is actually the default, but it's not documented as such, so being
explicit is probably good.)

[nharper, 2015/09/28 21:43:38]

Done.

+  }
+
+  if (!CBS_get_u8(&parameters_list, &param))
+    return 0;

[davidben, 2015/09/25 21:51:50]

Optional: I would probably fold this line and the CBS_len check into the pile of
||s above.

[nharper, 2015/09/28 21:43:39]

Done.

+  bool valid_param = false;
+  for (size_t i = 0; i < supported_params_.size(); ++i) {
+    if (param == supported_params_[i]) {
+      negotiated_param_ = TokenBindingParam(param);
+      valid_param = true;
+      break;
+    }
+  }
+  if (CBS_len(&parameters_list) > 0 || !valid_param)

[mattm, 2015/09/24 22:13:12]

Add a comment about why you don't need to check the rest of the parameters_list

[davidben, 2015/09/25 21:51:50]

If valid_param is false, we probably want to emit SSL_AD_ILLEGAL_PARAMETER
rather than SSL_AD_DECODE_ERROR. Not that it really matters at all.

[davidben, 2015/09/25 21:51:50]

parameters_list is only ever supposed to have one element for a client.

[mattm, 2015/09/25 22:01:30]

Right, I just meant there should be a comment stating that. Otherwise if you're
just reading over the code it looks rather weird.

[nharper, 2015/09/28 21:43:39]

Done.

+    return 0;

[mattm, 2015/09/24 22:13:12]

Is setting an appropriate value to *al (out_alert_value) when returning an error
required or optional for this function?

[davidben, 2015/09/25 21:51:50]

*al is only processed on failure, so it can be left alone.

[mattm, 2015/09/25 22:01:30]

I was asking about the failure cases specifically, but it looks like you
addressed that in comments above.

[davidben, 2015/09/25 22:04:25]

Right, sorry, misread that. :-)

+  negotiated_ = true;
+  return 1;
+}
+
+// static
+int TokenBindingExtension::ClientAddCallback(SSL* s,
+                                             unsigned int ext_type,
+                                             const unsigned char** out,
+                                             size_t* outlen,
+                                             int* al,
+                                             void* add_arg) {
+  SSLClientSocketOpenSSL* socket =
+      SSLClientSocketOpenSSL::SSLContext::GetInstance()->GetClientSocketFromSSL(
+          s);
+  return socket->token_binding_extension_.ClientAdd(s, ext_type, out, outlen,
+                                                    al);
+}
+
+// static
+void TokenBindingExtension::ClientFreeCallback(SSL* s,
+                                               unsigned ext_type,
+                                               const unsigned char* out,
+                                               void* add_arg) {
+  OPENSSL_free(const_cast<unsigned char*>(out));
+}
+
+// static
+int TokenBindingExtension::ClientParseCallback(SSL* s,
+                                               unsigned int ext_type,
+                                               const unsigned char* in,
+                                               size_t inlen,
+                                               int* al,
+                                               void* parse_arg) {
+  SSLClientSocketOpenSSL* socket =
+      SSLClientSocketOpenSSL::SSLContext::GetInstance()->GetClientSocketFromSSL(
+          s);
+  return socket->token_binding_extension_.ClientParse(s, ext_type, in, inlen,
+                                                      al);
+}
+
 }  // namespace net