Bug 1592007 - land NSS NSS_3_48_BETA1 UPGRADE_NSS_RELEASE, r=kjacobs a=jcristau
authorJ.C. Jones <jc@mozilla.com>
Tue, 03 Dec 2019 06:19:08 +0200
changeset 566678 56717f3c48219bfe7c16934908c674bfd4c720af
parent 566677 fbc9674496f7d56e7ccf5b583ae5386b03ecc6e7
child 566679 f7c6c838e79df777942723c5abdd0662acf8b04d
push id12373
push userccoroiu@mozilla.com
push dateWed, 04 Dec 2019 10:47:12 +0000
treeherdermozilla-beta@b69378d81dcf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskjacobs, jcristau
bugs1592007, 1593401, 1600775, 1599545, 1597799
milestone72.0
Bug 1592007 - land NSS NSS_3_48_BETA1 UPGRADE_NSS_RELEASE, r=kjacobs a=jcristau 2019-12-02 Kevin Jacobs <kjacobs@mozilla.com> * lib/ssl/sslsnce.c: Bug 1593401 - Fix race condition in self-encrypt functions r=mt,jcj [77976f3fefca] [NSS_3_48_BETA1] 2019-12-02 J.C. Jones <jjones@mozilla.com> * automation/release/nspr-version.txt: Bug 1600775 - Require NSPR 4.24 for NSS 3.48 r=kaie,kjacobs [b6141fb86799] * gtests/ssl_gtest/tls_filter.h: Bug 1599545 - fixup, clang-format r=me [8ffef87ef51b] 2019-12-02 Kevin Jacobs <kjacobs@mozilla.com> * cpputil/tls_parser.h, gtests/ssl_gtest/ssl_keyupdate_unittest.cc, gtests/ssl_gtest/tls_filter.h, lib/ssl/tls13con.c: Bug 1599545 - Fix assertion and add test for early Key Update message r=mt Remove an overzealous assertion when a Key Update message is received too early, and add a test for the expected alert condition. Also adds `TlsEncryptedHandshakeMessageReplacer` for replacing TLS 1.3 encrypted handshake messages. This is a simple implementation where only the first byte of the message is changed to the new type (so as to trigger the desired handler). [a5dbf68d182d] 2019-11-27 J.C. Jones <jjones@mozilla.com> * lib/ckfw/object.c: Bug 1597799 - Guard against null ptrs in NSSCKFWObject r=kjacobs There's a bunch of similar code that could use guards in here, but I wanted to be minimal for this patch. [eab4d3c8c76d] Differential Revision: https://phabricator.services.mozilla.com//D55581
security/nss/TAG-INFO
security/nss/automation/release/nspr-version.txt
security/nss/coreconf/coreconf.dep
security/nss/cpputil/tls_parser.h
security/nss/gtests/ssl_gtest/ssl_keyupdate_unittest.cc
security/nss/gtests/ssl_gtest/tls_filter.h
security/nss/lib/ckfw/object.c
security/nss/lib/ssl/sslsnce.c
security/nss/lib/ssl/tls13con.c
--- a/security/nss/TAG-INFO
+++ b/security/nss/TAG-INFO
@@ -1,1 +1,1 @@
-10722c590949
\ No newline at end of file
+NSS_3_48_BETA1
\ No newline at end of file
--- a/security/nss/automation/release/nspr-version.txt
+++ b/security/nss/automation/release/nspr-version.txt
@@ -1,9 +1,9 @@
-4.23
+4.24
 
 # The first line of this file must contain the human readable NSPR
 # version number, which is the minimum required version of NSPR
 # that is supported by this version of NSS.
 #
 # This information is used by release automation,
 # when creating an NSS source archive.
 #
--- a/security/nss/coreconf/coreconf.dep
+++ b/security/nss/coreconf/coreconf.dep
@@ -5,8 +5,9 @@
 
 /*
  * A dummy header file that is a dependency for all the object files.
  * Used to force a full recompilation of NSS in Mozilla's Tinderbox
  * depend builds.  See comments in rules.mk.
  */
 
 #error "Do not include this header file."
+
--- a/security/nss/cpputil/tls_parser.h
+++ b/security/nss/cpputil/tls_parser.h
@@ -26,16 +26,17 @@ const uint8_t kTlsHandshakeNewSessionTic
 const uint8_t kTlsHandshakeHelloRetryRequest = 6;
 const uint8_t kTlsHandshakeEncryptedExtensions = 8;
 const uint8_t kTlsHandshakeCertificate = 11;
 const uint8_t kTlsHandshakeServerKeyExchange = 12;
 const uint8_t kTlsHandshakeCertificateRequest = 13;
 const uint8_t kTlsHandshakeCertificateVerify = 15;
 const uint8_t kTlsHandshakeClientKeyExchange = 16;
 const uint8_t kTlsHandshakeFinished = 20;
+const uint8_t kTlsHandshakeKeyUpdate = 24;
 
 const uint8_t kTlsAlertWarning = 1;
 const uint8_t kTlsAlertFatal = 2;
 
 const uint8_t kTlsAlertCloseNotify = 0;
 const uint8_t kTlsAlertUnexpectedMessage = 10;
 const uint8_t kTlsAlertBadRecordMac = 20;
 const uint8_t kTlsAlertRecordOverflow = 22;
--- a/security/nss/gtests/ssl_gtest/ssl_keyupdate_unittest.cc
+++ b/security/nss/gtests/ssl_gtest/ssl_keyupdate_unittest.cc
@@ -28,16 +28,47 @@ TEST_F(TlsConnectTest, KeyUpdateClient) 
   ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
   Connect();
   EXPECT_EQ(SECSuccess, SSL_KeyUpdate(client_->ssl_fd(), PR_FALSE));
   SendReceive(50);
   SendReceive(60);
   CheckEpochs(4, 3);
 }
 
+TEST_F(TlsConnectStreamTls13, KeyUpdateTooEarly_Client) {
+  StartConnect();
+  auto filter = MakeTlsFilter<TlsEncryptedHandshakeMessageReplacer>(
+      server_, kTlsHandshakeFinished, kTlsHandshakeKeyUpdate);
+  filter->EnableDecryption();
+
+  client_->Handshake();
+  server_->Handshake();
+  ExpectAlert(client_, kTlsAlertUnexpectedMessage);
+  client_->Handshake();
+  client_->CheckErrorCode(SSL_ERROR_RX_UNEXPECTED_KEY_UPDATE);
+  server_->Handshake();
+  server_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT);
+}
+
+TEST_F(TlsConnectStreamTls13, KeyUpdateTooEarly_Server) {
+  StartConnect();
+  auto filter = MakeTlsFilter<TlsEncryptedHandshakeMessageReplacer>(
+      client_, kTlsHandshakeFinished, kTlsHandshakeKeyUpdate);
+  filter->EnableDecryption();
+
+  client_->Handshake();
+  server_->Handshake();
+  client_->Handshake();
+  ExpectAlert(server_, kTlsAlertUnexpectedMessage);
+  server_->Handshake();
+  server_->CheckErrorCode(SSL_ERROR_RX_UNEXPECTED_KEY_UPDATE);
+  client_->Handshake();
+  client_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT);
+}
+
 TEST_F(TlsConnectTest, KeyUpdateClientRequestUpdate) {
   ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
   Connect();
   EXPECT_EQ(SECSuccess, SSL_KeyUpdate(client_->ssl_fd(), PR_TRUE));
   // SendReceive() only gives each peer one chance to read.  This isn't enough
   // when the read on one side generates another handshake message.  A second
   // read gives each peer an extra chance to consume the KeyUpdate.
   SendReceive(50);
--- a/security/nss/gtests/ssl_gtest/tls_filter.h
+++ b/security/nss/gtests/ssl_gtest/tls_filter.h
@@ -449,16 +449,77 @@ class TlsHandshakeDropper : public TlsHa
  protected:
   PacketFilter::Action FilterHandshake(const HandshakeHeader& header,
                                        const DataBuffer& input,
                                        DataBuffer* output) override {
     return DROP;
   }
 };
 
+class TlsEncryptedHandshakeMessageReplacer : public TlsRecordFilter {
+ public:
+  TlsEncryptedHandshakeMessageReplacer(const std::shared_ptr<TlsAgent>& a,
+                                       uint8_t old_ct, uint8_t new_ct)
+      : TlsRecordFilter(a), old_ct_(old_ct), new_ct_(new_ct) {}
+
+ protected:
+  PacketFilter::Action FilterRecord(const TlsRecordHeader& header,
+                                    const DataBuffer& record, size_t* offset,
+                                    DataBuffer* output) override {
+    if (header.content_type() != ssl_ct_application_data) {
+      return KEEP;
+    }
+
+    uint16_t protection_epoch = 0;
+    uint8_t inner_content_type;
+    DataBuffer plaintext;
+    if (!Unprotect(header, record, &protection_epoch, &inner_content_type,
+                   &plaintext) ||
+        !plaintext.len()) {
+      return KEEP;
+    }
+
+    if (inner_content_type != ssl_ct_handshake) {
+      return KEEP;
+    }
+
+    size_t off = 0;
+    uint32_t msg_len = 0;
+    uint32_t msg_type = 255;  // Not a real message
+    do {
+      if (!plaintext.Read(off, 1, &msg_type) || msg_type == old_ct_) {
+        break;
+      }
+
+      // Increment and check next messages
+      if (!plaintext.Read(++off, 3, &msg_len)) {
+        break;
+      }
+      off += 3 + msg_len;
+    } while (msg_type != old_ct_);
+
+    if (msg_type == old_ct_) {
+      plaintext.Write(off, new_ct_, 1);
+    }
+
+    DataBuffer ciphertext;
+    bool ok = Protect(spec(protection_epoch), header, inner_content_type,
+                      plaintext, &ciphertext, 0);
+    if (!ok) {
+      return KEEP;
+    }
+    *offset = header.Write(output, *offset, ciphertext);
+    return CHANGE;
+  }
+
+ private:
+  uint8_t old_ct_;
+  uint8_t new_ct_;
+};
+
 class TlsExtensionInjector : public TlsHandshakeFilter {
  public:
   TlsExtensionInjector(const std::shared_ptr<TlsAgent>& a, uint16_t ext,
                        const DataBuffer& data)
       : TlsHandshakeFilter(a), extension_(ext), data_(data) {}
 
  protected:
   PacketFilter::Action FilterHandshake(const HandshakeHeader& header,
--- a/security/nss/lib/ckfw/object.c
+++ b/security/nss/lib/ckfw/object.c
@@ -488,28 +488,21 @@ nssCKFWObject_GetAttributeTypes(
 NSS_IMPLEMENT CK_ULONG
 nssCKFWObject_GetAttributeSize(
     NSSCKFWObject *fwObject,
     CK_ATTRIBUTE_TYPE attribute,
     CK_RV *pError)
 {
     CK_ULONG rv;
 
-#ifdef NSSDEBUG
     if (!pError) {
         return (CK_ULONG)0;
     }
 
-    *pError = nssCKFWObject_verifyPointer(fwObject);
-    if (CKR_OK != *pError) {
-        return (CK_ULONG)0;
-    }
-#endif /* NSSDEBUG */
-
-    if (!fwObject->mdObject->GetAttributeSize) {
+    if (!fwObject || !fwObject->mdObject || !fwObject->mdObject->GetAttributeSize) {
         *pError = CKR_GENERAL_ERROR;
         return (CK_ULONG)0;
     }
 
     *pError = nssCKFWMutex_Lock(fwObject->mutex);
     if (CKR_OK != *pError) {
         return (CK_ULONG)0;
     }
--- a/security/nss/lib/ssl/sslsnce.c
+++ b/security/nss/lib/ssl/sslsnce.c
@@ -1688,40 +1688,44 @@ ssl_SelfEncryptSetup(void)
 /* Configure a self encryption key pair.  |explicitConfig| is set to true for
  * calls to SSL_SetSessionTicketKeyPair(), false for implicit configuration.
  * This assumes that the setup has been run. */
 static SECStatus
 ssl_SetSelfEncryptKeyPair(SECKEYPublicKey *pubKey,
                           SECKEYPrivateKey *privKey,
                           PRBool explicitConfig)
 {
-    SECKEYPublicKey *pubKeyCopy;
-    SECKEYPrivateKey *privKeyCopy;
+    SECKEYPublicKey *pubKeyCopy, *oldPubKey;
+    SECKEYPrivateKey *privKeyCopy, *oldPrivKey;
 
     PORT_Assert(ssl_self_encrypt_key_pair.lock);
+    pubKeyCopy = SECKEY_CopyPublicKey(pubKey);
+    privKeyCopy = SECKEY_CopyPrivateKey(privKey);
 
-    pubKeyCopy = SECKEY_CopyPublicKey(pubKey);
-    if (!pubKeyCopy) {
-        PORT_SetError(SEC_ERROR_NO_MEMORY);
-        return SECFailure;
-    }
-
-    privKeyCopy = SECKEY_CopyPrivateKey(privKey);
-    if (!privKeyCopy) {
+    if (!pubKeyCopy || !privKeyCopy) {
         SECKEY_DestroyPublicKey(pubKeyCopy);
+        SECKEY_DestroyPrivateKey(privKeyCopy);
         PORT_SetError(SEC_ERROR_NO_MEMORY);
         return SECFailure;
     }
 
     PR_RWLock_Wlock(ssl_self_encrypt_key_pair.lock);
-    ssl_CleanupSelfEncryptKeyPair();
+    oldPubKey = ssl_self_encrypt_key_pair.pubKey;
+    oldPrivKey = ssl_self_encrypt_key_pair.privKey;
     ssl_self_encrypt_key_pair.pubKey = pubKeyCopy;
     ssl_self_encrypt_key_pair.privKey = privKeyCopy;
     ssl_self_encrypt_key_pair.configured = explicitConfig;
     PR_RWLock_Unlock(ssl_self_encrypt_key_pair.lock);
+
+    if (oldPubKey) {
+        PORT_Assert(oldPrivKey);
+        SECKEY_DestroyPublicKey(oldPubKey);
+        SECKEY_DestroyPrivateKey(oldPrivKey);
+    }
+
     return SECSuccess;
 }
 
 /* This is really the self-encryption keys but it has the
  * wrong name for historical API stability reasons. */
 SECStatus
 SSL_SetSessionTicketKeyPair(SECKEYPublicKey *pubKey,
                             SECKEYPrivateKey *privKey)
@@ -1770,26 +1774,43 @@ ssl_GetSelfEncryptKeyPair(SECKEYPublicKe
                           SECKEYPrivateKey **privKey)
 {
     if (PR_SUCCESS != PR_CallOnce(&ssl_self_encrypt_key_pair.setup,
                                   &ssl_SelfEncryptSetup)) {
         PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
         return SECFailure;
     }
 
+    SECKEYPublicKey *pubKeyCopy;
+    SECKEYPrivateKey *privKeyCopy;
+    PRBool noKey = PR_FALSE;
+
     PR_RWLock_Rlock(ssl_self_encrypt_key_pair.lock);
-    *pubKey = ssl_self_encrypt_key_pair.pubKey;
-    *privKey = ssl_self_encrypt_key_pair.privKey;
+    if (ssl_self_encrypt_key_pair.pubKey && ssl_self_encrypt_key_pair.privKey) {
+        pubKeyCopy = SECKEY_CopyPublicKey(ssl_self_encrypt_key_pair.pubKey);
+        privKeyCopy = SECKEY_CopyPrivateKey(ssl_self_encrypt_key_pair.privKey);
+    } else {
+        noKey = PR_TRUE;
+    }
     PR_RWLock_Unlock(ssl_self_encrypt_key_pair.lock);
-    if (!*pubKey) {
-        PORT_Assert(!*privKey);
+
+    if (noKey) {
         PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
         return SECFailure;
     }
-    PORT_Assert(*privKey);
+
+    if (!pubKeyCopy || !privKeyCopy) {
+        SECKEY_DestroyPublicKey(pubKeyCopy);
+        SECKEY_DestroyPrivateKey(privKeyCopy);
+        PORT_SetError(SEC_ERROR_NO_MEMORY);
+        return SECFailure;
+    }
+
+    *pubKey = pubKeyCopy;
+    *privKey = privKeyCopy;
     return SECSuccess;
 }
 
 static PRBool
 ssl_GenerateSelfEncryptKeys(void *pwArg, PRUint8 *keyName,
                             PK11SymKey **aesKey, PK11SymKey **macKey);
 
 static PRStatus
@@ -2048,45 +2069,53 @@ loser:
         PK11_FreeSymKey(macKeyTmp);
     return SECFailure;
 }
 
 static SECStatus
 ssl_GenerateSelfEncryptKeys(void *pwArg, PRUint8 *keyName,
                             PK11SymKey **encKey, PK11SymKey **macKey)
 {
-    SECKEYPrivateKey *svrPrivKey;
-    SECKEYPublicKey *svrPubKey;
+    SECKEYPrivateKey *svrPrivKey = NULL;
+    SECKEYPublicKey *svrPubKey = NULL;
     PRUint32 now;
-    SECStatus rv;
     cacheDesc *cache = &globalCache;
 
-    rv = ssl_GetSelfEncryptKeyPair(&svrPubKey, &svrPrivKey);
+    SECStatus rv = ssl_GetSelfEncryptKeyPair(&svrPubKey, &svrPrivKey);
     if (rv != SECSuccess || !cache->cacheMem) {
         /* No key pair for wrapping, or the cache is uninitialized. Generate
          * keys and return them without caching. */
-        return GenerateSelfEncryptKeys(pwArg, keyName, encKey, macKey);
-    }
-
-    now = LockSidCacheLock(cache->keyCacheLock, 0);
-    if (!now)
-        return SECFailure;
-
-    if (*(cache->ticketKeysValid)) {
-        rv = UnwrapCachedSelfEncryptKeys(svrPrivKey, keyName, encKey, macKey);
+        rv = GenerateSelfEncryptKeys(pwArg, keyName, encKey, macKey);
     } else {
-        /* Keys do not exist, create them. */
-        rv = GenerateAndWrapSelfEncryptKeys(svrPubKey, pwArg, keyName,
-                                            encKey, macKey);
-        if (rv == SECSuccess) {
-            *(cache->ticketKeysValid) = 1;
+        now = LockSidCacheLock(cache->keyCacheLock, 0);
+        if (!now) {
+            goto loser;
         }
+
+        if (*(cache->ticketKeysValid)) {
+            rv = UnwrapCachedSelfEncryptKeys(svrPrivKey, keyName, encKey, macKey);
+        } else {
+            /* Keys do not exist, create them. */
+            rv = GenerateAndWrapSelfEncryptKeys(svrPubKey, pwArg, keyName,
+                                                encKey, macKey);
+            if (rv == SECSuccess) {
+                *(cache->ticketKeysValid) = 1;
+            }
+        }
+        UnlockSidCacheLock(cache->keyCacheLock);
     }
+    SECKEY_DestroyPublicKey(svrPubKey);
+    SECKEY_DestroyPrivateKey(svrPrivKey);
+    return rv;
+
+loser:
     UnlockSidCacheLock(cache->keyCacheLock);
-    return rv;
+    SECKEY_DestroyPublicKey(svrPubKey);
+    SECKEY_DestroyPrivateKey(svrPrivKey);
+    return SECFailure;
 }
 
 /* The caller passes in the new value it wants
  * to set.  This code tests the wrapped sym key entry in the shared memory.
  * If it is uninitialized, this function writes the caller's value into
  * the disk entry, and returns false.
  * Otherwise, it overwrites the caller's wswk with the value obtained from
  * the disk, and returns PR_TRUE.
--- a/security/nss/lib/ssl/tls13con.c
+++ b/security/nss/lib/ssl/tls13con.c
@@ -793,17 +793,16 @@ tls13_HandleKeyUpdate(sslSocket *ss, PRU
     PRUint32 update;
 
     SSL_TRC(3, ("%d: TLS13[%d]: %s handle key update",
                 SSL_GETPID(), ss->fd, SSL_ROLE(ss)));
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
 
-    PORT_Assert(ss->firstHsDone);
     if (!tls13_IsPostHandshake(ss)) {
         FATAL_ERROR(ss, SSL_ERROR_RX_UNEXPECTED_KEY_UPDATE, unexpected_message);
         return SECFailure;
     }
 
     rv = TLS13_CHECK_HS_STATE(ss, SSL_ERROR_RX_UNEXPECTED_KEY_UPDATE,
                               idle_handshake);
     if (rv != SECSuccess) {