From 1b22f85a5673ed5078737481231b4daa58a2b6f0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 27 Oct 2016 16:36:32 -0400 Subject: [PATCH] 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 --- ssl/ssl_session.c | 6 ++++++ ssl/ssl_test.cc | 12 ++++++++++++ ssl/test/bssl_shim.cc | 15 ++++++++++----- ssl/test/packeted_bio.cc | 33 ++++++++++++--------------------- ssl/test/packeted_bio.h | 13 +++++-------- 5 files changed, 45 insertions(+), 34 deletions(-) diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c index c2396b17..2c20bc3c 100644 --- a/ssl/ssl_session.c +++ b/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; } diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 1c4ba7fe..55ac9231 100644 --- a/ssl/ssl_test.cc +++ b/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 server_ctx(SSL_CTX_new(TLS_method())); bssl::UniquePtr 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; diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 2eee8885..08b81f30 100644 --- a/ssl/test/bssl_shim.cc +++ b/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 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 *out_session, return false; } if (config->is_dtls) { - bssl::UniquePtr packeted = PacketedBioCreate(!config->async); + bssl::UniquePtr 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 = SetupCtx(&config); if (!ssl_ctx) { ERR_print_errors_fp(stderr); diff --git a/ssl/test/packeted_bio.cc b/ssl/test/packeted_bio.cc index f7267fc6..8331b4b6 100644 --- a/ssl/test/packeted_bio.cc +++ b/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) - : advance_clock(advance_clock_arg) { + PacketedBio(timeval *clock_arg, bool 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) { - return clock.tv_usec < read_deadline.tv_usec; + if (clock->tv_sec == read_deadline.tv_sec) { + 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)); -} - // 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 PacketedBioCreate(bool advance_clock) { +bssl::UniquePtr PacketedBioCreate(timeval *clock, bool advance_clock) { bssl::UniquePtr 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_sec += data->clock.tv_usec / 1000000; - data->clock.tv_usec %= 1000000; - data->clock.tv_sec += data->timeout.tv_sec; + data->clock->tv_usec += data->timeout.tv_usec; + data->clock->tv_sec += data->clock->tv_usec / 1000000; + data->clock->tv_usec %= 1000000; + data->clock->tv_sec += data->timeout.tv_sec; memset(&data->timeout, 0, sizeof(data->timeout)); return true; } diff --git a/ssl/test/packeted_bio.h b/ssl/test/packeted_bio.h index 07930d47..9d4cdcbb 100644 --- a/ssl/test/packeted_bio.h +++ b/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 -// |BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT| based on it. +// blocking datagram socket. It uses the value of |*clock| as the clock and +// 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 PacketedBioCreate(bool advance_clock); +bssl::UniquePtr PacketedBioCreate(timeval *clock, bool advance_clock); -// PacketedBioGetClock returns the current time for |bio|. -timeval PacketedBioGetClock(const BIO *bio); - -// PacketedBioAdvanceClock advances |bio|'s internal clock and returns true if -// there is a pending timeout. Otherwise, it returns false. +// PacketedBioAdvanceClock advances |bio|'s clock and returns true if there is a +// pending timeout. Otherwise, it returns false. bool PacketedBioAdvanceClock(BIO *bio);