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 (primarily
the master secret), but it is much more efficient for TLS 1.3.

Also, 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.

Finally, this REMOVES compression support from libssl entirely.

--HG--
branch : NSS_TLS13_DRAFT19_BRANCH
extra : rebase_source : 9d8a05d08fcaec783c54e4a9c45105b964e607d8
extra : amend_source : d9aa17d8dce4ab8ff599657c9db215d91ad67090
extra : intermediate-source : 60552a0d8fc13732e06458ce9c6be9e8264a256b
extra : source : e9619fea6154a4e1c4fd731a96a9a6ac7e40135e

coreconf/config.gypi

@@ -96,7 +96,6 @@
     'mozilla_client%': 0,
     'moz_fold_libs%': 0,
     'moz_folded_library_name%': '',
-    'ssl_enable_zlib%': 1,
     'sanitizer_flags%': 0,
     'test_build%': 0,
     'no_zdefs%': 0,

coreconf/config.mk

@@ -172,7 +172,7 @@ endif
 
 # FIPS support requires startup tests to be executed at load time of shared modules.
 # For performance reasons, these tests are disabled by default.
-# When compiling binaries that must support FIPS mode, 
+# When compiling binaries that must support FIPS mode,
 # you should define NSS_FORCE_FIPS
 #
 # NSS_NO_INIT_SUPPORT is always defined on platforms that don't support
@@ -199,8 +199,3 @@ DEFINES += -DNO_NSPR_10_SUPPORT
 
 # Hide old, deprecated, TLS cipher suite names when building NSS
 DEFINES += -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES
-
-# Mozilla's mozilla/modules/zlib/src/zconf.h adds the MOZ_Z_ prefix to zlib
-# exported symbols, which causes problem when NSS is built as part of Mozilla.
-# So we add a NSS_SSL_ENABLE_ZLIB variable to allow Mozilla to turn this off.
-NSS_SSL_ENABLE_ZLIB = 1

gtests/nss_bogo_shim/Makefile

@@ -30,10 +30,6 @@ include ../common/gtest.mk
 
 CFLAGS += -I$(CORE_DEPTH)/lib/ssl
 
-ifdef NSS_SSL_ENABLE_ZLIB
-include $(CORE_DEPTH)/coreconf/zlib.mk
-endif
-
 #######################################################################
 # (5) Execute "global" rules. (OPTIONAL)                              #
 #######################################################################
@@ -48,5 +44,3 @@ include $(CORE_DEPTH)/coreconf/rules.mk
 #######################################################################
 # (7) Execute "local" rules. (OPTIONAL).                              #
 #######################################################################
-
-

gtests/ssl_gtest/Makefile

@@ -29,10 +29,6 @@ include ../common/gtest.mk
 
 CFLAGS += -I$(CORE_DEPTH)/lib/ssl
 
-ifdef NSS_SSL_ENABLE_ZLIB
-include $(CORE_DEPTH)/coreconf/zlib.mk
-endif
-
 ifdef NSS_DISABLE_TLS_1_3
 NSS_DISABLE_TLS_1_3=1
 # Run parameterized tests only, for which we can easily exclude TLS 1.3

gtests/ssl_gtest/libssl_internals.c

@@ -76,7 +76,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;
 
@@ -92,7 +92,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);
@@ -104,8 +104,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);
   }
 }
 
@@ -241,17 +241,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;
@@ -269,7 +268,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;
 }
@@ -283,7 +282,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);
 }
@@ -311,25 +310,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;
 }
 

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);
 void SSLInt_ForceRtTimerExpiry(PRFileDesc *fd);
 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);

gtests/ssl_gtest/ssl_drop_unittest.cc

@@ -426,7 +426,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);
   }
@@ -448,16 +448,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_;
 };
 

gtests/ssl_gtest/ssl_loopback_unittest.cc

@@ -323,10 +323,8 @@ TEST_P(TlsConnectDatagram, TestDtlsHolddownExpiry) {
   SSLInt_ForceRtTimerExpiry(client_->ssl_fd());
   SSLInt_ForceRtTimerExpiry(server_->ssl_fd());
   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 {

gtests/ssl_gtest/tls_agent.cc

@@ -699,6 +699,49 @@ 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;
+  // 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 (variant_ == ssl_variant_datagram) {
+    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;
+        }
+      }
+    }
+  }
+  // A client using false start will still be reading cleartext while it has a
+  // spec prepared for reading ciphertext.  In DTLS, that also means being
+  // prepared 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 +767,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);
 }

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,

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());

gtests/ssl_gtest/tls_filter.cc

@@ -59,7 +59,7 @@ 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 << std::endl;
   }
   if (!sending) {
     return;
@@ -69,11 +69,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);
 }
 

lib/ssl/config.mk

@@ -57,11 +57,6 @@ endif
 
 endif
 
-ifdef NSS_SSL_ENABLE_ZLIB
-DEFINES += -DNSS_SSL_ENABLE_ZLIB
-include $(CORE_DEPTH)/coreconf/zlib.mk
-endif
-
 ifdef NSS_DISABLE_TLS_1_3
 DEFINES += -DNSS_DISABLE_TLS_1_3
 endif

lib/ssl/dtls13con.c

@@ -244,36 +244,6 @@ dtls13_HandleOutOfEpochRecord(sslSocket *ss, const ssl3CipherSpec *spec,
     return SECFailure;
 }
 
-/* Store the null cipher spec with the right refct. */
-SECStatus
-dtls13_SaveNullCipherSpec(sslSocket *ss, const ssl3CipherSpec *crSpec)
-{
-    ssl3CipherSpec *spec;
-    extern const char kKeyPhaseCleartext[];
-    PORT_Assert(IS_DTLS(ss));
-
-    spec = PORT_ZNew(ssl3CipherSpec);
-    if (!spec) {
-        PORT_SetError(SEC_ERROR_NO_MEMORY);
-        return SECFailure;
-    }
-    spec->refCt = 1;
-    spec->cipher_def = crSpec->cipher_def;
-    spec->mac_def = crSpec->mac_def;
-    spec->decode = crSpec->decode;
-    spec->epoch = crSpec->epoch;
-    PORT_Memcpy(&spec->recvdRecords, &crSpec->recvdRecords,
-                sizeof(spec->recvdRecords));
-    spec->direction = CipherSpecRead;
-    spec->phase = kKeyPhaseCleartext;
-    spec->read_seq_num = crSpec->write_seq_num;
-    spec->refCt = 1;
-
-    PR_APPEND_LINK(&spec->link, &ss->ssl3.hs.cipherSpecs);
-
-    return SECSuccess;
-}
-
 SECStatus
 dtls13_HandleAck(sslSocket *ss, sslBuffer *databuf)
 {