Bug 1556591 - Eliminate races in uses of PK11_SetWrapKey, r=kjacobs

These functions are terrible, but in the spirit of ABI compatibility, I
can't really change their signature.  And it isn't important enough to fix them,
so this introduces a quick hack.

This is wrapped up in the mess that is Bug 1556588, but I'm proceeding here
under the assumption that unconditional use of the lock is safe and that a
non-thread-safe module will explode in many other ways.

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

3 files changed, 48 insertions, 19 deletions

diff --git a/lib/pk11wrap/pk11pub.h b/lib/pk11wrap/pk11pub.h
index 9ca4018d9..7e73458ea 100644
--- a/lib/pk11wrap/pk11pub.h
+++ b/lib/pk11wrap/pk11pub.h
@@ -275,12 +275,9 @@ PK11SymKey *PK11_ImportSymKeyWithFlags(PK11SlotInfo *slot,
 PK11SymKey *PK11_SymKeyFromHandle(PK11SlotInfo *slot, PK11SymKey *parent,
                                   PK11Origin origin, CK_MECHANISM_TYPE type, CK_OBJECT_HANDLE keyID,
                                   PRBool owner, void *wincx);
+/* PK11_GetWrapKey and PK11_SetWrapKey are not thread safe. */
 PK11SymKey *PK11_GetWrapKey(PK11SlotInfo *slot, int wrap,
                             CK_MECHANISM_TYPE type, int series, void *wincx);
-/*
- * This function is not thread-safe.  It can only be called when only
- * one thread has a reference to wrapKey.
- */
 void PK11_SetWrapKey(PK11SlotInfo *slot, int wrap, PK11SymKey *wrapKey);
 CK_MECHANISM_TYPE PK11_GetMechanism(PK11SymKey *symKey);
 /*
diff --git a/lib/pk11wrap/pk11skey.c b/lib/pk11wrap/pk11skey.c
index a1c38223f..ff66fd8c0 100644
--- a/lib/pk11wrap/pk11skey.c
+++ b/lib/pk11wrap/pk11skey.c
@@ -357,7 +357,9 @@ PK11_SymKeyFromHandle(PK11SlotInfo *slot, PK11SymKey *parent, PK11Origin origin,
 }
 
 /*
- * turn key handle into an appropriate key object
+ * Restore a symmetric wrapping key that was saved using PK11_SetWrapKey.
+ *
+ * This function is provided for ABI compatibility; see PK11_SetWrapKey below.
  */
 PK11SymKey *
 PK11_GetWrapKey(PK11SlotInfo *slot, int wrap, CK_MECHANISM_TYPE type,
@@ -365,33 +367,51 @@ PK11_GetWrapKey(PK11SlotInfo *slot, int wrap, CK_MECHANISM_TYPE type,
 {
     PK11SymKey *symKey = NULL;
 
-    if (slot->series != series)
-        return NULL;
-    if (slot->refKeys[wrap] == CK_INVALID_HANDLE)
+    PK11_EnterSlotMonitor(slot);
+    if (slot->series != series ||
+        slot->refKeys[wrap] == CK_INVALID_HANDLE) {
+        PK11_ExitSlotMonitor(slot);
         return NULL;
-    if (type == CKM_INVALID_MECHANISM)
+    }
+
+    if (type == CKM_INVALID_MECHANISM) {
         type = slot->wrapMechanism;
+    }
 
     symKey = PK11_SymKeyFromHandle(slot, NULL, PK11_OriginDerive,
                                    slot->wrapMechanism, slot->refKeys[wrap], PR_FALSE, wincx);
+    PK11_ExitSlotMonitor(slot);
     return symKey;
 }
 
 /*
- * This function is not thread-safe because it sets wrapKey->sessionOwner
- * without using a lock or atomic routine.  It can only be called when
- * only one thread has a reference to wrapKey.
+ * This function sets an attribute on the current slot with a wrapping key.  The
+ * data saved is ephemeral; it needs to be run every time the program is
+ * invoked.
+ *
+ * Since NSS 3.45, this function is marginally more thread safe.  It uses the
+ * slot lock (if present) and fails silently if a value is already set.  Use
+ * PK11_GetWrapKey() after calling this function to get the current wrapping key
+ * in case there was an update on another thread.
+ *
+ * Either way, using this function is inadvisable.  It's provided for ABI
+ * compatibility only.
  */
 void
 PK11_SetWrapKey(PK11SlotInfo *slot, int wrap, PK11SymKey *wrapKey)
 {
-    /* save the handle and mechanism for the wrapping key */
-    /* mark the key and session as not owned by us to they don't get freed
-     * when the key goes way... that lets us reuse the key later */
-    slot->refKeys[wrap] = wrapKey->objectID;
-    wrapKey->owner = PR_FALSE;
-    wrapKey->sessionOwner = PR_FALSE;
-    slot->wrapMechanism = wrapKey->type;
+    PK11_EnterSlotMonitor(slot);
+    if (wrap < PR_ARRAY_SIZE(slot->refKeys) &&
+        slot->refKeys[wrap] == CK_INVALID_HANDLE) {
+        /* save the handle and mechanism for the wrapping key */
+        /* mark the key and session as not owned by us so they don't get freed
+         * when the key goes way... that lets us reuse the key later */
+        slot->refKeys[wrap] = wrapKey->objectID;
+        wrapKey->owner = PR_FALSE;
+        wrapKey->sessionOwner = PR_FALSE;
+        slot->wrapMechanism = wrapKey->type;
+    }
+    PK11_ExitSlotMonitor(slot);
 }
 
 /*
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index d9f4b5234..67f162955 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -11378,7 +11378,19 @@ ssl3_CacheWrappedSecret(sslSocket *ss, sslSessionID *sid,
             wrappingKey = PK11_KeyGen(symKeySlot, mechanism, NULL,
                                       keyLength, pwArg);
             if (wrappingKey) {
+                /* The thread safety characteristics of PK11_[SG]etWrapKey is
+                * abominable.  This protects against races in calling
+                * PK11_SetWrapKey by dropping and re-acquiring the canonical
+                * value once it is set.  The mutex in PK11_[SG]etWrapKey will
+                * ensure that races produce the same value in the end. */
                 PK11_SetWrapKey(symKeySlot, wrapKeyIndex, wrappingKey);
+                PK11_FreeSymKey(wrappingKey);
+                wrappingKey = PK11_GetWrapKey(symKeySlot, wrapKeyIndex,
+                                              CKM_INVALID_MECHANISM, incarnation, pwArg);
+                if (!wrappingKey) {
+                    PK11_FreeSlot(symKeySlot);
+                    return SECFailure;
+                }
             }
         }
     } else {