Reject tickets from the future.

This shouldn't happen, but it is good to check to avoid the potential
underflow in ssl_session_is_time_valid.

This required tweaking the mock clock in bssl_shim to stop going back in
time.

Change-Id: Id3ab8755139e989190d0b53d4bf90fe1ac203022
Reviewed-on: https://boringssl-review.googlesource.com/11841
Reviewed-by: David Benjamin <davidben@google.com>

ssl/ssl_session.c

@@ -599,6 +599,12 @@ int ssl_session_is_time_valid(const SSL *ssl, const SSL_SESSION *session) {
 
   struct timeval now;
   ssl_get_current_time(ssl, &now);
+
+  /* Reject tickets from the future to avoid underflow. */
+  if ((long)now.tv_sec < session->time) {
+    return 0;
+  }
+
   return session->timeout >= (long)now.tv_sec - session->time;
 }
 

ssl/ssl_test.cc

@@ -2047,6 +2047,9 @@ static bool TestSessionTimeout() {
   }
 
   for (uint16_t version : kTLSVersions) {
+    static const int kStartTime = 1000;
+    g_current_time.tv_sec = kStartTime;
+
     bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(TLS_method()));
     bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
     if (!server_ctx || !client_ctx ||
@@ -2088,6 +2091,15 @@ static bool TestSessionTimeout() {
       fprintf(stderr, "Error resuming session (version = %04x).\n", version);
       return false;
     }
+
+    // Rewind the clock to before the session was minted.
+    g_current_time.tv_sec = kStartTime - 1;
+
+    if (!ExpectSessionReused(client_ctx.get(), server_ctx.get(), session.get(),
+                             false /* expect session not reused */)) {
+      fprintf(stderr, "Error resuming session (version = %04x).\n", version);
+      return false;
+    }
   }
 
   return true;

ssl/test/bssl_shim.cc

@@ -594,8 +594,10 @@ static unsigned PskServerCallback(SSL *ssl, const char *identity,
   return config->psk.size();
 }
 
+static timeval g_clock;
+
 static void CurrentTimeCallback(const SSL *ssl, timeval *out_clock) {
-  *out_clock = PacketedBioGetClock(GetTestState(ssl)->packeted_bio);
+  *out_clock = g_clock;
 }
 
 static void ChannelIdCallback(SSL *ssl, EVP_PKEY **out_pkey) {
@@ -923,9 +925,7 @@ static bssl::UniquePtr<SSL_CTX> SetupCtx(const TestConfig *config) {
   SSL_CTX_enable_tls_channel_id(ssl_ctx.get());
   SSL_CTX_set_channel_id_cb(ssl_ctx.get(), ChannelIdCallback);
 
-  if (config->is_dtls) {
+  SSL_CTX_set_current_time_cb(ssl_ctx.get(), CurrentTimeCallback);
-    SSL_CTX_set_current_time_cb(ssl_ctx.get(), CurrentTimeCallback);
-  }
 
   SSL_CTX_set_info_callback(ssl_ctx.get(), InfoCallback);
   SSL_CTX_sess_set_new_cb(ssl_ctx.get(), NewSessionCallback);
@@ -1473,7 +1473,7 @@ static bool DoExchange(bssl::UniquePtr<SSL_SESSION> *out_session,
     return false;
   }
   if (config->is_dtls) {
-    bssl::UniquePtr<BIO> packeted = PacketedBioCreate(!config->async);
+    bssl::UniquePtr<BIO> packeted = PacketedBioCreate(&g_clock, !config->async);
     if (!packeted) {
       return false;
     }
@@ -1748,6 +1748,11 @@ static int Main(int argc, char **argv) {
     return Usage(argv[0]);
   }
 
+  // Some code treats the zero time special, so initialize the clock to a
+  // non-zero time.
+  g_clock.tv_sec = 1234;
+  g_clock.tv_usec = 1234;
+
   bssl::UniquePtr<SSL_CTX> ssl_ctx = SetupCtx(&config);
   if (!ssl_ctx) {
     ERR_print_errors_fp(stderr);

ssl/test/packeted_bio.cc

@@ -31,10 +31,9 @@ const uint8_t kOpcodeTimeout = 'T';
 const uint8_t kOpcodeTimeoutAck = 't';
 
 struct PacketedBio {
-  explicit PacketedBio(bool advance_clock_arg)
+  PacketedBio(timeval *clock_arg, bool advance_clock_arg)
-      : advance_clock(advance_clock_arg) {
+      : clock(clock_arg), advance_clock(advance_clock_arg) {
     memset(&timeout, 0, sizeof(timeout));
-    memset(&clock, 0, sizeof(clock));
     memset(&read_deadline, 0, sizeof(read_deadline));
   }
 
@@ -47,14 +46,14 @@ struct PacketedBio {
       return true;
     }
 
-    if (clock.tv_sec == read_deadline.tv_sec) {
+    if (clock->tv_sec == read_deadline.tv_sec) {
-      return clock.tv_usec < read_deadline.tv_usec;
+      return clock->tv_usec < read_deadline.tv_usec;
     }
-    return clock.tv_sec < read_deadline.tv_sec;
+    return clock->tv_sec < read_deadline.tv_sec;
   }
 
   timeval timeout;
-  timeval clock;
+  timeval *clock;
   timeval read_deadline;
   bool advance_clock;
 };
@@ -66,10 +65,6 @@ PacketedBio *GetData(BIO *bio) {
   return (PacketedBio *)bio->ptr;
 }
 
-const PacketedBio *GetData(const BIO *bio) {
-  return GetData(const_cast<BIO*>(bio));
-}
-
 // ReadAll reads |len| bytes from |bio| into |out|. It returns 1 on success and
 // 0 or -1 on error.
 static int ReadAll(BIO *bio, uint8_t *out, size_t len) {
@@ -272,19 +267,15 @@ const BIO_METHOD g_packeted_bio_method = {
 
 }  // namespace
 
-bssl::UniquePtr<BIO> PacketedBioCreate(bool advance_clock) {
+bssl::UniquePtr<BIO> PacketedBioCreate(timeval *clock, bool advance_clock) {
   bssl::UniquePtr<BIO> bio(BIO_new(&g_packeted_bio_method));
   if (!bio) {
     return nullptr;
   }
-  bio->ptr = new PacketedBio(advance_clock);
+  bio->ptr = new PacketedBio(clock, advance_clock);
   return bio;
 }
 
-timeval PacketedBioGetClock(const BIO *bio) {
-  return GetData(bio)->clock;
-}
-
 bool PacketedBioAdvanceClock(BIO *bio) {
   PacketedBio *data = GetData(bio);
   if (data == nullptr) {
@@ -295,10 +286,10 @@ bool PacketedBioAdvanceClock(BIO *bio) {
     return false;
   }
 
-  data->clock.tv_usec += data->timeout.tv_usec;
+  data->clock->tv_usec += data->timeout.tv_usec;
-  data->clock.tv_sec += data->clock.tv_usec / 1000000;
+  data->clock->tv_sec += data->clock->tv_usec / 1000000;
-  data->clock.tv_usec %= 1000000;
+  data->clock->tv_usec %= 1000000;
-  data->clock.tv_sec += data->timeout.tv_sec;
+  data->clock->tv_sec += data->timeout.tv_sec;
   memset(&data->timeout, 0, sizeof(data->timeout));
   return true;
 }

ssl/test/packeted_bio.h

@@ -28,21 +28,18 @@ OPENSSL_MSVC_PRAGMA(warning(pop))
 
 
 // PacketedBioCreate creates a filter BIO which implements a reliable in-order
-// blocking datagram socket. It internally maintains a clock and honors
+// blocking datagram socket. It uses the value of |*clock| as the clock and
-// |BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT| based on it.
+// honors |BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT| based on it.
 //
 // During a |BIO_read|, the peer may signal the filter BIO to simulate a
 // timeout. If |advance_clock| is true, it automatically advances the clock and
 // continues reading, subject to the read deadline. Otherwise, it fails
 // immediately. The caller must then call |PacketedBioAdvanceClock| before
 // retrying |BIO_read|.
-bssl::UniquePtr<BIO> PacketedBioCreate(bool advance_clock);
+bssl::UniquePtr<BIO> PacketedBioCreate(timeval *clock, bool advance_clock);
 
-// PacketedBioGetClock returns the current time for |bio|.
+// PacketedBioAdvanceClock advances |bio|'s clock and returns true if there is a
-timeval PacketedBioGetClock(const BIO *bio);
+// pending timeout. Otherwise, it returns false.
-
-// PacketedBioAdvanceClock advances |bio|'s internal clock and returns true if
-// there is a pending timeout. Otherwise, it returns false.
 bool PacketedBioAdvanceClock(BIO *bio);