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>
diff --git a/ssl/internal.h b/ssl/internal.h index d71ea57..cdb5cff 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -2032,17 +2032,17 @@ // 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 @@ 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.
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 39c5d92..4e53e7c 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc
@@ -569,8 +569,6 @@ 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 @@ 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 @@ 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 @@ 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; }
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index d5af8aa..70be17e 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc
@@ -414,47 +414,44 @@ // 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 @@ 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 @@ 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;
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index e0e7504..fab6afe 100644 --- a/ssl/t1_lib.cc +++ b/ssl/t1_lib.cc
@@ -3490,7 +3490,7 @@ 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 @@ 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 @@ 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 {