Bug 1398679 - Make cipher specs properly directional, r=ekr

This makes each cipher spec unidirectional.  This is a tiny bit less efficient
in TLS 1.2 and earlier, where some of the material could be shared (a few
pointers essentially) but it is much more efficient for TLS 1.3.

There is now only one variable of each type on the specs.  Up to now, the specs
had two copies of almost everything to support being used for both read and
write.  Now there are separate specs for reading and writing.  We only
duplicate the pointers to the master secret, and the cipher definitions.

This also does away with the backing array that was used to hold two copies of
specs.  Cipher specs are allocated on the heap as they are used and reference
counted, using the same system as is already used for TLS 1.3.

This uses the |direction| attribute that was previously added for TLS 1.3 and
uses that more thoroughly.

8 files changed, 83 insertions, 71 deletions

diff --git a/gtests/ssl_gtest/libssl_internals.c b/gtests/ssl_gtest/libssl_internals.c
index 6f6bb02ce..14d21d467 100644
--- a/gtests/ssl_gtest/libssl_internals.c
+++ b/gtests/ssl_gtest/libssl_internals.c
@@ -77,7 +77,7 @@ SECStatus SSLInt_SetMTU(PRFileDesc *fd, PRUint16 mtu) {
   return SECSuccess;
 }
 
-PRInt32 SSLInt_CountTls13CipherSpecs(PRFileDesc *fd) {
+PRInt32 SSLInt_CountCipherSpecs(PRFileDesc *fd) {
   PRCList *cur_p;
   PRInt32 ct = 0;
 
@@ -93,7 +93,7 @@ PRInt32 SSLInt_CountTls13CipherSpecs(PRFileDesc *fd) {
   return ct;
 }
 
-void SSLInt_PrintTls13CipherSpecs(const char *label, PRFileDesc *fd) {
+void SSLInt_PrintCipherSpecs(const char *label, PRFileDesc *fd) {
   PRCList *cur_p;
 
   sslSocket *ss = ssl_FindSocket(fd);
@@ -105,8 +105,8 @@ void SSLInt_PrintTls13CipherSpecs(const char *label, PRFileDesc *fd) {
   for (cur_p = PR_NEXT_LINK(&ss->ssl3.hs.cipherSpecs);
        cur_p != &ss->ssl3.hs.cipherSpecs; cur_p = PR_NEXT_LINK(cur_p)) {
     ssl3CipherSpec *spec = (ssl3CipherSpec *)cur_p;
-    fprintf(stderr, "  %s %s refct=%d\n", spec->phase,
-            spec->direction == CipherSpecRead ? "read" : "write", spec->refCt);
+    fprintf(stderr, "  %s spec epoch=%d (%s) refct=%d\n", SPEC_DIR(spec),
+            spec->epoch, spec->phase, spec->refCt);
   }
 }
 
@@ -244,17 +244,16 @@ SECStatus SSLInt_AdvanceReadSeqNum(PRFileDesc *fd, PRUint64 to) {
   }
   ssl_GetSpecWriteLock(ss);
   spec = ss->ssl3.crSpec;
-  spec->read_seq_num = to;
+  spec->seqNum = to;
 
   /* For DTLS, we need to fix the record sequence number.  For this, we can just
    * scrub the entire structure on the assumption that the new sequence number
    * is far enough past the last received sequence number. */
-  if (spec->read_seq_num <=
-      spec->recvdRecords.right + DTLS_RECVD_RECORDS_WINDOW) {
+  if (spec->seqNum <= spec->recvdRecords.right + DTLS_RECVD_RECORDS_WINDOW) {
     PORT_SetError(SEC_ERROR_INVALID_ARGS);
     return SECFailure;
   }
-  dtls_RecordSetRecvd(&spec->recvdRecords, spec->read_seq_num);
+  dtls_RecordSetRecvd(&spec->recvdRecords, spec->seqNum);
 
   ssl_ReleaseSpecWriteLock(ss);
   return SECSuccess;
@@ -272,7 +271,7 @@ SECStatus SSLInt_AdvanceWriteSeqNum(PRFileDesc *fd, PRUint64 to) {
     return SECFailure;
   }
   ssl_GetSpecWriteLock(ss);
-  ss->ssl3.cwSpec->write_seq_num = to;
+  ss->ssl3.cwSpec->seqNum = to;
   ssl_ReleaseSpecWriteLock(ss);
   return SECSuccess;
 }
@@ -286,7 +285,7 @@ SECStatus SSLInt_AdvanceWriteSeqByAWindow(PRFileDesc *fd, PRInt32 extra) {
     return SECFailure;
   }
   ssl_GetSpecReadLock(ss);
-  to = ss->ssl3.cwSpec->write_seq_num + DTLS_RECVD_RECORDS_WINDOW + extra;
+  to = ss->ssl3.cwSpec->seqNum + DTLS_RECVD_RECORDS_WINDOW + extra;
   ssl_ReleaseSpecReadLock(ss);
   return SSLInt_AdvanceWriteSeqNum(fd, to);
 }
@@ -314,25 +313,19 @@ SECStatus SSLInt_SetCipherSpecChangeFunc(PRFileDesc *fd,
   return SECSuccess;
 }
 
-static ssl3KeyMaterial *GetKeyingMaterial(PRBool isServer,
-                                          ssl3CipherSpec *spec) {
-  return isServer ? &spec->server : &spec->client;
+PK11SymKey *SSLInt_CipherSpecToKey(const ssl3CipherSpec *spec) {
+  return spec->keyMaterial.key;
 }
 
-PK11SymKey *SSLInt_CipherSpecToKey(PRBool isServer, ssl3CipherSpec *spec) {
-  return GetKeyingMaterial(isServer, spec)->write_key;
+SSLCipherAlgorithm SSLInt_CipherSpecToAlgorithm(const ssl3CipherSpec *spec) {
+  return spec->cipherDef->calg;
 }
 
-SSLCipherAlgorithm SSLInt_CipherSpecToAlgorithm(PRBool isServer,
-                                                ssl3CipherSpec *spec) {
-  return spec->cipher_def->calg;
+const PRUint8 *SSLInt_CipherSpecToIv(const ssl3CipherSpec *spec) {
+  return spec->keyMaterial.iv;
 }
 
-unsigned char *SSLInt_CipherSpecToIv(PRBool isServer, ssl3CipherSpec *spec) {
-  return GetKeyingMaterial(isServer, spec)->write_iv;
-}
-
-PRUint16 SSLInt_CipherSpecToEpoch(PRBool isServer, ssl3CipherSpec *spec) {
+PRUint16 SSLInt_CipherSpecToEpoch(const ssl3CipherSpec *spec) {
   return spec->epoch;
 }
 
diff --git a/gtests/ssl_gtest/libssl_internals.h b/gtests/ssl_gtest/libssl_internals.h
index bf95cba4f..e7ea9ef34 100644
--- a/gtests/ssl_gtest/libssl_internals.h
+++ b/gtests/ssl_gtest/libssl_internals.h
@@ -24,8 +24,8 @@ SECStatus SSLInt_UpdateSSLv2ClientRandom(PRFileDesc *fd, uint8_t *rnd,
 PRBool SSLInt_ExtensionNegotiated(PRFileDesc *fd, PRUint16 ext);
 void SSLInt_ClearSelfEncryptKey();
 void SSLInt_SetSelfEncryptMacKey(PK11SymKey *key);
-PRInt32 SSLInt_CountTls13CipherSpecs(PRFileDesc *fd);
-void SSLInt_PrintTls13CipherSpecs(const char *label, PRFileDesc *fd);
+PRInt32 SSLInt_CountCipherSpecs(PRFileDesc *fd);
+void SSLInt_PrintCipherSpecs(const char *label, PRFileDesc *fd);
 SECStatus SSLInt_ShiftDtlsTimers(PRFileDesc *fd, PRIntervalTime shift);
 SECStatus SSLInt_SetMTU(PRFileDesc *fd, PRUint16 mtu);
 PRBool SSLInt_CheckSecretsDestroyed(PRFileDesc *fd);
@@ -43,11 +43,10 @@ SSLKEAType SSLInt_GetKEAType(SSLNamedGroup group);
 SECStatus SSLInt_SetCipherSpecChangeFunc(PRFileDesc *fd,
                                          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);
-PRUint16 SSLInt_CipherSpecToEpoch(PRBool isServer, ssl3CipherSpec *spec);
+PRUint16 SSLInt_CipherSpecToEpoch(const ssl3CipherSpec *spec);
+PK11SymKey *SSLInt_CipherSpecToKey(const ssl3CipherSpec *spec);
+SSLCipherAlgorithm SSLInt_CipherSpecToAlgorithm(const ssl3CipherSpec *spec);
+const PRUint8 *SSLInt_CipherSpecToIv(const ssl3CipherSpec *spec);
 void SSLInt_SetTicketLifetime(uint32_t lifetime);
 void SSLInt_SetMaxEarlyDataSize(uint32_t size);
 SECStatus SSLInt_SetSocketMaxEarlyDataSize(PRFileDesc *fd, uint32_t size);
diff --git a/gtests/ssl_gtest/ssl_drop_unittest.cc b/gtests/ssl_gtest/ssl_drop_unittest.cc
index 9392ec400..bc8703eee 100644
--- a/gtests/ssl_gtest/ssl_drop_unittest.cc
+++ b/gtests/ssl_gtest/ssl_drop_unittest.cc
@@ -606,7 +606,7 @@ TEST_F(TlsDropDatagram13, ReorderServerEE) {
 class TlsSendCipherSpecCapturer {
  public:
   TlsSendCipherSpecCapturer(std::shared_ptr<TlsAgent>& agent)
-      : is_server_(agent->role() == TlsAgent::SERVER), send_cipher_specs_() {
+      : send_cipher_specs_() {
     SSLInt_SetCipherSpecChangeFunc(agent->ssl_fd(), CipherSpecChanged,
                                    (void*)this);
   }
@@ -628,16 +628,14 @@ class TlsSendCipherSpecCapturer {
     auto self = static_cast<TlsSendCipherSpecCapturer*>(arg);
 
     auto spec = std::make_shared<TlsCipherSpec>();
-    bool ret =
-        spec->Init(SSLInt_CipherSpecToEpoch(self->is_server_, newSpec),
-                   SSLInt_CipherSpecToAlgorithm(self->is_server_, newSpec),
-                   SSLInt_CipherSpecToKey(self->is_server_, newSpec),
-                   SSLInt_CipherSpecToIv(self->is_server_, newSpec));
+    bool ret = spec->Init(SSLInt_CipherSpecToEpoch(newSpec),
+                          SSLInt_CipherSpecToAlgorithm(newSpec),
+                          SSLInt_CipherSpecToKey(newSpec),
+                          SSLInt_CipherSpecToIv(newSpec));
     EXPECT_EQ(true, ret);
     self->send_cipher_specs_.push_back(spec);
   }
 
-  bool is_server_;
   std::vector<std::shared_ptr<TlsCipherSpec>> send_cipher_specs_;
 };
 
diff --git a/gtests/ssl_gtest/ssl_loopback_unittest.cc b/gtests/ssl_gtest/ssl_loopback_unittest.cc
index 49521916c..7a4abe5af 100644
--- a/gtests/ssl_gtest/ssl_loopback_unittest.cc
+++ b/gtests/ssl_gtest/ssl_loopback_unittest.cc
@@ -351,7 +351,7 @@ TEST_P(TlsHolddownTest, TestDtlsHolddownExpiry) {
   SendReceive();
   if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
     // One for send, one for receive.
-    EXPECT_EQ(2, SSLInt_CountTls13CipherSpecs(client_->ssl_fd()));
+    EXPECT_EQ(2, SSLInt_CountCipherSpecs(client_->ssl_fd()));
   }
 }
 
@@ -366,10 +366,8 @@ TEST_P(TlsHolddownTest, TestDtlsHolddownExpiryResumption) {
   Connect();
   RunAllTimersDown();
   SendReceive();
-  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
-    // One for send, one for receive.
-    EXPECT_EQ(2, SSLInt_CountTls13CipherSpecs(client_->ssl_fd()));
-  }
+  // One for send, one for receive.
+  EXPECT_EQ(2, SSLInt_CountCipherSpecs(client_->ssl_fd()));
 }
 
 class TlsPreCCSHeaderInjector : public TlsRecordFilter {
diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc
index 731a842a9..983106148 100644
--- a/gtests/ssl_gtest/tls_agent.cc
+++ b/gtests/ssl_gtest/tls_agent.cc
@@ -699,6 +699,50 @@ void TlsAgent::ResetPreliminaryInfo() {
   expected_cipher_suite_ = 0;
 }
 
+void TlsAgent::ValidateCipherSpecs() {
+  PRInt32 cipherSpecs = SSLInt_CountCipherSpecs(ssl_fd());
+  // We use one ciphersuite in each direction.
+  PRInt32 expected = 2;
+  if (variant_ == ssl_variant_datagram) {
+    // For DTLS 1.3, the client retains the cipher spec for early data and the
+    // handshake so that it can retransmit EndOfEarlyData and its final flight.
+    // It also retains the handshake read cipher spec so that it can read ACKs
+    // from the server. The server retains the handshake read cipher spec so it
+    // can read the client's retransmitted Finished.
+    if (expected_version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+      if (role_ == CLIENT) {
+        expected = info_.earlyDataAccepted ? 5 : 4;
+      } else {
+        expected = 3;
+      }
+    } else {
+      // For DTLS 1.1 and 1.2, the last endpoint to send maintains a cipher spec
+      // until the holddown timer runs down.
+      if (expect_resumption_) {
+        if (role_ == CLIENT) {
+          expected = 3;
+        }
+      } else {
+        if (role_ == SERVER) {
+          expected = 3;
+        }
+      }
+    }
+  }
+  // This function will be run before the handshake completes if false start is
+  // enabled.  In that case, the client will still be reading cleartext, but
+  // will have a spec prepared for reading ciphertext.  With DTLS, the client
+  // will also have a spec retained for retransmission of handshake messages.
+  if (role_ == CLIENT && falsestart_enabled_ && !handshake_callback_called_) {
+    EXPECT_GT(SSL_LIBRARY_VERSION_TLS_1_3, expected_version_);
+    expected = (variant_ == ssl_variant_datagram) ? 4 : 3;
+  }
+  EXPECT_EQ(expected, cipherSpecs);
+  if (expected != cipherSpecs) {
+    SSLInt_PrintCipherSpecs(role_str().c_str(), ssl_fd());
+  }
+}
+
 void TlsAgent::Connected() {
   if (state_ == STATE_CONNECTED) {
     return;
@@ -724,27 +768,7 @@ void TlsAgent::Connected() {
   EXPECT_EQ(SECSuccess, rv);
   EXPECT_EQ(sizeof(csinfo_), csinfo_.length);
 
-  if (expected_version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
-    PRInt32 cipherSuites = SSLInt_CountTls13CipherSpecs(ssl_fd());
-    // We use one ciphersuite in each direction.
-    PRInt32 expected = 2;
-    // For DTLS, the client retains the cipher spec for early data and the
-    // handshake so that it can retransmit EndOfEarlyData and its final flight.
-    // It also retains the handshake read cipher spec so that it can
-    // read ACKs from the server. The server retains the handshake read
-    // cipher spec so it can read the client's retransmitted Finished.
-    if (variant_ == ssl_variant_datagram) {
-      if (role_ == CLIENT) {
-        expected = info_.earlyDataAccepted ? 5 : 4;
-      } else {
-        expected = 3;
-      }
-    }
-    EXPECT_EQ(expected, cipherSuites);
-    if (expected != cipherSuites) {
-      SSLInt_PrintTls13CipherSpecs(role_str().c_str(), ssl_fd());
-    }
-  }
+  ValidateCipherSpecs();
 
   SetState(STATE_CONNECTED);
 }
diff --git a/gtests/ssl_gtest/tls_agent.h b/gtests/ssl_gtest/tls_agent.h
index 4569e6b3b..80191d4c7 100644
--- a/gtests/ssl_gtest/tls_agent.h
+++ b/gtests/ssl_gtest/tls_agent.h
@@ -258,6 +258,7 @@ class TlsAgent : public PollTarget {
   const static char* states[];
 
   void SetState(State state);
+  void ValidateCipherSpecs();
 
   // Dummy auth certificate hook.
   static SECStatus AuthCertificateHook(void* arg, PRFileDesc* fd,
diff --git a/gtests/ssl_gtest/tls_connect.cc b/gtests/ssl_gtest/tls_connect.cc
index ac8eb906b..bcf89e96a 100644
--- a/gtests/ssl_gtest/tls_connect.cc
+++ b/gtests/ssl_gtest/tls_connect.cc
@@ -293,11 +293,11 @@ void TlsConnectTestBase::CheckConnected() {
       variant_ == ssl_variant_datagram) {
     client_->Handshake();
     client_->Handshake();
-    auto suites = SSLInt_CountTls13CipherSpecs(client_->ssl_fd());
+    auto suites = SSLInt_CountCipherSpecs(client_->ssl_fd());
     // Verify that we dropped the client's retransmission cipher suites.
     EXPECT_EQ(2, suites) << "Client has the wrong number of suites";
     if (suites != 2) {
-      SSLInt_PrintTls13CipherSpecs("client", client_->ssl_fd());
+      SSLInt_PrintCipherSpecs("client", client_->ssl_fd());
     }
   }
   EXPECT_EQ(client_->version(), server_->version());
diff --git a/gtests/ssl_gtest/tls_filter.cc b/gtests/ssl_gtest/tls_filter.cc
index 74930703a..479317b0c 100644
--- a/gtests/ssl_gtest/tls_filter.cc
+++ b/gtests/ssl_gtest/tls_filter.cc
@@ -60,7 +60,8 @@ void TlsRecordFilter::CipherSpecChanged(void* arg, PRBool sending,
   if (g_ssl_gtest_verbose) {
     std::cerr << (isServer ? "server" : "client") << ": "
               << (sending ? "send" : "receive")
-              << " cipher spec changed:  " << newSpec->phase << std::endl;
+              << " cipher spec changed:  " << newSpec->epoch << " ("
+              << newSpec->phase << ")" << std::endl;
   }
   if (!sending) {
     return;
@@ -70,11 +71,9 @@ void TlsRecordFilter::CipherSpecChanged(void* arg, PRBool sending,
   self->out_sequence_number_ = 0;
   self->dropped_record_ = false;
   self->cipher_spec_.reset(new TlsCipherSpec());
-  bool ret =
-      self->cipher_spec_->Init(SSLInt_CipherSpecToEpoch(isServer, newSpec),
-                               SSLInt_CipherSpecToAlgorithm(isServer, newSpec),
-                               SSLInt_CipherSpecToKey(isServer, newSpec),
-                               SSLInt_CipherSpecToIv(isServer, newSpec));
+  bool ret = self->cipher_spec_->Init(
+      SSLInt_CipherSpecToEpoch(newSpec), SSLInt_CipherSpecToAlgorithm(newSpec),
+      SSLInt_CipherSpecToKey(newSpec), SSLInt_CipherSpecToIv(newSpec));
   EXPECT_EQ(true, ret);
 }