Bug 1593401 - Fix race condition in self-encrypt functions r=mt,jcjNSS_3_48_BETA1

This patch fixes a race/UAF around `ssl_GetSelfEncryptKeyPair`. Previously, this function would return a pointer to global memory, which could be freed by another thread while in use.

The initially-proposed solution was to limit implicit configuration to occur only once. While this solves the problem within the scope of implicit configuration, explicit configuration could cause the same
UAF (though it would probably be much less frequent, as there are no such crash reports). For a more complete fix, `ssl_GetSelfEncryptKeyPair` can make a copy of the global key within its critical section.

We can still restrict implicit configuration to occur only once, but it's not necessary to fix the UAF.

//Side note: //It seems there's an unrelated deadlock condition in `ssl_GenerateSelfEncryptKeys`, in the case where `LockSidCacheLock` returns null. This is also fixed.

Differential Revision: https://phabricator.services.mozilla.com/D55060

1 files changed, 63 insertions, 34 deletions

diff --git a/lib/ssl/sslsnce.c b/lib/ssl/sslsnce.c
index 75402a780..36c82117e 100644
--- a/lib/ssl/sslsnce.c
+++ b/lib/ssl/sslsnce.c
@@ -1693,30 +1693,34 @@ 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);
-    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;
 }
 
@@ -1775,16 +1779,33 @@ ssl_GetSelfEncryptKeyPair(SECKEYPublicKey **pubKey,
         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;
 }
 
@@ -2053,35 +2074,43 @@ 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);
     }
-    UnlockSidCacheLock(cache->keyCacheLock);
+    SECKEY_DestroyPublicKey(svrPubKey);
+    SECKEY_DestroyPrivateKey(svrPrivKey);
     return rv;
+
+loser:
+    UnlockSidCacheLock(cache->keyCacheLock);
+    SECKEY_DestroyPublicKey(svrPubKey);
+    SECKEY_DestroyPrivateKey(svrPrivKey);
+    return SECFailure;
 }
 
 /* The caller passes in the new value it wants