Bug 1340841 - Ticket lifetime duration is in seconds, r=ekr
authorMartin Thomson <martin.thomson@gmail.com>
Mon, 20 Feb 2017 10:02:05 +1100
changeset 13161 93b99b0936d3926be0ea3f7b69bc81ec46475494
parent 13159 68583fcd9b35e40b462cd9d019307f627fb384f4
child 13164 d8a46a65d1be25e6524651b06ceb619f9bfebf56
push id2039
push usermartin.thomson@gmail.com
push dateTue, 21 Feb 2017 01:12:36 +0000
reviewersekr
bugs1340841
Bug 1340841 - Ticket lifetime duration is in seconds, r=ekr
gtests/ssl_gtest/libssl_internals.c
gtests/ssl_gtest/libssl_internals.h
gtests/ssl_gtest/ssl_resumption_unittest.cc
gtests/ssl_gtest/tls_connect.cc
lib/ssl/ssl3con.c
lib/ssl/ssl3exthandle.c
lib/ssl/sslimpl.h
lib/ssl/sslnonce.c
lib/ssl/tls13con.c
--- a/gtests/ssl_gtest/libssl_internals.c
+++ b/gtests/ssl_gtest/libssl_internals.c
@@ -362,8 +362,12 @@ SECStatus SSLInt_UsingShortHeaders(PRFil
   if (!ss) {
     return SECFailure;
   }
 
   *result = ss->ssl3.hs.shortHeaders;
 
   return SECSuccess;
 }
+
+void SSLInt_SetTicketLifetime(uint32_t lifetime) {
+  ssl_ticket_lifetime = lifetime;
+}
--- a/gtests/ssl_gtest/libssl_internals.h
+++ b/gtests/ssl_gtest/libssl_internals.h
@@ -44,10 +44,11 @@ SECStatus SSLInt_SetCipherSpecChangeFunc
                                          sslCipherSpecChangedFunc func,
                                          void *arg);
 PK11SymKey *SSLInt_CipherSpecToKey(PRBool isServer, ssl3CipherSpec *spec);
 SSLCipherAlgorithm SSLInt_CipherSpecToAlgorithm(PRBool isServer,
                                                 ssl3CipherSpec *spec);
 unsigned char *SSLInt_CipherSpecToIv(PRBool isServer, ssl3CipherSpec *spec);
 SECStatus SSLInt_EnableShortHeaders(PRFileDesc *fd);
 SECStatus SSLInt_UsingShortHeaders(PRFileDesc *fd, PRBool *result);
+void SSLInt_SetTicketLifetime(uint32_t lifetime);
 
 #endif  // ndef libssl_internals_h_
--- a/gtests/ssl_gtest/ssl_resumption_unittest.cc
+++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc
@@ -196,16 +196,73 @@ TEST_P(TlsConnectGeneric, ConnectResumeC
   Reset();
   ClearServerCache();
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   ExpectResumption(RESUME_NONE);
   Connect();
   SendReceive();
 }
 
+TEST_P(TlsConnectGeneric, ConnectWithExpiredTicketAtClient) {
+  SSLInt_SetTicketLifetime(1);  // one second
+  // This causes a ticket resumption.
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  Connect();
+  SendReceive();
+
+  WAIT_(false, 1000);
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ExpectResumption(RESUME_NONE);
+
+  // TLS 1.3 uses the pre-shared key extension instead.
+  SSLExtensionType xtn = (version_ >= SSL_LIBRARY_VERSION_TLS_1_3)
+                             ? ssl_tls13_pre_shared_key_xtn
+                             : ssl_session_ticket_xtn;
+  auto capture = std::make_shared<TlsExtensionCapture>(xtn);
+  client_->SetPacketFilter(capture);
+  Connect();
+
+  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    EXPECT_FALSE(capture->captured());
+  } else {
+    EXPECT_TRUE(capture->captured());
+    EXPECT_EQ(0U, capture->extension().len());
+  }
+}
+
+TEST_P(TlsConnectGeneric, ConnectWithExpiredTicketAtServer) {
+  SSLInt_SetTicketLifetime(1);  // one second
+  // This causes a ticket resumption.
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  Connect();
+  SendReceive();
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ExpectResumption(RESUME_NONE);
+
+  SSLExtensionType xtn = (version_ >= SSL_LIBRARY_VERSION_TLS_1_3)
+                             ? ssl_tls13_pre_shared_key_xtn
+                             : ssl_session_ticket_xtn;
+  auto capture = std::make_shared<TlsExtensionCapture>(xtn);
+  client_->SetPacketFilter(capture);
+  client_->StartConnect();
+  server_->StartConnect();
+  client_->Handshake();
+  EXPECT_TRUE(capture->captured());
+  EXPECT_LT(0U, capture->extension().len());
+
+  WAIT_(false, 1000);  // Let the ticket expire on the server.
+
+  Handshake();
+  CheckConnected();
+}
+
 // This callback switches out the "server" cert used on the server with
 // the "client" certificate, which should be the same type.
 static int32_t SwitchCertificates(TlsAgent* agent, const SECItem* srvNameArr,
                                   uint32_t srvNameArrSize) {
   bool ok = agent->ConfigServerCert("client");
   if (!ok) return SSL_SNI_SEND_ALERT;
 
   return 0;  // first config
--- a/gtests/ssl_gtest/tls_connect.cc
+++ b/gtests/ssl_gtest/tls_connect.cc
@@ -169,16 +169,17 @@ void TlsConnectTestBase::ClearServerCach
   SSL_ShutdownServerSessionIDCache();
   SSLInt_ClearSessionTicketKey();
   SSL_ConfigServerSessionIDCache(1024, 0, 0, g_working_dir_path.c_str());
 }
 
 void TlsConnectTestBase::SetUp() {
   SSL_ConfigServerSessionIDCache(1024, 0, 0, g_working_dir_path.c_str());
   SSLInt_ClearSessionTicketKey();
+  SSLInt_SetTicketLifetime(10);
   ClearStats();
   Init();
 }
 
 void TlsConnectTestBase::TearDown() {
   client_ = nullptr;
   server_ = nullptr;
 
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -10276,17 +10276,17 @@ ssl3_SendNewSessionTicket(sslSocket *ss)
     /* Serialize the handshake message. Length =
      * lifetime (4) + ticket length (2) + ticket. */
     rv = ssl3_AppendHandshakeHeader(ss, new_session_ticket,
                                     4 + 2 + ticket.len);
     if (rv != SECSuccess)
         goto loser;
 
     /* This is a fixed value. */
-    rv = ssl3_AppendHandshakeNumber(ss, TLS_EX_SESS_TICKET_LIFETIME_HINT, 4);
+    rv = ssl3_AppendHandshakeNumber(ss, ssl_ticket_lifetime, 4);
     if (rv != SECSuccess)
         goto loser;
 
     /* Encode the ticket. */
     rv = ssl3_AppendHandshakeVariable(ss, ticket.data, ticket.len, 2);
     if (rv != SECSuccess)
         goto loser;
 
--- a/lib/ssl/ssl3exthandle.c
+++ b/lib/ssl/ssl3exthandle.c
@@ -845,16 +845,19 @@ ssl3_ClientHandleStatusRequestXtn(const 
         return SECFailure;
     }
 
     /* Keep track of negotiated extensions. */
     xtnData->negotiated[xtnData->numNegotiated++] = ex_type;
     return SECSuccess;
 }
 
+PRUint32 ssl_ticket_lifetime = 2 * 24 * 60 * 60; /* 2 days in seconds */
+#define TLS_EX_SESS_TICKET_VERSION (0x0103)
+
 /*
  * Called from ssl3_SendNewSessionTicket, tls13_SendNewSessionTicket
  */
 SECStatus
 ssl3_EncodeSessionTicket(sslSocket *ss,
                          const NewSessionTicket *ticket,
                          SECItem *ticket_data)
 {
@@ -1562,18 +1565,17 @@ ssl3_ProcessSessionTicketCommon(sslSocke
     /* Done parsing.  Check that all bytes have been consumed. */
     if (buffer_len != padding_length)
         goto no_ticket;
 
     /* Use the ticket if it has not expired, otherwise free the allocated
      * memory since the ticket is of no use.
      */
     if (parsed_session_ticket->timestamp != 0 &&
-        parsed_session_ticket->timestamp +
-                TLS_EX_SESS_TICKET_LIFETIME_HINT >
+        parsed_session_ticket->timestamp + ssl_ticket_lifetime >
             ssl_Time()) {
 
         sid = ssl3_NewSessionID(ss, PR_TRUE);
         if (sid == NULL) {
             rv = SECFailure;
             goto loser;
         }
 
@@ -1618,16 +1620,17 @@ ssl3_ProcessSessionTicketCommon(sslSocke
                 goto loser;
             }
         }
         if (parsed_session_ticket->srvName.data != NULL) {
             if (sid->u.ssl3.srvName.data) {
                 SECITEM_FreeItem(&sid->u.ssl3.srvName, PR_FALSE);
             }
             sid->u.ssl3.srvName = parsed_session_ticket->srvName;
+            parsed_session_ticket->srvName.data = NULL;
         }
         if (parsed_session_ticket->alpnSelection.data != NULL) {
             sid->u.ssl3.alpnSelection = parsed_session_ticket->alpnSelection;
             /* So we don't free below. */
             parsed_session_ticket->alpnSelection.data = NULL;
         }
         ss->statelessResume = PR_TRUE;
         ss->sec.ci.sid = sid;
@@ -1657,16 +1660,19 @@ loser:
 
     if (parsed_session_ticket != NULL) {
         if (parsed_session_ticket->peer_cert.data) {
             SECITEM_FreeItem(&parsed_session_ticket->peer_cert, PR_FALSE);
         }
         if (parsed_session_ticket->alpnSelection.data) {
             SECITEM_FreeItem(&parsed_session_ticket->alpnSelection, PR_FALSE);
         }
+        if (parsed_session_ticket->srvName.data) {
+            SECITEM_FreeItem(&parsed_session_ticket->srvName, PR_FALSE);
+        }
         PORT_ZFree(parsed_session_ticket, sizeof(SessionTicket));
     }
 
     return rv;
 }
 
 SECStatus
 ssl3_ServerHandleSessionTicketXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUint16 ex_type,
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -1216,26 +1216,22 @@ struct sslSocketStr {
     /* True when the current session is a stateless resume. */
     PRBool statelessResume;
     TLSExtensionData xtnData;
 
     /* Whether we are doing stream or datagram mode */
     SSLProtocolVariant protocolVariant;
 };
 
-/* All the global data items declared here should be protected using the
-** ssl_global_data_lock, which is a reader/writer lock.
-*/
-extern NSSRWLock *ssl_global_data_lock;
 extern char ssl_debug;
 extern char ssl_trace;
 extern FILE *ssl_trace_iob;
 extern FILE *ssl_keylog_iob;
-extern PRUint32 ssl_sid_timeout;
 extern PRUint32 ssl3_sid_timeout;
+extern PRUint32 ssl_ticket_lifetime;
 
 extern const char *const ssl3_cipherName[];
 
 extern sslSessionIDLookupFunc ssl_sid_lookup;
 extern sslSessionIDCacheFunc ssl_sid_cache;
 extern sslSessionIDUncacheFunc ssl_sid_uncache;
 
 extern const sslNamedGroupDef ssl_named_groups[];
@@ -1693,20 +1689,16 @@ extern void ssl3_SetSIDSessionTicket(ssl
 SECStatus ssl3_EncodeSessionTicket(sslSocket *ss,
                                    const NewSessionTicket *ticket_input,
                                    SECItem *ticket_data);
 SECStatus ssl_MaybeSetSessionTicketKeyPair(const sslKeyPair *keyPair);
 SECStatus ssl_GetSessionTicketKeys(sslSocket *ss, unsigned char *keyName,
                                    PK11SymKey **encKey, PK11SymKey **macKey);
 void ssl_ResetSessionTicketKeys();
 
-/* Tell clients to consider tickets valid for this long. */
-#define TLS_EX_SESS_TICKET_LIFETIME_HINT (2 * 24 * 60 * 60) /* 2 days */
-#define TLS_EX_SESS_TICKET_VERSION (0x0103)
-
 extern SECStatus ssl3_ValidateNextProtoNego(const unsigned char *data,
                                             unsigned int length);
 
 /* Construct a new NSPR socket for the app to use */
 extern PRFileDesc *ssl_NewPRSocket(sslSocket *ss, PRFileDesc *fd);
 extern void ssl_FreePRSocket(PRFileDesc *fd);
 
 /* Internal config function so SSL3 can initialize the present state of
--- a/lib/ssl/sslnonce.c
+++ b/lib/ssl/sslnonce.c
@@ -14,17 +14,16 @@
 
 #include "sslimpl.h"
 #include "sslproto.h"
 #include "nssilock.h"
 #if defined(XP_UNIX) || defined(XP_WIN) || defined(_WINDOWS) || defined(XP_BEOS)
 #include <time.h>
 #endif
 
-PRUint32 ssl_sid_timeout = 100;
 PRUint32 ssl3_sid_timeout = 86400L; /* 24 hours */
 
 static sslSessionID *cache = NULL;
 static PZLock *cacheLock = NULL;
 
 /* sids can be in one of 4 states:
  *
  * never_cached,        created, but not yet put into cache.
@@ -466,17 +465,17 @@ ssl_TicketTimeValid(const NewSessionTick
 {
     PRTime endTime;
 
     if (ticket->ticket_lifetime_hint == 0) {
         return PR_TRUE;
     }
 
     endTime = ticket->received_timestamp +
-              (PRTime)(ticket->ticket_lifetime_hint * PR_USEC_PER_MSEC);
+              (PRTime)(ticket->ticket_lifetime_hint * PR_USEC_PER_SEC);
     return endTime > PR_Now();
 }
 
 void
 ssl3_SetSIDSessionTicket(sslSessionID *sid,
                          /*in/out*/ NewSessionTicket *newSessionTicket)
 {
     PORT_Assert(sid);
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -3803,17 +3803,17 @@ tls13_SendNewSessionTicket(sslSocket *ss
     SECStatus rv;
     NewSessionTicket ticket = { 0 };
     PRUint32 max_early_data_size_len = 0;
     ticket.flags = 0;
     if (ss->opt.enable0RttData) {
         ticket.flags |= ticket_allow_early_data;
         max_early_data_size_len = 8; /* type + len + value. */
     }
-    ticket.ticket_lifetime_hint = TLS_EX_SESS_TICKET_LIFETIME_HINT;
+    ticket.ticket_lifetime_hint = ssl_ticket_lifetime;
 
     rv = ssl3_EncodeSessionTicket(ss, &ticket, &ticket_data);
     if (rv != SECSuccess)
         goto loser;
 
     message_length =
         4 +                           /* lifetime */
         4 +                           /* ticket_age_add */
@@ -3822,17 +3822,17 @@ tls13_SendNewSessionTicket(sslSocket *ss
         ticket_data.len;
 
     rv = ssl3_AppendHandshakeHeader(ss, new_session_ticket,
                                     message_length);
     if (rv != SECSuccess)
         goto loser;
 
     /* This is a fixed value. */
-    rv = ssl3_AppendHandshakeNumber(ss, TLS_EX_SESS_TICKET_LIFETIME_HINT, 4);
+    rv = ssl3_AppendHandshakeNumber(ss, ssl_ticket_lifetime, 4);
     if (rv != SECSuccess)
         goto loser;
 
     /* The ticket age obfuscator. */
     rv = PK11_GenerateRandom((PRUint8 *)&ticket.ticket_age_add,
                              sizeof(ticket.ticket_age_add));
     if (rv != SECSuccess)
         goto loser;