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
--- 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) {