Bug 1543874 - Use an external clock for SSL functions, r=ekr,kevinjacobs

Summary:
This adds a new (experimental) API that allows users of libssl to provide their
own clock function.  This is primarily of use in testing, but it also enables
our QUIC implementation, which also runs off an external clock.

SSL Sockets (and session IDs, when they are in memory) now have a "now()"
function and void* arg attached to them.  By default, this is a function that
calls PR_Now().  These values are copied from the socket to any session ID that
is created from the socket, and to any session ID that is restored from the
session cache.

The ssl_Time() and ssl_TimeUsec() functions have been removed.

As part of this, the experimental SSL_SetupAntiReplay() function had to be
modified to take an external clock (PR_Now() suffices generally).  That function
relies on knowing the time, and it doesn't have a socket to work from.  To avoid
problems arising from the change in the signature, SSL_SetupAntiReplay is now
removed.

There are now three uses of time in the library:

* The primary source of time runs of these newly added functions.  This governs
  session expiry, 0-RTT checks, and related functions.

* The session cache uses a separate time to manage its locking.  This is of type
  PRUint32 in seconds (rather than PRTime in microseconds).  In investigating
  this, I found several places where this time in seconds was leaking across to
  the main functions via the lastAccessTime property.  That was fixed.  The
  cache functions that use time now all call ssl_CacheNow() to get time.

* DTLS timers run using PRIntervalTime.  This is a little annoying and these
  could be made to use the main time source, but that would result in
  conversions between PRTime and PRIntervalTime at the DTLS API.  PRIntervalTime
  has a different epoch to PRTime, so this would be a little awkward.

Only the first of these can be controlled using the new API.

Bugs found:

* Expiration time of resumption tokens was based on the sid->expirationTime,
  which didn't account for the lifetime provided by the server.  These are now
  capped by the minimum of ssl_ticket_lifetime and the value the server
  indicates.

  I removed ssl3_sid_timeout, the old limit, because inconsistent lifetimes
  between client and server messed with tests.  The client would have a lower
  cap than the server, which prevented testing of the enforcement of server
  limits without jumping through hoops.

* There was a missing time conversion in tls13_InWindow which made the window
  checks too lenient.

* lastAccessTime was being set to seconds-since-epoch instead of
  microseconds-since-epoch in a few places.

Reviewers: ekr, KevinJacobs

Reviewed By: KevinJacobs

Subscribers: cjpatton

Bug #: 1543874

Differential Revision: https://phabricator.services.mozilla.com/D27238

--HG--
extra : rebase_source : 3317ecc00f37fc09f0e7c36e947dcd162d1d258a
extra : amend_source : cf2bfb90a05911e0a0cc76bd377d99ccca8e1900

cmd/selfserv/selfserv.c

@@ -1954,7 +1954,7 @@ server_main(
         if (enabledVersions.max < SSL_LIBRARY_VERSION_TLS_1_3) {
             errExit("You tried enabling 0RTT without enabling TLS 1.3!");
         }
-        rv = SSL_SetupAntiReplay(10 * PR_USEC_PER_SEC, 7, 14);
+        rv = SSL_InitAntiReplay(PR_Now(), 10L * PR_USEC_PER_SEC, 7, 14);
         if (rv != SECSuccess) {
             errExit("error configuring anti-replay ");
         }

fuzz/tls_client_target.cc

@@ -106,6 +106,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t len) {
   // Probably not too important for clients.
   SSL_SetURL(ssl_fd, "server");
 
+  FixTime(ssl_fd);
   SetSocketOptions(ssl_fd, config);
   EnableAllCipherSuites(ssl_fd);
   SetupCallbacks(ssl_fd, config.get());

fuzz/tls_common.cc

@@ -5,9 +5,18 @@
 #include <assert.h>
 
 #include "ssl.h"
+#include "sslexp.h"
 
 #include "tls_common.h"
 
+static PRTime FixedTime(void*) { return 1234; }
+
+// Fix the time input, to avoid any time-based variation.
+void FixTime(PRFileDesc* fd) {
+  SECStatus rv = SSL_SetTimeFunc(fd, FixedTime, nullptr);
+  assert(rv == SECSuccess);
+}
+
 PRStatus EnableAllProtocolVersions() {
   SSLVersionRange supported;
 

fuzz/tls_common.h

@@ -7,6 +7,7 @@
 
 #include "prinit.h"
 
+void FixTime(PRFileDesc* fd);
 PRStatus EnableAllProtocolVersions();
 void EnableAllCipherSuites(PRFileDesc* fd);
 void DoHandshake(PRFileDesc* fd, bool isServer);

fuzz/tls_server_target.cc

@@ -118,6 +118,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t len) {
   PRFileDesc* ssl_fd = ImportFD(model.get(), fd.get());
   assert(ssl_fd == fd.get());
 
+  FixTime(ssl_fd);
   SetSocketOptions(ssl_fd, config);
   DoHandshake(ssl_fd, true);
 

gtests/ssl_gtest/libssl_internals.c

@@ -109,9 +109,10 @@ void SSLInt_PrintCipherSpecs(const char *label, PRFileDesc *fd) {
   }
 }
 
-/* Force a timer expiry by backdating when all active timers were started. We
- * could set the remaining time to 0 but then backoff would not work properly if
- * we decide to test it. */
+/* DTLS timers are separate from the time that the rest of the stack uses.
+ * Force a timer expiry by backdating when all active timers were started.
+ * We could set the remaining time to 0 but then backoff would not work properly
+ * if we decide to test it. */
 SECStatus SSLInt_ShiftDtlsTimers(PRFileDesc *fd, PRIntervalTime shift) {
   size_t i;
   sslSocket *ss = ssl_FindSocket(fd);
@@ -297,10 +298,6 @@ SSLKEAType SSLInt_GetKEAType(SSLNamedGroup group) {
   return groupDef->keaType;
 }
 
-void SSLInt_SetTicketLifetime(uint32_t lifetime) {
-  ssl_ticket_lifetime = lifetime;
-}
-
 SECStatus SSLInt_SetSocketMaxEarlyDataSize(PRFileDesc *fd, uint32_t size) {
   sslSocket *ss;
 
@@ -324,10 +321,6 @@ SECStatus SSLInt_SetSocketMaxEarlyDataSize(PRFileDesc *fd, uint32_t size) {
   return SECSuccess;
 }
 
-void SSLInt_RolloverAntiReplay(void) {
-  tls13_AntiReplayRollover(ssl_TimeUsec());
-}
-
 SECStatus SSLInt_HasPendingHandshakeData(PRFileDesc *fd, PRBool *pending) {
   sslSocket *ss = ssl_FindSocket(fd);
   if (!ss) {

gtests/ssl_gtest/libssl_internals.h

@@ -40,8 +40,6 @@ SECStatus SSLInt_AdvanceReadSeqNum(PRFileDesc *fd, PRUint64 to);
 SECStatus SSLInt_AdvanceWriteSeqByAWindow(PRFileDesc *fd, PRInt32 extra);
 SSLKEAType SSLInt_GetKEAType(SSLNamedGroup group);
 SECStatus SSLInt_HasPendingHandshakeData(PRFileDesc *fd, PRBool *pending);
-void SSLInt_SetTicketLifetime(uint32_t lifetime);
 SECStatus SSLInt_SetSocketMaxEarlyDataSize(PRFileDesc *fd, uint32_t size);
-void SSLInt_RolloverAntiReplay(void);
 
 #endif  // ndef libssl_internals_h_

gtests/ssl_gtest/ssl_0rtt_unittest.cc

@@ -46,7 +46,7 @@ TEST_P(TlsConnectTls13, ZeroRttServerRejectByOption) {
 }
 
 TEST_P(TlsConnectTls13, ZeroRttApparentReplayAfterRestart) {
-  // The test fixtures call SSL_SetupAntiReplay() in SetUp().  This results in
+  // The test fixtures call SSL_InitAntiReplay() in SetUp().  This results in
   // 0-RTT being rejected until at least one window passes.  SetupFor0Rtt()
   // forces a rollover of the anti-replay filters, which clears this state.
   // Here, we do the setup manually here without that forced rollover.
@@ -106,7 +106,7 @@ class TlsZeroRttReplayTest : public TlsConnectTls13 {
     SendReceive();
 
     if (rollover) {
-      SSLInt_RolloverAntiReplay();
+      RolloverAntiReplay();
     }
 
     // Now replay that packet against the server.
@@ -184,20 +184,21 @@ TEST_P(TlsConnectTls13, ZeroRttServerOnly) {
   CheckKeys();
 }
 
-// A small sleep after sending the ClientHello means that the ticket age that
-// arrives at the server is too low.  With a small tolerance for variation in
-// ticket age (which is determined by the |window| parameter that is passed to
-// SSL_SetupAntiReplay()), the server then rejects early data.
+// Advancing time after sending the ClientHello means that the ticket age that
+// arrives at the server is too low.  The server then rejects early data if this
+// delay exceeds half the anti-replay window.
 TEST_P(TlsConnectTls13, ZeroRttRejectOldTicket) {
+  static const PRTime kWindow = 10 * PR_USEC_PER_SEC;
+  EXPECT_EQ(SECSuccess, SSL_InitAntiReplay(now(), kWindow, 1, 3));
   SetupForZeroRtt();
+
+  Reset();
+  StartConnect();
   client_->Set0RttEnabled(true);
   server_->Set0RttEnabled(true);
-  EXPECT_EQ(SECSuccess, SSL_SetupAntiReplay(1, 1, 3));
-  SSLInt_RolloverAntiReplay();  // Make sure to flush replay state.
-  SSLInt_RolloverAntiReplay();
   ExpectResumption(RESUME_TICKET);
-  ZeroRttSendReceive(true, false, []() {
-    PR_Sleep(PR_MillisecondsToInterval(10));
+  ZeroRttSendReceive(true, false, [this]() {
+    AdvanceTime(1 + kWindow / 2);
     return true;
   });
   Handshake();
@@ -212,13 +213,15 @@ TEST_P(TlsConnectTls13, ZeroRttRejectOldTicket) {
 // small tolerance for variation in ticket age and the ticket will appear to
 // arrive prematurely, causing the server to reject early data.
 TEST_P(TlsConnectTls13, ZeroRttRejectPrematureTicket) {
+  static const PRTime kWindow = 10 * PR_USEC_PER_SEC;
+  EXPECT_EQ(SECSuccess, SSL_InitAntiReplay(now(), kWindow, 1, 3));
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
   server_->Set0RttEnabled(true);
   StartConnect();
   client_->Handshake();  // ClientHello
   server_->Handshake();  // ServerHello
-  PR_Sleep(PR_MillisecondsToInterval(10));
+  AdvanceTime(1 + kWindow / 2);
   Handshake();  // Remainder of handshake
   CheckConnected();
   SendReceive();
@@ -227,9 +230,6 @@ TEST_P(TlsConnectTls13, ZeroRttRejectPrematureTicket) {
   Reset();
   client_->Set0RttEnabled(true);
   server_->Set0RttEnabled(true);
-  EXPECT_EQ(SECSuccess, SSL_SetupAntiReplay(1, 1, 3));
-  SSLInt_RolloverAntiReplay();  // Make sure to flush replay state.
-  SSLInt_RolloverAntiReplay();
   ExpectResumption(RESUME_TICKET);
   ExpectEarlyDataAccepted(false);
   StartConnect();
@@ -870,6 +870,59 @@ TEST_F(TlsConnectDatagram13, ZeroRttShortReadDtls) {
   CheckConnected();
 }
 
+// There are few ways in which TLS uses the clock and most of those operate on
+// timescales that would be ridiculous to wait for in a test.  This is the one
+// test we have that uses the real clock.  It tests that time passes by checking
+// that a small sleep results in rejection of early data. 0-RTT has a
+// configurable timer, which makes it ideal for this.
+TEST_F(TlsConnectStreamTls13, TimePassesByDefault) {
+  // Set a tiny anti-replay window.  This has to be at least 2 milliseconds to
+  // have any chance of being relevant as that is the smallest window that we
+  // can detect.  Anything smaller rounds to zero.
+  static const unsigned int kTinyWindowMs = 5;
+  EXPECT_EQ(SECSuccess, SSL_InitAntiReplay(
+                            PR_Now(), kTinyWindowMs * PR_USEC_PER_MSEC, 1, 5));
+
+  // Calling EnsureTlsSetup() replaces the time function on client and server,
+  // which we don't want, so initialize each directly.
+  client_->EnsureTlsSetup();
+  server_->EnsureTlsSetup();
+  client_->StartConnect();  // Also avoid StartConnect().
+  server_->StartConnect();
+
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  server_->Set0RttEnabled(true);
+  Handshake();
+  CheckConnected();
+  SendReceive();  // Absorb a session ticket.
+  CheckKeys();
+
+  // Clear the first window.
+  PR_Sleep(PR_MillisecondsToInterval(kTinyWindowMs));
+
+  Reset();
+  client_->EnsureTlsSetup();
+  server_->EnsureTlsSetup();
+  client_->StartConnect();
+  server_->StartConnect();
+
+  // Early data is rejected by the server only if time passes for it as well.
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+  ExpectResumption(RESUME_TICKET);
+  ZeroRttSendReceive(true, false, []() {
+    // Sleep long enough that we minimize the risk of our RTT estimation being
+    // duped by stutters in test execution.  This is very long to allow for
+    // flaky and low-end hardware, especially what our CI runs on.
+    PR_Sleep(PR_MillisecondsToInterval(1000));
+    return true;
+  });
+  Handshake();
+  ExpectEarlyDataAccepted(false);
+  CheckConnected();
+}
+
 #ifndef NSS_DISABLE_TLS_1_3
 INSTANTIATE_TEST_CASE_P(Tls13ZeroRttReplayTest, TlsZeroRttReplayTest,
                         TlsConnectTestBase::kTlsVariantsAll);

gtests/ssl_gtest/ssl_fuzz_unittest.cc

@@ -22,7 +22,7 @@ namespace nss_test {
 const uint8_t kShortEmptyFinished[8] = {0};
 const uint8_t kLongEmptyFinished[128] = {0};
 
-class TlsFuzzTest : public ::testing::Test {};
+class TlsFuzzTest : public TlsConnectGeneric {};
 
 // Record the application data stream.
 class TlsApplicationDataRecorder : public TlsRecordFilter {
@@ -46,16 +46,9 @@ class TlsApplicationDataRecorder : public TlsRecordFilter {
   DataBuffer buffer_;
 };
 
-// Ensure that ssl_Time() returns a constant value.
-FUZZ_F(TlsFuzzTest, SSL_Time_Constant) {
-  PRUint32 now = ssl_TimeSec();
-  PR_Sleep(PR_SecondsToInterval(2));
-  EXPECT_EQ(ssl_TimeSec(), now);
-}
-
 // Check that due to the deterministic PRNG we derive
 // the same master secret in two consecutive TLS sessions.
-FUZZ_P(TlsConnectGeneric, DeterministicExporter) {
+FUZZ_P(TlsFuzzTest, DeterministicExporter) {
   const char kLabel[] = "label";
   std::vector<unsigned char> out1(32), out2(32);
 
@@ -95,7 +88,7 @@ FUZZ_P(TlsConnectGeneric, DeterministicExporter) {
 
 // Check that due to the deterministic RNG two consecutive
 // TLS sessions will have the exact same transcript.
-FUZZ_P(TlsConnectGeneric, DeterministicTranscript) {
+FUZZ_P(TlsFuzzTest, DeterministicTranscript) {
   // Make sure we have RSA blinding params.
   Connect();
 
@@ -130,9 +123,7 @@ FUZZ_P(TlsConnectGeneric, DeterministicTranscript) {
 // with all supported TLS versions, STREAM and DGRAM.
 // Check that records are NOT encrypted.
 // Check that records don't have a MAC.
-FUZZ_P(TlsConnectGeneric, ConnectSendReceive_NullCipher) {
-  EnsureTlsSetup();
-
+FUZZ_P(TlsFuzzTest, ConnectSendReceive_NullCipher) {
   // Set up app data filters.
   auto client_recorder = MakeTlsFilter<TlsApplicationDataRecorder>(client_);
   auto server_recorder = MakeTlsFilter<TlsApplicationDataRecorder>(server_);
@@ -157,7 +148,7 @@ FUZZ_P(TlsConnectGeneric, ConnectSendReceive_NullCipher) {
 }
 
 // Check that an invalid Finished message doesn't abort the connection.
-FUZZ_P(TlsConnectGeneric, BogusClientFinished) {
+FUZZ_P(TlsFuzzTest, BogusClientFinished) {
   EnsureTlsSetup();
 
   MakeTlsFilter<TlsInspectorReplaceHandshakeMessage>(
@@ -168,7 +159,7 @@ FUZZ_P(TlsConnectGeneric, BogusClientFinished) {
 }
 
 // Check that an invalid Finished message doesn't abort the connection.
-FUZZ_P(TlsConnectGeneric, BogusServerFinished) {
+FUZZ_P(TlsFuzzTest, BogusServerFinished) {
   EnsureTlsSetup();
 
   MakeTlsFilter<TlsInspectorReplaceHandshakeMessage>(
@@ -179,7 +170,7 @@ FUZZ_P(TlsConnectGeneric, BogusServerFinished) {
 }
 
 // Check that an invalid server auth signature doesn't abort the connection.
-FUZZ_P(TlsConnectGeneric, BogusServerAuthSignature) {
+FUZZ_P(TlsFuzzTest, BogusServerAuthSignature) {
   EnsureTlsSetup();
   uint8_t msg_type = version_ == SSL_LIBRARY_VERSION_TLS_1_3
                          ? kTlsHandshakeCertificateVerify
@@ -190,7 +181,7 @@ FUZZ_P(TlsConnectGeneric, BogusServerAuthSignature) {
 }
 
 // Check that an invalid client auth signature doesn't abort the connection.
-FUZZ_P(TlsConnectGeneric, BogusClientAuthSignature) {
+FUZZ_P(TlsFuzzTest, BogusClientAuthSignature) {
   EnsureTlsSetup();
   client_->SetupClientAuth();
   server_->RequestClientAuth(true);
@@ -199,7 +190,7 @@ FUZZ_P(TlsConnectGeneric, BogusClientAuthSignature) {
 }
 
 // Check that session ticket resumption works.
-FUZZ_P(TlsConnectGeneric, SessionTicketResumption) {
+FUZZ_P(TlsFuzzTest, SessionTicketResumption) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   Connect();
   SendReceive();
@@ -212,31 +203,53 @@ FUZZ_P(TlsConnectGeneric, SessionTicketResumption) {
 }
 
 // Check that session tickets are not encrypted.
-FUZZ_P(TlsConnectGeneric, UnencryptedSessionTickets) {
+FUZZ_P(TlsFuzzTest, UnencryptedSessionTickets) {
   ConfigureSessionCache(RESUME_TICKET, RESUME_TICKET);
 
   auto filter = MakeTlsFilter<TlsHandshakeRecorder>(
       server_, kTlsHandshakeNewSessionTicket);
   Connect();
 
   std::cerr << "ticket" << filter->buffer() << std::endl;
-  size_t offset = 4; /* lifetime */
+  size_t offset = 4;  // Skip lifetime.
+
   if (version_ == SSL_LIBRARY_VERSION_TLS_1_3) {
-    offset += 4; /* ticket_age_add */
+    offset += 4;  // Skip ticket_age_add.
     uint32_t nonce_len = 0;
     EXPECT_TRUE(filter->buffer().Read(offset, 1, &nonce_len));
     offset += 1 + nonce_len;
   }
-  offset += 2 + /* ticket length */
-            2;  /* TLS_EX_SESS_TICKET_VERSION */
+
+  offset += 2;  // Skip the ticket length.
+
+  // This bit parses the contents of the ticket, which would ordinarily be
+  // encrypted.  Start by checking that we have the right version.  This needs
+  // to be updated every time that TLS_EX_SESS_TICKET_VERSION is changed.  But
+  // we don't use the #define.  That way, any time that code is updated, this
+  // test will fail unless it is manually checked.
+  uint32_t ticket_version;
+  EXPECT_TRUE(filter->buffer().Read(offset, 2, &ticket_version));
+  EXPECT_EQ(0x010aU, ticket_version);
+  offset += 2;
+
   // Check the protocol version number.
   uint32_t tls_version = 0;
   EXPECT_TRUE(filter->buffer().Read(offset, sizeof(version_), &tls_version));
   EXPECT_EQ(version_, static_cast<decltype(version_)>(tls_version));
+  offset += sizeof(version_);
 
   // Check the cipher suite.
   uint32_t suite = 0;
-  EXPECT_TRUE(filter->buffer().Read(offset + sizeof(version_), 2, &suite));
+  EXPECT_TRUE(filter->buffer().Read(offset, 2, &suite));
   client_->CheckCipherSuite(static_cast<uint16_t>(suite));
 }
+
+INSTANTIATE_TEST_CASE_P(
+    FuzzStream, TlsFuzzTest,
+    ::testing::Combine(TlsConnectTestBase::kTlsVariantsStream,
+                       TlsConnectTestBase::kTlsVAll));
+INSTANTIATE_TEST_CASE_P(
+    FuzzDatagram, TlsFuzzTest,
+    ::testing::Combine(TlsConnectTestBase::kTlsVariantsDatagram,
+                       TlsConnectTestBase::kTlsV11Plus));
 }