Bug 1315865 - Automatic KeyUpdate to avoid cipher exhaustion, r=ekr

7 files changed, 177 insertions, 32 deletions

diff --git a/gtests/ssl_gtest/ssl_ciphersuite_unittest.cc b/gtests/ssl_gtest/ssl_ciphersuite_unittest.cc
index 98d7899aa..810656868 100644
--- a/gtests/ssl_gtest/ssl_ciphersuite_unittest.cc
+++ b/gtests/ssl_gtest/ssl_ciphersuite_unittest.cc
@@ -235,27 +235,29 @@ TEST_P(TlsCipherSuiteTest, ResumeCipherSuite) {
   ConnectAndCheckCipherSuite();
 }
 
-// This only works for stream ciphers because we modify the sequence number -
-// which is included explicitly in the DTLS record header - and that trips a
-// different error code.  Note that the message that the client sends would not
-// decrypt (the nonce/IV wouldn't match), but the record limit is hit before
-// attempting to decrypt a record.
 TEST_P(TlsCipherSuiteTest, ReadLimit) {
   SetupCertificate();
   EnableSingleCipher();
   ConnectAndCheckCipherSuite();
-  EXPECT_EQ(SECSuccess,
-            SSLInt_AdvanceWriteSeqNum(client_->ssl_fd(), last_safe_write()));
-  EXPECT_EQ(SECSuccess,
-            SSLInt_AdvanceReadSeqNum(server_->ssl_fd(), last_safe_write()));
+  if (version_ < SSL_LIBRARY_VERSION_TLS_1_3) {
+    uint64_t last = last_safe_write();
+    EXPECT_EQ(SECSuccess, SSLInt_AdvanceWriteSeqNum(client_->ssl_fd(), last));
+    EXPECT_EQ(SECSuccess, SSLInt_AdvanceReadSeqNum(server_->ssl_fd(), last));
 
-  client_->SendData(10, 10);
-  server_->ReadBytes();  // This should be OK.
+    client_->SendData(10, 10);
+    server_->ReadBytes();  // This should be OK.
+  } else {
+    // In TLS 1.3, reading or writing triggers a KeyUpdate.  That would mean
+    // that the sequence numbers would reset and we wouldn't hit the limit.  So
+    // we move the sequence number to one less than the limit directly and don't
+    // test sending and receiving just before the limit.
+    uint64_t last = record_limit() - 1;
+    EXPECT_EQ(SECSuccess, SSLInt_AdvanceReadSeqNum(server_->ssl_fd(), last));
+  }
 
-  // The payload needs to be big enough to pass for encrypted.  In the extreme
-  // case (TLS 1.3), this means 1 for payload, 1 for content type and 16 for
-  // authentication tag.
-  static const uint8_t payload[18] = {6};
+  // The payload needs to be big enough to pass for encrypted.  The code checks
+  // the limit before it tries to decrypt.
+  static const uint8_t payload[32] = {6};
   DataBuffer record;
   uint64_t epoch;
   if (variant_ == ssl_variant_datagram) {
@@ -270,13 +272,17 @@ TEST_P(TlsCipherSuiteTest, ReadLimit) {
   TlsAgentTestBase::MakeRecord(variant_, kTlsApplicationDataType, version_,
                                payload, sizeof(payload), &record,
                                (epoch << 48) | record_limit());
-  server_->adapter()->PacketReceived(record);
+  client_->SendDirect(record);
   server_->ExpectReadWriteError();
   server_->ReadBytes();
   EXPECT_EQ(SSL_ERROR_TOO_MANY_RECORDS, server_->error_code());
 }
 
 TEST_P(TlsCipherSuiteTest, WriteLimit) {
+  // This asserts in TLS 1.3 because we expect an automatic update.
+  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    return;
+  }
   SetupCertificate();
   EnableSingleCipher();
   ConnectAndCheckCipherSuite();
diff --git a/gtests/ssl_gtest/ssl_keyupdate_unittest.cc b/gtests/ssl_gtest/ssl_keyupdate_unittest.cc
index c2d03aa48..d03775c25 100644
--- a/gtests/ssl_gtest/ssl_keyupdate_unittest.cc
+++ b/gtests/ssl_gtest/ssl_keyupdate_unittest.cc
@@ -116,4 +116,63 @@ TEST_F(TlsConnectTest, KeyUpdateBothRequest) {
   CheckEpochs(5, 5);
 }
 
+// If the sequence number exceeds the number of writes before an automatic
+// update (currently 3/4 of the max records for the cipher suite), then the
+// stack should send an update automatically (but not request one).
+TEST_F(TlsConnectTest, KeyUpdateAutomaticOnWrite) {
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  ConnectWithCipherSuite(TLS_AES_128_GCM_SHA256);
+
+  // Set this to one below the write threshold.
+  uint64_t threshold = (0x5aULL << 28) * 3 / 4;
+  EXPECT_EQ(SECSuccess,
+            SSLInt_AdvanceWriteSeqNum(client_->ssl_fd(), threshold));
+  EXPECT_EQ(SECSuccess, SSLInt_AdvanceReadSeqNum(server_->ssl_fd(), threshold));
+
+  // This should be OK.
+  client_->SendData(10);
+  server_->ReadBytes();
+
+  // This should cause the client to update.
+  client_->SendData(10);
+  server_->ReadBytes();
+
+  SendReceive(100);
+  CheckEpochs(4, 3);
+}
+
+// If the sequence number exceeds a certain number of reads (currently 7/8 of
+// the max records for the cipher suite), then the stack should send AND request
+// an update automatically.  However, the sender (client) will be above its
+// automatic update threshold, so the KeyUpdate - that it sends with the old
+// cipher spec - will exceed the receiver (server) automatic update threshold.
+// The receiver gets a packet with a sequence number over its automatic read
+// update threshold.  Even though the sender has updated, the code that checks
+// the sequence numbers at the receiver doesn't know this and it will request an
+// update.  This causes two updates: one from the sender (without requesting a
+// response) and one from the receiver (which does request a response).
+TEST_F(TlsConnectTest, KeyUpdateAutomaticOnRead) {
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  ConnectWithCipherSuite(TLS_AES_128_GCM_SHA256);
+
+  // Move to right at the read threshold.  Unlike the write test, we can't send
+  // packets because that would cause the client to update, which would spoil
+  // the test.
+  uint64_t threshold = ((0x5aULL << 28) * 7 / 8) + 1;
+  EXPECT_EQ(SECSuccess,
+            SSLInt_AdvanceWriteSeqNum(client_->ssl_fd(), threshold));
+  EXPECT_EQ(SECSuccess, SSLInt_AdvanceReadSeqNum(server_->ssl_fd(), threshold));
+
+  // This should cause the client to update, but not early enough to prevent the
+  // server from updating also.
+  client_->SendData(10);
+  server_->ReadBytes();
+
+  // Need two SendReceive() calls to ensure that the update that the server
+  // requested is properly generated and consumed.
+  SendReceive(70);
+  SendReceive(80);
+  CheckEpochs(5, 4);
+}
+
 }  // namespace nss_test
diff --git a/gtests/ssl_gtest/tls_connect.cc b/gtests/ssl_gtest/tls_connect.cc
index 2eca87aea..0af5123e9 100644
--- a/gtests/ssl_gtest/tls_connect.cc
+++ b/gtests/ssl_gtest/tls_connect.cc
@@ -172,13 +172,13 @@ void TlsConnectTestBase::CheckEpochs(uint16_t client_epoch,
 
   EXPECT_EQ(SECSuccess,
             SSLInt_GetEpochs(client_->ssl_fd(), &read_epoch, &write_epoch));
-  EXPECT_EQ(server_epoch, read_epoch);
-  EXPECT_EQ(client_epoch, write_epoch);
+  EXPECT_EQ(server_epoch, read_epoch) << "client read epoch";
+  EXPECT_EQ(client_epoch, write_epoch) << "client write epoch";
 
   EXPECT_EQ(SECSuccess,
             SSLInt_GetEpochs(server_->ssl_fd(), &read_epoch, &write_epoch));
-  EXPECT_EQ(client_epoch, read_epoch);
-  EXPECT_EQ(server_epoch, write_epoch);
+  EXPECT_EQ(client_epoch, read_epoch) << "server read epoch";
+  EXPECT_EQ(server_epoch, write_epoch) << "server write epoch";
 }
 
 void TlsConnectTestBase::ClearStats() {
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 2b4d203ad..61878ae99 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -2205,6 +2205,8 @@ ssl_ProtectRecord(sslSocket *ss, ssl3CipherSpec *cwSpec, SSL3ContentType type,
     PORT_Assert(SSL_BUFFER_LEN(wrBuf) == 0);
     PORT_Assert(cwSpec->cipherDef->max_records <= RECORD_SEQ_MAX);
     if (cwSpec->seqNum >= cwSpec->cipherDef->max_records) {
+        /* We should have automatically updated before here in TLS 1.3. */
+        PORT_Assert(cwSpec->version < SSL_LIBRARY_VERSION_TLS_1_3);
         SSL_TRC(3, ("%d: SSL[-]: write sequence number at limit 0x%0llx",
                     SSL_GETPID(), cwSpec->seqNum));
         PORT_SetError(SSL_ERROR_TOO_MANY_RECORDS);
diff --git a/lib/ssl/sslsecur.c b/lib/ssl/sslsecur.c
index 1081f082a..11fa93043 100644
--- a/lib/ssl/sslsecur.c
+++ b/lib/ssl/sslsecur.c
@@ -762,6 +762,55 @@ ssl_SecureShutdown(sslSocket *ss, int nsprHow)
 
 /************************************************************************/
 
+static SECStatus
+tls13_CheckKeyUpdate(sslSocket *ss, CipherSpecDirection dir)
+{
+    PRBool keyUpdate;
+    ssl3CipherSpec *spec;
+    sslSequenceNumber seqNum;
+    sslSequenceNumber margin;
+    SECStatus rv;
+
+    /* Bug 1413368: enable for DTLS */
+    if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3 || IS_DTLS(ss)) {
+        return SECSuccess;
+    }
+
+    /* If both sides update at the same number, then this will cause two updates
+     * to happen at once. The problem is that the KeyUpdate itself consumes a
+     * sequence number, and that will trigger the reading side to request an
+     * update.
+     *
+     * If we have the writing side update first, the writer will be the one that
+     * drives the update.  An update by the writer doesn't need a response, so
+     * it is more efficient overall.  The margins here are pretty arbitrary, but
+     * having the write margin larger reduces the number of times that a
+     * KeyUpdate is sent by a reader. */
+    ssl_GetSpecReadLock(ss);
+    if (dir == CipherSpecRead) {
+        spec = ss->ssl3.crSpec;
+        margin = spec->cipherDef->max_records / 8;
+    } else {
+        spec = ss->ssl3.cwSpec;
+        margin = spec->cipherDef->max_records / 4;
+    }
+    seqNum = spec->seqNum;
+    keyUpdate = seqNum > spec->cipherDef->max_records - margin;
+    ssl_ReleaseSpecReadLock(ss);
+    if (!keyUpdate) {
+        return SECSuccess;
+    }
+
+    SSL_TRC(5, ("%d: SSL[%d]: automatic key update at %llx for %s cipher spec",
+                SSL_GETPID(), ss->fd, seqNum,
+                (dir == CipherSpecRead) ? "read" : "write"));
+    ssl_GetSSL3HandshakeLock(ss);
+    rv = tls13_SendKeyUpdate(ss, (dir == CipherSpecRead) ? update_requested : update_not_requested,
+                             dir == CipherSpecWrite /* buffer */);
+    ssl_ReleaseSSL3HandshakeLock(ss);
+    return rv;
+}
+
 int
 ssl_SecureRecv(sslSocket *ss, unsigned char *buf, int len, int flags)
 {
@@ -801,6 +850,10 @@ ssl_SecureRecv(sslSocket *ss, unsigned char *buf, int len, int flags)
             rv = ssl_Do1stHandshake(ss);
         }
         ssl_Release1stHandshakeLock(ss);
+    } else {
+        if (tls13_CheckKeyUpdate(ss, CipherSpecRead) != SECSuccess) {
+            rv = PR_FAILURE;
+        }
     }
     if (rv < 0) {
         return rv;
@@ -884,11 +937,19 @@ ssl_SecureSend(sslSocket *ss, const unsigned char *buf, int len, int flags)
         }
         ssl_Release1stHandshakeLock(ss);
     }
+
     if (rv < 0) {
         ss->writerThread = NULL;
         goto done;
     }
 
+    if (ss->firstHsDone) {
+        if (tls13_CheckKeyUpdate(ss, CipherSpecWrite) != SECSuccess) {
+            rv = PR_FAILURE;
+            goto done;
+        }
+    }
+
     if (zeroRtt) {
         /* There's a limit to the number of early data octets we can send.
          *
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index f481b6609..23082fdbf 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -639,13 +639,8 @@ tls13_UpdateTrafficKeys(sslSocket *ss, CipherSpecDirection direction)
     return SECSuccess;
 }
 
-typedef enum {
-    update_not_requested = 0,
-    update_requested = 1
-} tls13KeyUpdateRequest;
-
-static SECStatus
-tls13_SendKeyUpdate(sslSocket *ss, tls13KeyUpdateRequest request)
+SECStatus
+tls13_SendKeyUpdate(sslSocket *ss, tls13KeyUpdateRequest request, PRBool buffer)
 {
     SECStatus rv;
 
@@ -685,7 +680,9 @@ tls13_SendKeyUpdate(sslSocket *ss, tls13KeyUpdateRequest request)
         goto loser;
     }
 
-    rv = ssl3_FlushHandshake(ss, 0);
+    /* If we have been asked to buffer, then do so.  This allows us to coalesce
+     * a KeyUpdate with a pending write. */
+    rv = ssl3_FlushHandshake(ss, buffer ? ssl_SEND_FLAG_FORCE_INTO_BUFFER : 0);
     if (rv != SECSuccess) {
         goto loser; /* error code set by ssl3_FlushHandshake */
     }
@@ -712,8 +709,20 @@ SSLExp_KeyUpdate(PRFileDesc *fd, PRBool requestUpdate)
         return SECFailure;
     }
 
+    if (!ss->firstHsDone) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return SECFailure;
+    }
+
+    rv = TLS13_CHECK_HS_STATE(ss, SEC_ERROR_INVALID_ARGS,
+                              idle_handshake);
+    if (rv != SECSuccess) {
+        return SECFailure;
+    }
+
     ssl_GetSSL3HandshakeLock(ss);
-    rv = tls13_SendKeyUpdate(ss, requestUpdate ? update_requested : update_not_requested);
+    rv = tls13_SendKeyUpdate(ss, requestUpdate ? update_requested : update_not_requested,
+                             PR_FALSE /* don't buffer */);
 
     /* Remember that we are the ones that initiated this KeyUpdate. */
     if (rv == SECSuccess) {
@@ -766,8 +775,8 @@ tls13_HandleKeyUpdate(sslSocket *ss, PRUint8 *b, unsigned int length)
         FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_KEY_UPDATE, decode_error);
         return SECFailure;
     }
-    if (update != update_requested &&
-        update != update_not_requested) {
+    if (!(update == update_requested ||
+          update == update_not_requested)) {
         FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_KEY_UPDATE, decode_error);
         return SECFailure;
     }
@@ -789,7 +798,8 @@ tls13_HandleKeyUpdate(sslSocket *ss, PRUint8 *b, unsigned int length)
             sendUpdate = PR_TRUE;
         }
         if (sendUpdate) {
-            rv = tls13_SendKeyUpdate(ss, update_not_requested);
+            /* Respond immediately (don't buffer). */
+            rv = tls13_SendKeyUpdate(ss, update_not_requested, PR_FALSE);
             if (rv != SECSuccess) {
                 return SECFailure; /* Error already set. */
             }
diff --git a/lib/ssl/tls13con.h b/lib/ssl/tls13con.h
index 07f56f07d..1aaffb651 100644
--- a/lib/ssl/tls13con.h
+++ b/lib/ssl/tls13con.h
@@ -18,6 +18,11 @@ typedef enum {
     tls13_extension_unknown
 } tls13ExtensionStatus;
 
+typedef enum {
+    update_not_requested = 0,
+    update_requested = 1
+} tls13KeyUpdateRequest;
+
 #define TLS13_MAX_FINISHED_SIZE 64
 
 SECStatus tls13_UnprotectRecord(
@@ -108,6 +113,8 @@ SECStatus SSLExp_SetupAntiReplay(PRTime window, unsigned int k,
 SECStatus SSLExp_HelloRetryRequestCallback(PRFileDesc *fd,
                                            SSLHelloRetryRequestCallback cb,
                                            void *arg);
+SECStatus tls13_SendKeyUpdate(sslSocket *ss, tls13KeyUpdateRequest request,
+                              PRBool buffer);
 SECStatus SSLExp_KeyUpdate(PRFileDesc *fd, PRBool requestUpdate);
 PRBool tls13_MaybeTls13(sslSocket *ss);
 void tls13_SetSpecRecordVersion(sslSocket *ss, ssl3CipherSpec *spec);