Rotate the default ticket encryption key.

The ticket encryption key is rotated automatically once every 24 hours,
unless a key has been configured manually (i.e. using
|SSL_CTX_set_tlsext_ticket_keys|) or one of the custom ticket encryption
methods is used.

Change-Id: I0dfff28b33e58e96b3bbf7f94dcd6d2642f37aec
Reviewed-on: https://boringssl-review.googlesource.com/18924
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/internal.h

@@ -113,6 +113,7 @@
 #include <openssl/stack.h>
 #include <openssl/thread.h>
 
+#include <assert.h>
 #include <string.h>
 
 #if defined(_MSC_VER)
@@ -462,6 +463,42 @@ OPENSSL_EXPORT void CRYPTO_STATIC_MUTEX_unlock_read(
 OPENSSL_EXPORT void CRYPTO_STATIC_MUTEX_unlock_write(
     struct CRYPTO_STATIC_MUTEX *lock);
 
+#if defined(__cplusplus)
+extern "C++" {
+
+namespace bssl {
+
+namespace internal {
+
+/* MutexLockBase is a RAII helper for CRYPTO_MUTEX locking. */
+template <void (*LockFunc)(CRYPTO_MUTEX *), void (*ReleaseFunc)(CRYPTO_MUTEX *)>
+class MutexLockBase {
+ public:
+  explicit MutexLockBase(CRYPTO_MUTEX *mu) : mu_(mu) {
+    assert(mu_ != nullptr);
+    LockFunc(mu_);
+  }
+  ~MutexLockBase() { ReleaseFunc(mu_); }
+  MutexLockBase(const MutexLockBase<LockFunc, ReleaseFunc> &) = delete;
+  MutexLockBase &operator=(const MutexLockBase<LockFunc, ReleaseFunc> &) =
+      delete;
+
+ private:
+  CRYPTO_MUTEX *const mu_;
+};
+
+}  // namespace internal
+
+using MutexWriteLock =
+    internal::MutexLockBase<CRYPTO_MUTEX_lock_write, CRYPTO_MUTEX_unlock_write>;
+using MutexReadLock =
+    internal::MutexLockBase<CRYPTO_MUTEX_lock_read, CRYPTO_MUTEX_unlock_read>;
+
+}  // namespace bssl
+
+}  // extern "C++"
+#endif  // defined(__cplusplus)
+
 
 /* Thread local storage. */
 

include/openssl/ssl.h

@@ -1975,10 +1975,14 @@ OPENSSL_EXPORT SSL_SESSION *SSL_magic_pending_session_ptr(void);
  * An attacker that compromises a server's session ticket key can impersonate
  * the server and, prior to TLS 1.3, retroactively decrypt all application
  * traffic from sessions using that ticket key. Thus ticket keys must be
- * regularly rotated for forward secrecy. Note the default key is currently not
- * rotated.
- *
- * TODO(davidben): This is silly. Rotate the default key automatically. */
+ * regularly rotated for forward secrecy. Note the default key is rotated
+ * automatically once every 48 hours but manually configured keys are not. */
+
+/* SSL_DEFAULT_TICKET_KEY_ROTATION_INTERVAL is the interval with which the
+ * default session ticket encryption key is rotated, if in use. If any
+ * non-default ticket encryption mechanism is configured, automatic rotation is
+ * disabled. */
+#define SSL_DEFAULT_TICKET_KEY_ROTATION_INTERVAL (2 * 24 * 60 * 60)
 
 /* SSL_CTX_get_tlsext_ticket_keys writes |ctx|'s session ticket key material to
  * |len| bytes of |out|. It returns one on success and zero if |len| is not
@@ -3134,7 +3138,8 @@ OPENSSL_EXPORT void (*SSL_CTX_get_keylog_callback(const SSL_CTX *ctx))(
 /* SSL_CTX_set_current_time_cb configures a callback to retrieve the current
  * time, which should be set in |*out_clock|. This can be used for testing
  * purposes; for example, a callback can be configured that returns a time
- * set explicitly by the test. */
+ * set explicitly by the test. The |ssl| pointer passed to |cb| is always null.
+ */
 OPENSSL_EXPORT void SSL_CTX_set_current_time_cb(
     SSL_CTX *ctx, void (*cb)(const SSL *ssl, struct timeval *out_clock));
 
@@ -4217,6 +4222,17 @@ struct ssl_cipher_preference_list_st {
   uint8_t *in_group_flags;
 };
 
+struct tlsext_ticket_key {
+  uint8_t name[SSL_TICKET_KEY_NAME_LEN];
+  uint8_t hmac_key[16];
+  uint8_t aes_key[16];
+  /* 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;
+};
+
 /* ssl_ctx_st (aka |SSL_CTX|) contains configuration common to several SSL
  * connections. */
 struct ssl_ctx_st {
@@ -4359,10 +4375,14 @@ struct ssl_ctx_st {
   /* TLS extensions servername callback */
   int (*tlsext_servername_callback)(SSL *, int *, void *);
   void *tlsext_servername_arg;
-  /* RFC 4507 session ticket keys */
-  uint8_t tlsext_tick_key_name[SSL_TICKET_KEY_NAME_LEN];
-  uint8_t tlsext_tick_hmac_key[16];
-  uint8_t tlsext_tick_aes_key[16];
+
+  /* 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|. */
+  struct tlsext_ticket_key *tlsext_ticket_key_current;
+  struct tlsext_ticket_key *tlsext_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);
@@ -4433,8 +4453,8 @@ struct ssl_ctx_st {
   void (*keylog_callback)(const SSL *ssl, const char *line);
 
   /* current_time_cb, if not NULL, is the function to use to get the current
-   * time. It sets |*out_clock| to the current time. See
-   * |SSL_CTX_set_current_time_cb|. */
+   * time. It sets |*out_clock| to the current time. The |ssl| argument is
+   * always NULL. See |SSL_CTX_set_current_time_cb|. */
   void (*current_time_cb)(const SSL *ssl, struct timeval *out_clock);
 
   /* pool is used for all |CRYPTO_BUFFER|s in case we wish to share certificate
@@ -4633,6 +4653,7 @@ namespace bssl {
 BORINGSSL_MAKE_DELETER(SSL, SSL_free)
 BORINGSSL_MAKE_DELETER(SSL_CTX, SSL_CTX_free)
 BORINGSSL_MAKE_DELETER(SSL_SESSION, SSL_SESSION_free)
+BORINGSSL_MAKE_DELETER(tlsext_ticket_key, OPENSSL_free);
 
 enum class OpenRecordResult {
   kOK,

ssl/internal.h

@@ -2066,6 +2066,7 @@ int ssl_compare_public_and_private_key(const EVP_PKEY *pubkey,
 int ssl_cert_check_private_key(const CERT *cert, const EVP_PKEY *privkey);
 int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server);
 int ssl_encrypt_ticket(SSL *ssl, CBB *out, const SSL_SESSION *session);
+int ssl_ctx_rotate_ticket_encryption_key(SSL_CTX *ctx);
 
 /* ssl_session_new returns a newly-allocated blank |SSL_SESSION| or nullptr on
  * error. */
@@ -2328,6 +2329,8 @@ int ssl_can_write(const SSL *ssl);
 int ssl_can_read(const SSL *ssl);
 
 void ssl_get_current_time(const SSL *ssl, struct OPENSSL_timeval *out_clock);
+void ssl_ctx_get_current_time(const SSL_CTX *ctx,
+                              struct OPENSSL_timeval *out_clock);
 
 /* ssl_reset_error_state resets state for |SSL_get_error|. */
 void ssl_reset_error_state(SSL *ssl);

ssl/ssl_lib.cc

@@ -357,11 +357,18 @@ void ssl_do_msg_callback(SSL *ssl, int is_write, int content_type,
 }
 
 void ssl_get_current_time(const SSL *ssl, struct OPENSSL_timeval *out_clock) {
-  if (ssl->ctx->current_time_cb != NULL) {
+  /* TODO(martinkr): Change callers to |ssl_ctx_get_current_time| and drop the
+   * |ssl| arg from |current_time_cb| if possible. */
+  ssl_ctx_get_current_time(ssl->ctx, out_clock);
+}
+
+void ssl_ctx_get_current_time(const SSL_CTX *ctx,
+                              struct OPENSSL_timeval *out_clock) {
+  if (ctx->current_time_cb != NULL) {
     /* TODO(davidben): Update current_time_cb to use OPENSSL_timeval. See
      * https://crbug.com/boringssl/155. */
     struct timeval clock;
-    ssl->ctx->current_time_cb(ssl, &clock);
+    ctx->current_time_cb(nullptr /* ssl */, &clock);
     if (clock.tv_sec < 0) {
       assert(0);
       out_clock->tv_sec = 0;
@@ -503,13 +510,6 @@ SSL_CTX *SSL_CTX_new(const SSL_METHOD *method) {
 
   ret->max_send_fragment = SSL3_RT_MAX_PLAIN_LENGTH;
 
-  /* Setup RFC4507 ticket keys */
-  if (!RAND_bytes(ret->tlsext_tick_key_name, 16) ||
-      !RAND_bytes(ret->tlsext_tick_hmac_key, 16) ||
-      !RAND_bytes(ret->tlsext_tick_aes_key, 16)) {
-    ret->options |= SSL_OP_NO_TICKET;
-  }
-
   /* Disable the auto-chaining feature by default. Once this has stuck without
    * problems, the feature will be removed entirely. */
   ret->mode = SSL_MODE_NO_AUTO_CHAIN;
@@ -571,6 +571,8 @@ void SSL_CTX_free(SSL_CTX *ctx) {
   OPENSSL_free(ctx->alpn_client_proto_list);
   EVP_PKEY_free(ctx->tlsext_channel_id_private);
   OPENSSL_free(ctx->verify_sigalgs);
+  OPENSSL_free(ctx->tlsext_ticket_key_current);
+  OPENSSL_free(ctx->tlsext_ticket_key_prev);
 
   OPENSSL_free(ctx);
 }
@@ -1587,10 +1589,18 @@ int SSL_CTX_get_tlsext_ticket_keys(SSL_CTX *ctx, void *out, size_t len) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_TICKET_KEYS_LENGTH);
     return 0;
   }
+
+  /* The default ticket keys are initialized lazily. Trigger a key
+   * rotation to initialize them. */
+  if (!ssl_ctx_rotate_ticket_encryption_key(ctx)) {
+    return 0;
+  }
+
   uint8_t *out_bytes = reinterpret_cast<uint8_t *>(out);
-  OPENSSL_memcpy(out_bytes, ctx->tlsext_tick_key_name, 16);
-  OPENSSL_memcpy(out_bytes + 16, ctx->tlsext_tick_hmac_key, 16);
-  OPENSSL_memcpy(out_bytes + 32, ctx->tlsext_tick_aes_key, 16);
+  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);
   return 1;
 }
 
@@ -1602,10 +1612,22 @@ 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;
+    }
+  }
+  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_tick_key_name, in_bytes, 16);
-  OPENSSL_memcpy(ctx->tlsext_tick_hmac_key, in_bytes + 16, 16);
-  OPENSSL_memcpy(ctx->tlsext_tick_aes_key, in_bytes + 32, 16);
+  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;
   return 1;
 }
 

ssl/ssl_session.cc

@@ -437,6 +437,59 @@ int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server) {
   return 1;
 }
 
+int ssl_ctx_rotate_ticket_encryption_key(SSL_CTX *ctx) {
+  OPENSSL_timeval now;
+  ssl_ctx_get_current_time(ctx, &now);
+  {
+    /* 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)) {
+      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)) {
+    /* The current key has not been initialized or it is expired. */
+    auto new_key = bssl::MakeUnique<struct tlsext_ticket_key>();
+    if (!new_key) {
+      return 0;
+    }
+    OPENSSL_memset(new_key.get(), 0, sizeof(struct tlsext_ticket_key));
+    if (ctx->tlsext_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 +=
+          SSL_DEFAULT_TICKET_KEY_ROTATION_INTERVAL;
+      OPENSSL_free(ctx->tlsext_ticket_key_prev);
+      ctx->tlsext_ticket_key_prev = ctx->tlsext_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;
+  }
+
+  /* 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;
+  }
+
+  return 1;
+}
+
 static int ssl_encrypt_ticket_with_cipher_ctx(SSL *ssl, CBB *out,
                                               const uint8_t *session_buf,
                                               size_t session_len) {
@@ -464,14 +517,19 @@ static int ssl_encrypt_ticket_with_cipher_ctx(SSL *ssl, CBB *out,
       return 0;
     }
   } else {
+    /* Rotate ticket key if necessary. */
+    if (!ssl_ctx_rotate_ticket_encryption_key(tctx)) {
+      return 0;
+    }
+    MutexReadLock lock(&tctx->lock);
     if (!RAND_bytes(iv, 16) ||
         !EVP_EncryptInit_ex(ctx.get(), EVP_aes_128_cbc(), NULL,
-                            tctx->tlsext_tick_aes_key, iv) ||
-        !HMAC_Init_ex(hctx.get(), tctx->tlsext_tick_hmac_key, 16,
+                            tctx->tlsext_ticket_key_current->aes_key, iv) ||
+        !HMAC_Init_ex(hctx.get(), tctx->tlsext_ticket_key_current->hmac_key, 16,
                       tlsext_tick_md(), NULL)) {
       return 0;
     }
-    OPENSSL_memcpy(key_name, tctx->tlsext_tick_key_name, 16);
+    OPENSSL_memcpy(key_name, tctx->tlsext_ticket_key_current->name, 16);
   }
 
   uint8_t *ptr;

ssl/ssl_test.cc

@@ -49,6 +49,31 @@ OPENSSL_MSVC_PRAGMA(warning(pop))
 #endif
 
 
+namespace bssl {
+
+namespace {
+
+struct VersionParam {
+  uint16_t version;
+  enum { is_tls, is_dtls } ssl_method;
+  const char name[8];
+};
+
+static const size_t kTicketKeyLen = 48;
+
+static const VersionParam kAllVersions[] = {
+    {SSL3_VERSION, VersionParam::is_tls, "SSL3"},
+    {TLS1_VERSION, VersionParam::is_tls, "TLS1"},
+    {TLS1_1_VERSION, VersionParam::is_tls, "TLS1_1"},
+    {TLS1_2_VERSION, VersionParam::is_tls, "TLS1_2"},
+// TLS 1.3 requires RSA-PSS, which is disabled for Android system builds.
+#if !defined(BORINGSSL_ANDROID_SYSTEM)
+    {TLS1_3_VERSION, VersionParam::is_tls, "TLS1_3"},
+#endif
+    {DTLS1_VERSION, VersionParam::is_dtls, "DTLS1"},
+    {DTLS1_2_VERSION, VersionParam::is_dtls, "DTLS1_2"},
+};
+
 struct ExpectedCipher {
   unsigned long id;
   int in_group_flag;
@@ -2232,6 +2257,23 @@ static bssl::UniquePtr<SSL_SESSION> ExpectSessionRenewed(SSL_CTX *client_ctx,
   return std::move(g_last_session);
 }
 
+static bool ExpectTicketKeyChanged(SSL_CTX *ctx, uint8_t *inout_key,
+                                   bool changed) {
+  uint8_t new_key[kTicketKeyLen];
+  int res = SSL_CTX_get_tlsext_ticket_keys(ctx, new_key, kTicketKeyLen) == 1;
+  if (res != 1) { /* May return 0, 1 or 48. */
+    fprintf(stderr, "SSL_CTX_get_tlsext_ticket_keys() returned %d.\n", res);
+    return false;
+  }
+  if (changed != !!OPENSSL_memcmp(inout_key, new_key, kTicketKeyLen)) {
+    fprintf(stderr, "Ticket key unexpectedly %s.\n",
+            changed ? "did not change" : "changed");
+    return false;
+  }
+  OPENSSL_memcpy(inout_key, new_key, kTicketKeyLen);
+  return true;
+}
+
 static int SwitchSessionIDContextSNI(SSL *ssl, int *out_alert, void *arg) {
   static const uint8_t kContext[] = {3};
 
@@ -2602,6 +2644,126 @@ static bool TestSessionTimeout(bool is_dtls, const SSL_METHOD *method,
   return true;
 }
 
+class DefaultSessionTicketKeyTest
+    : public ::testing::TestWithParam<VersionParam> {
+ public:
+  bssl::UniquePtr<SSL_CTX> CreateContext() const {
+    const VersionParam version = GetParam();
+    const SSL_METHOD *method = version.ssl_method == VersionParam::is_dtls
+                                   ? DTLS_method()
+                                   : TLS_method();
+    bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(method));
+    if (!ctx ||
+        !SSL_CTX_set_min_proto_version(ctx.get(), version.version) ||
+        !SSL_CTX_set_max_proto_version(ctx.get(), version.version)) {
+      return nullptr;
+    }
+    return ctx;
+  }
+};
+
+
+TEST_P(DefaultSessionTicketKeyTest, Initialization) {
+  bssl::UniquePtr<X509> cert = GetTestCertificate();
+  ASSERT_TRUE(cert);
+  bssl::UniquePtr<EVP_PKEY> key = GetTestKey();
+  ASSERT_TRUE(key);
+
+  bssl::UniquePtr<SSL_CTX> server_ctx = CreateContext();
+  ASSERT_TRUE(server_ctx);
+  static const uint8_t kZeroKey[kTicketKeyLen] = {};
+  uint8_t ticket_key[kTicketKeyLen];
+  ASSERT_EQ(1, SSL_CTX_get_tlsext_ticket_keys(server_ctx.get(), ticket_key,
+                                              kTicketKeyLen));
+  ASSERT_NE(0, OPENSSL_memcmp(ticket_key, kZeroKey, kTicketKeyLen));
+}
+
+TEST_P(DefaultSessionTicketKeyTest, Rotation) {
+  if (GetParam().version == SSL3_VERSION) {
+    return;
+  }
+
+  static const time_t kStartTime = 1001;
+  g_current_time.tv_sec = kStartTime;
+  uint8_t ticket_key[kTicketKeyLen];
+
+  bssl::UniquePtr<X509> cert = GetTestCertificate();
+  ASSERT_TRUE(cert);
+  bssl::UniquePtr<EVP_PKEY> key = GetTestKey();
+  ASSERT_TRUE(key);
+
+  bssl::UniquePtr<SSL_CTX> server_ctx = CreateContext();
+  ASSERT_TRUE(server_ctx);
+  bssl::UniquePtr<SSL_CTX> client_ctx = CreateContext();
+  ASSERT_TRUE(client_ctx);
+
+  /* We use session reuse as a proxy for ticket decryption success, hence
+   * disable session timeouts. */
+  SSL_CTX_set_timeout(server_ctx.get(), std::numeric_limits<uint32_t>::max());
+  SSL_CTX_set_session_psk_dhe_timeout(server_ctx.get(),
+                                      std::numeric_limits<uint32_t>::max());
+
+  SSL_CTX_set_current_time_cb(client_ctx.get(), FrozenTimeCallback);
+  SSL_CTX_set_current_time_cb(server_ctx.get(), CurrentTimeCallback);
+
+  SSL_CTX_set_session_cache_mode(client_ctx.get(), SSL_SESS_CACHE_BOTH);
+  SSL_CTX_set_session_cache_mode(server_ctx.get(), SSL_SESS_CACHE_OFF);
+
+  ASSERT_TRUE(SSL_CTX_use_certificate(server_ctx.get(), cert.get()));
+  ASSERT_TRUE(SSL_CTX_use_PrivateKey(server_ctx.get(), key.get()));
+
+  /* Initialize ticket_key with the current key. */
+  ASSERT_TRUE(
+      ExpectTicketKeyChanged(server_ctx.get(), ticket_key, true /* changed */));
+
+  /* Verify ticket resumption actually works. */
+  bssl::UniquePtr<SSL> client, server;
+  bssl::UniquePtr<SSL_SESSION> session =
+      CreateClientSession(client_ctx.get(), server_ctx.get());
+  ASSERT_TRUE(session);
+  EXPECT_TRUE(ExpectSessionReused(client_ctx.get(), server_ctx.get(),
+                                  session.get(), true /* reused */));
+
+  /* Advance time to just before key rotation. */
+  g_current_time.tv_sec += SSL_DEFAULT_TICKET_KEY_ROTATION_INTERVAL - 1;
+  EXPECT_TRUE(ExpectSessionReused(client_ctx.get(), server_ctx.get(),
+                                  session.get(), true /* reused */));
+  ASSERT_TRUE(ExpectTicketKeyChanged(server_ctx.get(), ticket_key,
+                                     false /* NOT changed */));
+
+  /* Force key rotation. */
+  g_current_time.tv_sec += 1;
+  bssl::UniquePtr<SSL_SESSION> new_session =
+      CreateClientSession(client_ctx.get(), server_ctx.get());
+  ASSERT_TRUE(
+      ExpectTicketKeyChanged(server_ctx.get(), ticket_key, true /* changed */));
+
+  /* Resumption with both old and new ticket should work. */
+  EXPECT_TRUE(ExpectSessionReused(client_ctx.get(), server_ctx.get(),
+                                  session.get(), true /* reused */));
+  EXPECT_TRUE(ExpectSessionReused(client_ctx.get(), server_ctx.get(),
+                                  new_session.get(), true /* reused */));
+  ASSERT_TRUE(ExpectTicketKeyChanged(server_ctx.get(), ticket_key,
+                                     false /* NOT changed */));
+
+  /* Force key rotation again. Resumption with the old ticket now fails. */
+  g_current_time.tv_sec += SSL_DEFAULT_TICKET_KEY_ROTATION_INTERVAL;
+  EXPECT_TRUE(ExpectSessionReused(client_ctx.get(), server_ctx.get(),
+                                  session.get(), false /* NOT reused */));
+  ASSERT_TRUE(
+      ExpectTicketKeyChanged(server_ctx.get(), ticket_key, true /* changed */));
+
+  /* But resumption with the newer session still works. */
+  EXPECT_TRUE(ExpectSessionReused(client_ctx.get(), server_ctx.get(),
+                                  new_session.get(), true /* reused */));
+}
+
+INSTANTIATE_TEST_CASE_P(WithTLSVersion, DefaultSessionTicketKeyTest,
+                        testing::ValuesIn(kAllVersions),
+                        [](const testing::TestParamInfo<VersionParam> &i) {
+                          return i.param.name;
+                        });
+
 static int SwitchContext(SSL *ssl, int *out_alert, void *arg) {
   SSL_CTX *ctx = reinterpret_cast<SSL_CTX*>(arg);
   SSL_set_SSL_CTX(ssl, ctx);
@@ -3313,39 +3475,21 @@ static bool TestRecordCallback(bool is_dtls, const SSL_METHOD *method,
   return true;
 }
 
-
 static bool ForEachVersion(bool (*test_func)(bool is_dtls,
                                              const SSL_METHOD *method,
                                              uint16_t version)) {
-  static uint16_t kTLSVersions[] = {
-      SSL3_VERSION,
-      TLS1_VERSION,
-      TLS1_1_VERSION,
-      TLS1_2_VERSION,
-// TLS 1.3 requires RSA-PSS, which is disabled for Android system builds.
-#if !defined(BORINGSSL_ANDROID_SYSTEM)
-      TLS1_3_VERSION,
-#endif
-  };
-
-  static uint16_t kDTLSVersions[] = {
-      DTLS1_VERSION, DTLS1_2_VERSION,
-  };
-
-  for (uint16_t version : kTLSVersions) {
-    if (!test_func(false, TLS_method(), version)) {
-      fprintf(stderr, "Test failed at TLS version %04x.\n", version);
-      return false;
-    }
-  }
-
-  for (uint16_t version : kDTLSVersions) {
-    if (!test_func(true, DTLS_method(), version)) {
-      fprintf(stderr, "Test failed at DTLS version %04x.\n", version);
+  for (auto version : kAllVersions) {
+    const bool is_dtls = version.ssl_method == VersionParam::is_dtls;
+    const SSL_METHOD *method = is_dtls ? DTLS_method() : TLS_method();
+    if (!test_func(is_dtls, method, version.version)) {
+      if (is_dtls) {
+        fprintf(stderr, "Test failed at DTLS version %04x.\n", version.version);
+      } else {
+        fprintf(stderr, "Test failed at TLS version %04x.\n", version.version);
+      }
       return false;
     }
   }
-
   return true;
 }
 
@@ -4064,3 +4208,6 @@ TEST(SSLTest, AllTests) {
     ADD_FAILURE() << "Tests failed";
   }
 }
+
+}  // namespace
+}  // namespace bssl

ssl/t1_lib.cc

@@ -3004,60 +3004,20 @@ int ssl_parse_serverhello_tlsext(SSL_HANDSHAKE *hs, CBS *cbs) {
   return 1;
 }
 
-static enum ssl_ticket_aead_result_t
-ssl_decrypt_ticket_with_cipher_ctx(SSL *ssl, uint8_t **out, size_t *out_len,
-                                   int *out_renew_ticket, const uint8_t *ticket,
-                                   size_t ticket_len) {
-  const SSL_CTX *const ssl_ctx = ssl->session_ctx;
-
-  ScopedHMAC_CTX hmac_ctx;
-  ScopedEVP_CIPHER_CTX cipher_ctx;
-
-  /* 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. */
-  if (ticket_len < SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH) {
-    return ssl_ticket_aead_ignore_ticket;
-  }
-  const uint8_t *iv = ticket + SSL_TICKET_KEY_NAME_LEN;
-
-  if (ssl_ctx->tlsext_ticket_key_cb != NULL) {
-    int cb_ret = ssl_ctx->tlsext_ticket_key_cb(
-        ssl, (uint8_t *)ticket /* name */, (uint8_t *)iv, cipher_ctx.get(),
-        hmac_ctx.get(), 0 /* decrypt */);
-    if (cb_ret < 0) {
-      return ssl_ticket_aead_error;
-    } else if (cb_ret == 0) {
-      return ssl_ticket_aead_ignore_ticket;
-    } else if (cb_ret == 2) {
-      *out_renew_ticket = 1;
-    }
-  } else {
-    /* Check the key name matches. */
-    if (OPENSSL_memcmp(ticket, ssl_ctx->tlsext_tick_key_name,
-                       SSL_TICKET_KEY_NAME_LEN) != 0) {
-      return ssl_ticket_aead_ignore_ticket;
-    }
-    if (!HMAC_Init_ex(hmac_ctx.get(), ssl_ctx->tlsext_tick_hmac_key,
-                      sizeof(ssl_ctx->tlsext_tick_hmac_key), tlsext_tick_md(),
-                      NULL) ||
-        !EVP_DecryptInit_ex(cipher_ctx.get(), EVP_aes_128_cbc(), NULL,
-                            ssl_ctx->tlsext_tick_aes_key, iv)) {
-      return ssl_ticket_aead_error;
-    }
-  }
-  size_t iv_len = EVP_CIPHER_CTX_iv_length(cipher_ctx.get());
+static enum ssl_ticket_aead_result_t decrypt_ticket_with_cipher_ctx(
+    uint8_t **out, size_t *out_len, EVP_CIPHER_CTX *cipher_ctx,
+    HMAC_CTX *hmac_ctx, const uint8_t *ticket, size_t ticket_len) {
+  size_t iv_len = EVP_CIPHER_CTX_iv_length(cipher_ctx);
 
   /* Check the MAC at the end of the ticket. */
   uint8_t mac[EVP_MAX_MD_SIZE];
-  size_t mac_len = HMAC_size(hmac_ctx.get());
+  size_t mac_len = HMAC_size(hmac_ctx);
   if (ticket_len < SSL_TICKET_KEY_NAME_LEN + iv_len + 1 + mac_len) {
     /* The ticket must be large enough for key name, IV, data, and MAC. */
     return ssl_ticket_aead_ignore_ticket;
   }
-  HMAC_Update(hmac_ctx.get(), ticket, ticket_len - mac_len);
-  HMAC_Final(hmac_ctx.get(), mac, NULL);
+  HMAC_Update(hmac_ctx, ticket, ticket_len - mac_len);
+  HMAC_Final(hmac_ctx, mac, NULL);
   int mac_ok =
       CRYPTO_memcmp(mac, ticket + (ticket_len - mac_len), mac_len) == 0;
 #if defined(BORINGSSL_UNSAFE_FUZZER_MODE)
@@ -3084,9 +3044,9 @@ ssl_decrypt_ticket_with_cipher_ctx(SSL *ssl, uint8_t **out, size_t *out_len,
     return ssl_ticket_aead_ignore_ticket;
   }
   int len1, len2;
-  if (!EVP_DecryptUpdate(cipher_ctx.get(), plaintext.get(), &len1, ciphertext,
+  if (!EVP_DecryptUpdate(cipher_ctx, plaintext.get(), &len1, ciphertext,
                          (int)ciphertext_len) ||
-      !EVP_DecryptFinal_ex(cipher_ctx.get(), plaintext.get() + len1, &len2)) {
+      !EVP_DecryptFinal_ex(cipher_ctx, plaintext.get() + len1, &len2)) {
     ERR_clear_error();
     return ssl_ticket_aead_ignore_ticket;
   }
@@ -3098,6 +3058,69 @@ ssl_decrypt_ticket_with_cipher_ctx(SSL *ssl, uint8_t **out, size_t *out_len,
   return ssl_ticket_aead_success;
 }
 
+static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_cb(
+    SSL *ssl, uint8_t **out, size_t *out_len, int *out_renew_ticket,
+    const uint8_t *ticket, size_t ticket_len) {
+  assert(ticket_len >= SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH);
+  ScopedEVP_CIPHER_CTX cipher_ctx;
+  ScopedHMAC_CTX hmac_ctx;
+  const uint8_t *iv = ticket + SSL_TICKET_KEY_NAME_LEN;
+  int cb_ret = ssl->session_ctx->tlsext_ticket_key_cb(
+      ssl, (uint8_t *)ticket /* name */, (uint8_t *)iv, cipher_ctx.get(),
+      hmac_ctx.get(), 0 /* decrypt */);
+  if (cb_ret < 0) {
+    return ssl_ticket_aead_error;
+  } else if (cb_ret == 0) {
+    return ssl_ticket_aead_ignore_ticket;
+  } else if (cb_ret == 2) {
+    *out_renew_ticket = 1;
+  } else {
+    assert(cb_ret == 1);
+  }
+  return decrypt_ticket_with_cipher_ctx(out, out_len, cipher_ctx.get(),
+                                        hmac_ctx.get(), ticket, ticket_len);
+}
+
+static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_ticket_keys(
+    SSL *ssl, uint8_t **out, size_t *out_len, const uint8_t *ticket,
+    size_t ticket_len) {
+  assert(ticket_len >= SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH);
+  SSL_CTX *ctx = ssl->session_ctx;
+
+  /* Rotate the ticket key if necessary. */
+  if (!ssl_ctx_rotate_ticket_encryption_key(ctx)) {
+    return ssl_ticket_aead_error;
+  }
+
+  /* Pick the matching ticket key and decrypt. */
+  ScopedEVP_CIPHER_CTX cipher_ctx;
+  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,
+                        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,
+                               SSL_TICKET_KEY_NAME_LEN)) {
+      key = ctx->tlsext_ticket_key_prev;
+    } else {
+      return ssl_ticket_aead_ignore_ticket;
+    }
+    const uint8_t *iv = ticket + SSL_TICKET_KEY_NAME_LEN;
+    if (!HMAC_Init_ex(hmac_ctx.get(), key->hmac_key, sizeof(key->hmac_key),
+                      tlsext_tick_md(), NULL) ||
+        !EVP_DecryptInit_ex(cipher_ctx.get(), EVP_aes_128_cbc(), NULL,
+                            key->aes_key, iv)) {
+      return ssl_ticket_aead_error;
+    }
+  }
+  return decrypt_ticket_with_cipher_ctx(out, out_len, cipher_ctx.get(),
+                                        hmac_ctx.get(), ticket, ticket_len);
+}
+
 static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_method(
     SSL *ssl, uint8_t **out, size_t *out_len, int *out_renew_ticket,
     const uint8_t *ticket, size_t ticket_len) {
@@ -3141,8 +3164,20 @@ enum ssl_ticket_aead_result_t ssl_process_ticket(
     result = ssl_decrypt_ticket_with_method(
         ssl, &plaintext, &plaintext_len, out_renew_ticket, ticket, ticket_len);
   } else {
-    result = ssl_decrypt_ticket_with_cipher_ctx(
-        ssl, &plaintext, &plaintext_len, out_renew_ticket, ticket, ticket_len);
+    /* 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. */
+    if (ticket_len < SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH) {
+      return ssl_ticket_aead_ignore_ticket;
+    }
+    if (ssl->session_ctx->tlsext_ticket_key_cb != NULL) {
+      result = ssl_decrypt_ticket_with_cb(ssl, &plaintext, &plaintext_len,
+                                          out_renew_ticket, ticket, ticket_len);
+    } else {
+      result = ssl_decrypt_ticket_with_ticket_keys(
+          ssl, &plaintext, &plaintext_len, ticket, ticket_len);
+    }
   }
 
   if (result != ssl_ticket_aead_success) {