C++ the ticket keys a bit.

While I'm here, remove the silly "tlsext_" prefix. At this point it's no
longer novel that a feature is encoded in an extension.

Change-Id: Ib5fbd2121333a213bdda0332885a8c90036ebc4d
Reviewed-on: https://boringssl-review.googlesource.com/29592
Reviewed-by: Adam Langley <agl@google.com>

ssl/internal.h

@@ -2032,17 +2032,17 @@ extern const SSL_X509_METHOD ssl_crypto_x509_method;
 // crypto/x509.
 extern const SSL_X509_METHOD ssl_noop_x509_method;
 
-struct tlsext_ticket_key {
+struct TicketKey {
   static constexpr bool kAllowUniquePtr = true;
 
-  uint8_t name[SSL_TICKET_KEY_NAME_LEN];
-  uint8_t hmac_key[16];
-  uint8_t aes_key[16];
+  uint8_t name[SSL_TICKET_KEY_NAME_LEN] = {0};
+  uint8_t hmac_key[16] = {0};
+  uint8_t aes_key[16] = {0};
   // next_rotation_tv_sec is the time (in seconds from the epoch) when the
   // current key should be superseded by a new key, or the time when a previous
   // key should be dropped. If zero, then the key should not be automatically
   // rotated.
-  uint64_t next_rotation_tv_sec;
+  uint64_t next_rotation_tv_sec = 0;
 };
 
 }  // namespace bssl
@@ -2952,17 +2952,16 @@ struct ssl_ctx_st {
   int (*tlsext_servername_callback)(SSL *, int *, void *) = nullptr;
   void *tlsext_servername_arg = nullptr;
 
-  // RFC 4507 session ticket keys. |tlsext_ticket_key_current| may be NULL
-  // before the first handshake and |tlsext_ticket_key_prev| may be NULL at any
-  // time. Automatically generated ticket keys are rotated as needed at
-  // handshake time. Hence, all access must be synchronized through |lock|.
-  bssl::tlsext_ticket_key *tlsext_ticket_key_current = nullptr;
-  bssl::tlsext_ticket_key *tlsext_ticket_key_prev = nullptr;
+  // RFC 4507 session ticket keys. |ticket_key_current| may be NULL before the
+  // first handshake and |ticket_key_prev| may be NULL at any time.
+  // Automatically generated ticket keys are rotated as needed at handshake
+  // time. Hence, all access must be synchronized through |lock|.
+  bssl::UniquePtr<bssl::TicketKey> ticket_key_current;
+  bssl::UniquePtr<bssl::TicketKey> ticket_key_prev;
 
   // Callback to support customisation of ticket key setting
-  int (*tlsext_ticket_key_cb)(SSL *ssl, uint8_t *name, uint8_t *iv,
-                              EVP_CIPHER_CTX *ectx, HMAC_CTX *hctx,
-                              int enc) = nullptr;
+  int (*ticket_key_cb)(SSL *ssl, uint8_t *name, uint8_t *iv,
+                       EVP_CIPHER_CTX *ectx, HMAC_CTX *hctx, int enc) = nullptr;
 
   // Server-only: psk_identity_hint is the default identity hint to send in
   // PSK-based key exchanges.

ssl/ssl_lib.cc

@@ -569,8 +569,6 @@ ssl_ctx_st::~ssl_ctx_st() {
   x509_method->ssl_ctx_free(this);
   sk_CertCompressionAlg_pop_free(cert_compression_algs,
                                  Delete<CertCompressionAlg>);
-  OPENSSL_free(tlsext_ticket_key_current);
-  OPENSSL_free(tlsext_ticket_key_prev);
 }
 
 SSL_CTX *SSL_CTX_new(const SSL_METHOD *method) {
@@ -1675,9 +1673,9 @@ int SSL_CTX_get_tlsext_ticket_keys(SSL_CTX *ctx, void *out, size_t len) {
 
   uint8_t *out_bytes = reinterpret_cast<uint8_t *>(out);
   MutexReadLock lock(&ctx->lock);
-  OPENSSL_memcpy(out_bytes, ctx->tlsext_ticket_key_current->name, 16);
-  OPENSSL_memcpy(out_bytes + 16, ctx->tlsext_ticket_key_current->hmac_key, 16);
-  OPENSSL_memcpy(out_bytes + 32, ctx->tlsext_ticket_key_current->aes_key, 16);
+  OPENSSL_memcpy(out_bytes, ctx->ticket_key_current->name, 16);
+  OPENSSL_memcpy(out_bytes + 16, ctx->ticket_key_current->hmac_key, 16);
+  OPENSSL_memcpy(out_bytes + 32, ctx->ticket_key_current->aes_key, 16);
   return 1;
 }
 
@@ -1689,22 +1687,19 @@ int SSL_CTX_set_tlsext_ticket_keys(SSL_CTX *ctx, const void *in, size_t len) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_TICKET_KEYS_LENGTH);
     return 0;
   }
-  if (!ctx->tlsext_ticket_key_current) {
-    ctx->tlsext_ticket_key_current =
-        (tlsext_ticket_key *)OPENSSL_malloc(sizeof(tlsext_ticket_key));
-    if (!ctx->tlsext_ticket_key_current) {
-      return 0;
-    }
+  auto key = MakeUnique<TicketKey>();
+  if (!key) {
+    return 0;
   }
-  OPENSSL_memset(ctx->tlsext_ticket_key_current, 0, sizeof(tlsext_ticket_key));
   const uint8_t *in_bytes = reinterpret_cast<const uint8_t *>(in);
-  OPENSSL_memcpy(ctx->tlsext_ticket_key_current->name, in_bytes, 16);
-  OPENSSL_memcpy(ctx->tlsext_ticket_key_current->hmac_key, in_bytes + 16, 16);
-  OPENSSL_memcpy(ctx->tlsext_ticket_key_current->aes_key, in_bytes + 32, 16);
-  OPENSSL_free(ctx->tlsext_ticket_key_prev);
-  ctx->tlsext_ticket_key_prev = nullptr;
-  // Disable automatic key rotation.
-  ctx->tlsext_ticket_key_current->next_rotation_tv_sec = 0;
+  OPENSSL_memcpy(key->name, in_bytes, 16);
+  OPENSSL_memcpy(key->hmac_key, in_bytes + 16, 16);
+  OPENSSL_memcpy(key->aes_key, in_bytes + 32, 16);
+  // Disable automatic key rotation for manually-configured keys. This is now
+  // the caller's responsibility.
+  key->next_rotation_tv_sec = 0;
+  ctx->ticket_key_current = std::move(key);
+  ctx->ticket_key_prev.reset();
   return 1;
 }
 
@@ -1712,7 +1707,7 @@ int SSL_CTX_set_tlsext_ticket_key_cb(
     SSL_CTX *ctx, int (*callback)(SSL *ssl, uint8_t *key_name, uint8_t *iv,
                                   EVP_CIPHER_CTX *ctx, HMAC_CTX *hmac_ctx,
                                   int encrypt)) {
-  ctx->tlsext_ticket_key_cb = callback;
+  ctx->ticket_key_cb = callback;
   return 1;
 }
 

ssl/ssl_session.cc

@@ -414,47 +414,44 @@ int ssl_ctx_rotate_ticket_encryption_key(SSL_CTX *ctx) {
     // Avoid acquiring a write lock in the common case (i.e. a non-default key
     // is used or the default keys have not expired yet).
     MutexReadLock lock(&ctx->lock);
-    if (ctx->tlsext_ticket_key_current &&
-        (ctx->tlsext_ticket_key_current->next_rotation_tv_sec == 0 ||
-         ctx->tlsext_ticket_key_current->next_rotation_tv_sec > now.tv_sec) &&
-        (!ctx->tlsext_ticket_key_prev ||
-         ctx->tlsext_ticket_key_prev->next_rotation_tv_sec > now.tv_sec)) {
+    if (ctx->ticket_key_current &&
+        (ctx->ticket_key_current->next_rotation_tv_sec == 0 ||
+         ctx->ticket_key_current->next_rotation_tv_sec > now.tv_sec) &&
+        (!ctx->ticket_key_prev ||
+         ctx->ticket_key_prev->next_rotation_tv_sec > now.tv_sec)) {
       return 1;
     }
   }
 
   MutexWriteLock lock(&ctx->lock);
-  if (!ctx->tlsext_ticket_key_current ||
-      (ctx->tlsext_ticket_key_current->next_rotation_tv_sec != 0 &&
-       ctx->tlsext_ticket_key_current->next_rotation_tv_sec <= now.tv_sec)) {
+  if (!ctx->ticket_key_current ||
+      (ctx->ticket_key_current->next_rotation_tv_sec != 0 &&
+       ctx->ticket_key_current->next_rotation_tv_sec <= now.tv_sec)) {
     // The current key has not been initialized or it is expired.
-    auto new_key = bssl::MakeUnique<struct tlsext_ticket_key>();
+    auto new_key = bssl::MakeUnique<TicketKey>();
     if (!new_key) {
       return 0;
     }
-    OPENSSL_memset(new_key.get(), 0, sizeof(struct tlsext_ticket_key));
-    if (ctx->tlsext_ticket_key_current) {
+    RAND_bytes(new_key->name, 16);
+    RAND_bytes(new_key->hmac_key, 16);
+    RAND_bytes(new_key->aes_key, 16);
+    new_key->next_rotation_tv_sec =
+        now.tv_sec + SSL_DEFAULT_TICKET_KEY_ROTATION_INTERVAL;
+    if (ctx->ticket_key_current) {
       // The current key expired. Rotate it to prev and bump up its rotation
       // timestamp. Note that even with the new rotation time it may still be
-      // expired and get droppped below.
-      ctx->tlsext_ticket_key_current->next_rotation_tv_sec +=
+      // expired and get dropped below.
+      ctx->ticket_key_current->next_rotation_tv_sec +=
           SSL_DEFAULT_TICKET_KEY_ROTATION_INTERVAL;
-      OPENSSL_free(ctx->tlsext_ticket_key_prev);
-      ctx->tlsext_ticket_key_prev = ctx->tlsext_ticket_key_current;
+      ctx->ticket_key_prev = std::move(ctx->ticket_key_current);
     }
-    ctx->tlsext_ticket_key_current = new_key.release();
-    RAND_bytes(ctx->tlsext_ticket_key_current->name, 16);
-    RAND_bytes(ctx->tlsext_ticket_key_current->hmac_key, 16);
-    RAND_bytes(ctx->tlsext_ticket_key_current->aes_key, 16);
-    ctx->tlsext_ticket_key_current->next_rotation_tv_sec =
-        now.tv_sec + SSL_DEFAULT_TICKET_KEY_ROTATION_INTERVAL;
+    ctx->ticket_key_current = std::move(new_key);
   }
 
   // Drop an expired prev key.
-  if (ctx->tlsext_ticket_key_prev &&
-      ctx->tlsext_ticket_key_prev->next_rotation_tv_sec <= now.tv_sec) {
-    OPENSSL_free(ctx->tlsext_ticket_key_prev);
-    ctx->tlsext_ticket_key_prev = nullptr;
+  if (ctx->ticket_key_prev &&
+      ctx->ticket_key_prev->next_rotation_tv_sec <= now.tv_sec) {
+    ctx->ticket_key_prev.reset();
   }
 
   return 1;
@@ -481,9 +478,9 @@ static int ssl_encrypt_ticket_with_cipher_ctx(SSL_HANDSHAKE *hs, CBB *out,
   SSL_CTX *tctx = hs->ssl->session_ctx.get();
   uint8_t iv[EVP_MAX_IV_LENGTH];
   uint8_t key_name[16];
-  if (tctx->tlsext_ticket_key_cb != NULL) {
-    if (tctx->tlsext_ticket_key_cb(hs->ssl, key_name, iv, ctx.get(), hctx.get(),
-                                   1 /* encrypt */) < 0) {
+  if (tctx->ticket_key_cb != NULL) {
+    if (tctx->ticket_key_cb(hs->ssl, key_name, iv, ctx.get(), hctx.get(),
+                            1 /* encrypt */) < 0) {
       return 0;
     }
   } else {
@@ -494,12 +491,12 @@ static int ssl_encrypt_ticket_with_cipher_ctx(SSL_HANDSHAKE *hs, CBB *out,
     MutexReadLock lock(&tctx->lock);
     if (!RAND_bytes(iv, 16) ||
         !EVP_EncryptInit_ex(ctx.get(), EVP_aes_128_cbc(), NULL,
-                            tctx->tlsext_ticket_key_current->aes_key, iv) ||
-        !HMAC_Init_ex(hctx.get(), tctx->tlsext_ticket_key_current->hmac_key, 16,
+                            tctx->ticket_key_current->aes_key, iv) ||
+        !HMAC_Init_ex(hctx.get(), tctx->ticket_key_current->hmac_key, 16,
                       tlsext_tick_md(), NULL)) {
       return 0;
     }
-    OPENSSL_memcpy(key_name, tctx->tlsext_ticket_key_current->name, 16);
+    OPENSSL_memcpy(key_name, tctx->ticket_key_current->name, 16);
   }
 
   uint8_t *ptr;

ssl/t1_lib.cc

@@ -3490,7 +3490,7 @@ static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_cb(
   ScopedEVP_CIPHER_CTX cipher_ctx;
   ScopedHMAC_CTX hmac_ctx;
   const uint8_t *iv = ticket + SSL_TICKET_KEY_NAME_LEN;
-  int cb_ret = hs->ssl->session_ctx->tlsext_ticket_key_cb(
+  int cb_ret = hs->ssl->session_ctx->ticket_key_cb(
       hs->ssl, (uint8_t *)ticket /* name */, (uint8_t *)iv, cipher_ctx.get(),
       hmac_ctx.get(), 0 /* decrypt */);
   if (cb_ret < 0) {
@@ -3522,15 +3522,15 @@ static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_ticket_keys(
   ScopedHMAC_CTX hmac_ctx;
   {
     MutexReadLock lock(&ctx->lock);
-    const tlsext_ticket_key *key;
-    if (ctx->tlsext_ticket_key_current &&
-        !OPENSSL_memcmp(ctx->tlsext_ticket_key_current->name, ticket,
+    const TicketKey *key;
+    if (ctx->ticket_key_current &&
+        !OPENSSL_memcmp(ctx->ticket_key_current->name, ticket,
                         SSL_TICKET_KEY_NAME_LEN)) {
-      key = ctx->tlsext_ticket_key_current;
-    } else if (ctx->tlsext_ticket_key_prev &&
-               !OPENSSL_memcmp(ctx->tlsext_ticket_key_prev->name, ticket,
+      key = ctx->ticket_key_current.get();
+    } else if (ctx->ticket_key_prev &&
+               !OPENSSL_memcmp(ctx->ticket_key_prev->name, ticket,
                                SSL_TICKET_KEY_NAME_LEN)) {
-      key = ctx->tlsext_ticket_key_prev;
+      key = ctx->ticket_key_prev.get();
     } else {
       return ssl_ticket_aead_ignore_ticket;
     }
@@ -3589,14 +3589,14 @@ enum ssl_ticket_aead_result_t ssl_process_ticket(
     result = ssl_decrypt_ticket_with_method(
         hs, &plaintext, &plaintext_len, out_renew_ticket, ticket, ticket_len);
   } else {
-    // Ensure there is room for the key name and the largest IV
-    // |tlsext_ticket_key_cb| may try to consume. The real limit may be lower,
-    // but the maximum IV length should be well under the minimum size for the
-    // session material and HMAC.
+    // Ensure there is room for the key name and the largest IV |ticket_key_cb|
+    // may try to consume. The real limit may be lower, but the maximum IV
+    // length should be well under the minimum size for the session material and
+    // HMAC.
     if (ticket_len < SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH) {
       return ssl_ticket_aead_ignore_ticket;
     }
-    if (hs->ssl->session_ctx->tlsext_ticket_key_cb != NULL) {
+    if (hs->ssl->session_ctx->ticket_key_cb != NULL) {
       result = ssl_decrypt_ticket_with_cb(hs, &plaintext, &plaintext_len,
                                           out_renew_ticket, ticket, ticket_len);
     } else {