Implement Token Binding negotiation TLS extension

net/socket/ssl_client_socket_openssl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // OpenSSL binding for SSLClientSocket. The class layout and general principle
 // of operation is derived from SSLClientSocketNSS.
 
 #include "net/socket/ssl_client_socket_openssl.h"
 
 #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>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/environment.h"
 #include "base/lazy_instance.h"
 #include "base/memory/singleton.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 49 matching lines @@
 // overlap with any value of the net::Error range, including net::OK).
 const int kNoPendingResult = 1;
 
 // If a client doesn't have a list of protocols that it supports, but
 // the server supports NPN, choosing "http/1.1" is the best answer.
 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);
 }
 
 using ScopedX509Stack = crypto::ScopedOpenSSL<STACK_OF(X509), FreeX509Stack>;
 
 // Used for encoding the |connection_status| field of an SSLInfo object.
 int EncodeSSLConnectionStatus(uint16 cipher_suite,
                               int compression,
                               int version) {
@@ skipping 156 matching lines @@
         !ssl_keylog_file.empty()) {
       crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
       BIO* bio = BIO_new_file(ssl_keylog_file.c_str(), "a");
       if (!bio) {
         LOG(ERROR) << "Failed to open " << ssl_keylog_file;
         ERR_print_errors_cb(&LogErrorCallback, NULL);
       } else {
         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) {
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     DCHECK(socket);
     return socket->ClientCertRequestCallback(ssl);
   }
 
   static int CertVerifyCallback(X509_STORE_CTX *store_ctx, void *arg) {
     SSL* ssl = reinterpret_cast<SSL*>(X509_STORE_CTX_get_ex_data(
@@ skipping 185 matching lines @@
       disconnected_(false),
       npn_status_(kNextProtoUnsupported),
       channel_id_sent_(false),
       session_pending_(false),
       certificate_verified_(false),
       ssl_failure_state_(SSL_FAILURE_NONE),
       signature_result_(kNoPendingResult),
       transport_security_state_(context.transport_security_state),
       policy_enforcer_(context.cert_policy_enforcer),
       net_log_(transport_->socket()->NetLog()),
       weak_factory_(this) {

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

Initialize tb_was_negotiated_ and tb_negotiated_param_.

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

I'm initializing tb_was_negotiated_ to false here. I'm also initializing
tb_negotiated_param_ to a valid value, even though it's meaningless when
tb_was_negotiated_ is false.

   DCHECK(cert_verifier_);
 }
 
 SSLClientSocketOpenSSL::~SSLClientSocketOpenSSL() {
   Disconnect();
 }
 
 void SSLClientSocketOpenSSL::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   cert_request_info->host_and_port = host_and_port_;
@@ skipping 134 matching lines @@
 
   channel_id_sent_ = false;
   session_pending_ = false;
   certificate_verified_ = false;
   channel_id_request_.Cancel();
   ssl_failure_state_ = SSL_FAILURE_NONE;
 
   private_key_.reset();
   signature_result_ = kNoPendingResult;
   signature_.clear();
 }

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

Reset tb_was_negotiated_ and tb_negotiated_param_. (This is totally useless.
It's a remnant of some Connect/Disconnect API contract that we've half killed
but not completely killed yet and never worked for SSLClientSocket anyway. See
https://crbug.com/499289.)

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

I'm resetting tb_was_negotiated_, but not tb_negotiated_param_. The latter is
meaningless if the former is false, and unlike the constructor where it makes
sense to initialize tb_negotiated_param_ to avoid any asan/msan/valgrind issues,
there's no need for that here.

 
 bool SSLClientSocketOpenSSL::IsConnected() const {
   // If the handshake has not yet completed.
   if (!completed_connect_)
     return false;
   // If an asynchronous operation is still pending.
   if (user_read_buf_.get() || user_write_buf_.get())
     return true;
 
   return transport_->socket()->IsConnected();
@@ skipping 68 matching lines @@
   ssl_info->cert = server_cert_verify_result_.verified_cert;
   ssl_info->unverified_cert = server_cert_;
   ssl_info->cert_status = server_cert_verify_result_.cert_status;
   ssl_info->is_issued_by_known_root =
       server_cert_verify_result_.is_issued_by_known_root;
   ssl_info->public_key_hashes =
     server_cert_verify_result_.public_key_hashes;
   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);
 
   const SSL_CIPHER* cipher = SSL_get_current_cipher(ssl_);
   CHECK(cipher);
   ssl_info->security_bits = SSL_CIPHER_get_bits(cipher, NULL);
   ssl_info->key_exchange_info =
       SSL_SESSION_get_key_exchange_info(SSL_get_session(ssl_));
 
@@ skipping 378 matching lines @@
 
 int SSLClientSocketOpenSSL::DoHandshakeComplete(int result) {
   if (result < 0)
     return result;
 
   if (ssl_config_.version_fallback &&
       ssl_config_.version_max < ssl_config_.version_fallback_min) {
     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;
     unsigned alpn_len = 0;
     SSL_get0_alpn_selected(ssl_, &alpn_proto, &alpn_len);
     if (alpn_len > 0) {
       npn_proto_.assign(reinterpret_cast<const char*>(alpn_proto), alpn_len);
       npn_status_ = kNextProtoNegotiated;
       set_negotiation_extension(kExtensionALPN);
     }
   }
 
   RecordNegotiationExtension();
   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 ||
       cert_verifier_->SupportsOCSPStapling()) {
     const uint8_t* ocsp_response;
     size_t ocsp_response_len;
     SSL_get0_ocsp_response(ssl_, &ocsp_response, &ocsp_response_len);
 
     set_stapled_ocsp_response_received(ocsp_response_len != 0);
     UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled", ocsp_response_len != 0);
@@ skipping 1023 matching lines @@
   if (next_handshake_state_ == STATE_HANDSHAKE) {
     OnHandshakeIOComplete(signature_result_);
     return;
   }
 
   // During a renegotiation, either Read or Write calls may be blocked on an
   // asynchronous private key operation.
   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