Bug 1593401 - Fix race condition in self-encrypt functions r=mt,jcj NSS_3_48_BETA1
authorKevin Jacobs <kjacobs@mozilla.com>
Mon, 02 Dec 2019 22:09:32 +0000
changeset 15415 77976f3fefca36adea9d1087915078950283c6d4
parent 15414 b6141fb8679947fdd8ac1b43eca1f1fe829608fe
child 15416 06d5b4f91a9c61a962ef6070b33044f796ff61ed
push id3600
push userjjones@mozilla.com
push dateMon, 02 Dec 2019 22:26:45 +0000
reviewersmt, jcj
bugs1593401
Bug 1593401 - Fix race condition in self-encrypt functions r=mt,jcj 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
lib/ssl/sslsnce.c
--- a/lib/ssl/sslsnce.c
+++ b/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.