Merge pull request #205 from LinuxJedi/concurrent-destroy-object-double-free

Prevent double free in concurrent C_DestroyObject on shared token object

Author: philljj

src/crypto.c

@@ -1278,8 +1278,7 @@ static CK_RV AddRSAPrivateKeyObject(WP11_Session* session,
 err_out:
     if (rv != CKR_OK) {
         if (*phKey != CK_INVALID_HANDLE) {
-            /* ignore return value, logged in function */
-            (void)WP11_Session_RemoveObject(session, privKeyObject);
+            WP11_Session_RemoveObject(session, privKeyObject);
             *phKey = CK_INVALID_HANDLE;
         }
         if (privKeyObject != NULL) {
@@ -1475,8 +1474,7 @@ CK_RV C_CreateObject(CK_SESSION_HANDLE hSession, CK_ATTRIBUTE_PTR pTemplate,
     }
     rv = AddObject(session, object, pTemplate, ulCount, phObject);
     if (rv != CKR_OK) {
-        /* ignore return value, logged in function */
-        (void)WP11_Session_RemoveObject(session, object);
+        WP11_Session_RemoveObject(session, object);
         WP11_Object_Free(object);
     }
 
@@ -1664,6 +1662,7 @@ CK_RV C_DestroyObject(CK_SESSION_HANDLE hSession,
                       CK_OBJECT_HANDLE hObject)
 {
     int ret;
+    int onToken;
     CK_RV rv;
     WP11_Session* session;
     WP11_Object* obj = NULL;
@@ -1692,29 +1691,47 @@ CK_RV C_DestroyObject(CK_SESSION_HANDLE hSession,
         WOLFPKCS11_LEAVE("C_DestroyObject", rv);
         return rv;
     }
+    /* Derive onToken from the handle, not the object: it stays valid even if a
+     * concurrent destroy of the same handle frees the object out from under
+     * us, and it lets WP11_Session_RemoveObjectByHandle unlink token objects
+     * using pointer identity alone. */
+    onToken = WP11_Object_HandleOnToken(hObject);
 
     /* Only require R/W session for token objects */
-    if (!WP11_Session_IsRW(session) && WP11_Object_OnToken(obj)) {
+    if (!WP11_Session_IsRW(session) && onToken) {
         rv = CKR_SESSION_READ_ONLY;
         WOLFPKCS11_LEAVE("C_DestroyObject", rv);
         return rv;
     }
 
-    /* Reject destruction of objects whose CKA_DESTROYABLE has been set to
-     * CK_FALSE. Read the flag bit directly so the gate stays consistent
-     * regardless of which getter view applies. */
-    if (!WP11_Object_IsDestroyable(obj)) {
+    /* Remove and reject-if-not-destroyable in one locked step. The
+     * CKA_DESTROYABLE check is performed inside WP11_Session_RemoveObjectByHandle,
+     * once the object is confirmed still linked (and therefore alive), so it
+     * cannot race a concurrent free of the same handle. */
+    ret = WP11_Session_RemoveObjectByHandle(session, obj, onToken,
+                                            1 /* checkDestroyable */);
+    if (ret == WP11_OBJECT_ALREADY_REMOVED) {
+        /* Another thread destroyed this object first (a concurrent
+         * C_DestroyObject on the same shared token-object handle). That thread
+         * owns the free; do not touch the object again. */
+        rv = CKR_OBJECT_HANDLE_INVALID;
+        WOLFPKCS11_LEAVE("C_DestroyObject", rv);
+        return rv;
+    }
+    if (ret == WP11_OBJECT_NOT_DESTROYABLE) {
+        /* CKA_DESTROYABLE is CK_FALSE; object left in place. */
         rv = CKR_ACTION_PROHIBITED;
         WOLFPKCS11_LEAVE("C_DestroyObject", rv);
         return rv;
     }
-
-    rv = WP11_Session_RemoveObject(session, obj);
     /* Drop any active-operation reference to this object before freeing it so a
      * pending operation cannot use freed memory. */
     WP11_Slot_ClearActiveObject(WP11_Session_GetSlot(session), obj);
     WP11_Object_Free(obj);
 
+    /* The object was unlinked; a negative status means persisting the token
+     * afterwards failed. Surface it rather than reporting success. */
+    rv = (ret < 0) ? CKR_FUNCTION_FAILED : CKR_OK;
     WOLFPKCS11_LEAVE("C_DestroyObject", rv);
     return rv;
 }
@@ -7915,12 +7932,12 @@ CK_RV C_GenerateKeyPair(CK_SESSION_HANDLE hSession,
 
     if (rv != CKR_OK && pub != NULL) {
         if (*phPublicKey != CK_INVALID_HANDLE)
-            (void)WP11_Session_RemoveObject(session, pub);
+            WP11_Session_RemoveObject(session, pub);
         WP11_Object_Free(pub);
     }
     if (rv != CKR_OK && priv != NULL) {
         if (*phPrivateKey != CK_INVALID_HANDLE)
-            (void)WP11_Session_RemoveObject(session, priv);
+            WP11_Session_RemoveObject(session, priv);
         WP11_Object_Free(priv);
     }
 
@@ -8417,8 +8434,7 @@ CK_RV C_UnwrapKey(CK_SESSION_HANDLE hSession,
             }
             if (rv != CKR_OK) {
                 if (*phKey != CK_INVALID_HANDLE) {
-                    /* ignore return value, logged in function */
-                    (void)WP11_Session_RemoveObject(session, keyObj);
+                    WP11_Session_RemoveObject(session, keyObj);
                     *phKey = CK_INVALID_HANDLE;
                 }
                 if (keyObj != NULL) {

src/internal.c

@@ -922,7 +922,7 @@ static void wp11_Session_Final(WP11_Session* session)
         /* Free objects in session. */
         while ((obj = session->object) != NULL) {
             /* ignore return value, logged in function */
-            (void)WP11_Session_RemoveObject(session, obj);
+            WP11_Session_RemoveObject(session, obj);
             WP11_Object_Free(obj);
         }
         session->inUse = 0;
@@ -8815,51 +8815,122 @@ int WP11_Session_AddObject(WP11_Session* session, int onToken,
 }
 
 /**
- * Remove object to the session or token.
+ * Remove an object from its session or token list, addressing it by a
+ * caller-supplied onToken flag rather than by dereferencing the object.
  *
- * @param  session  [in]  Session object.
- * @param  object   [in]  Key Object object.
- * @return  -ve on failure.
- *          0 on success.
+ * Membership is established by pointer identity alone, so *object is never
+ * dereferenced until it is confirmed still linked (and therefore still alive).
+ * onToken must be derived from the handle (see WP11_Object_HandleOnToken), not
+ * from *object, so it stays valid even after a concurrent caller frees the
+ * object. This is the entry point for C_DestroyObject, where a second thread may
+ * be destroying the same shared token-object handle at the same time.
+ *
+ * Callers that hold the only reference to a live object should use the simpler
+ * WP11_Session_RemoveObject() wrapper instead: it derives onToken from the
+ * object itself and so cannot mismatch onToken against the wrong object.
+ *
+ * @param  session          [in]  Session performing the removal.
+ * @param  object           [in]  Object to remove, matched by pointer identity.
+ * @param  onToken          [in]  Non-zero if the handle names a token object.
+ *                                Derive from the handle, never from *object.
+ * @param  checkDestroyable [in]  Non-zero to honor CKA_DESTROYABLE and refuse to
+ *                                remove an object marked not destroyable.
+ * @return  WP11_OBJECT_ALREADY_REMOVED if the object was no longer linked, i.e.
+ *          a concurrent caller already removed it; the caller must not free it.
+ *          WP11_OBJECT_NOT_DESTROYABLE if checkDestroyable is set and the object
+ *          has CKA_DESTROYABLE = CK_FALSE; the object is left in place and must
+ *          not be freed.
+ *          -ve on a store failure after the object was unlinked.
+ *          0 on success; the caller now owns the object and must free it.
  */
-int WP11_Session_RemoveObject(WP11_Session* session, WP11_Object* object)
+int WP11_Session_RemoveObjectByHandle(WP11_Session* session,
+                                      WP11_Object* object, int onToken,
+                                      int checkDestroyable)
 {
     int ret = 0;
+    int found = 0;
     WP11_Object** curr;
-    WP11_Token* token;
-    int id;
+    WP11_Token* token = &session->slot->token;
+    WP11_Session* owner = session;
+    int id = 0;
+
+    /* The token lock guards every object list on the slot (session lists
+     * included). Take it before touching any list or the object. Token objects
+     * are shared across sessions, so two C_DestroyObject calls on the same
+     * handle can both pass the lock-free WP11_Object_Find and reach here with
+     * the same pointer; the loser must detect that the winner already unlinked
+     * (and is about to free) the object and bail out without a second free.
+     *
+     * Membership is established by pointer identity alone so that *object is
+     * never dereferenced until it is confirmed still linked (and therefore
+     * still alive). onToken comes from the handle, not from *object, so it is
+     * safe to trust even after the object has been freed. */
+    WP11_Lock_LockRW(&token->lock);
 
+    if (onToken) {
+        /* Token objects live on the shared token list. */
+        for (curr = &token->object; *curr != NULL; curr = &(*curr)->next) {
+            if (*curr == object) {
+                found = 1;
+                break;
+            }
+        }
+    }
+    else {
+        /* Session objects live on their owning session's list. Check the
+         * caller's own list first: it is the only possibility in the standard
+         * build and the common case under NSS, and needs no dereference of
+         * *object. */
+        for (curr = &owner->object; *curr != NULL; curr = &(*curr)->next) {
+            if (*curr == object) {
+                found = 1;
+                break;
+            }
+        }
 #ifdef WOLFPKCS11_NSS
-    if (object->session && (session != object->session))
-        session = object->session;
+        /* NSS makes objects visible across sessions, so the object may belong
+         * to a different session. Fall back to its recorded owner. This is
+         * reached only for a genuinely cross-session object - a freed object is
+         * normally caught as not-found above - and carries the same
+         * out-of-contract residual as a single-session concurrent misuse. */
+        if (!found && object->session != NULL && object->session != session) {
+            owner = object->session;
+            for (curr = &owner->object; *curr != NULL; curr = &(*curr)->next) {
+                if (*curr == object) {
+                    found = 1;
+                    break;
+                }
+            }
+        }
 #endif
+    }
 
-    /* Find the object in list and relink. */
-    if (object->onToken) {
-        WP11_Lock_LockRW(object->lock);
-        token = &session->slot->token;
+    if (!found) {
+        WP11_Lock_UnlockRW(&token->lock);
+        return WP11_OBJECT_ALREADY_REMOVED;
+    }
+
+    /* The object is confirmed linked, and therefore alive, so its fields may
+     * now be read safely under the lock. Enforce CKA_DESTROYABLE here rather
+     * than in the caller so the check cannot race a concurrent free. */
+    if (checkDestroyable && (object->opFlag & WP11_FLAG_NOT_DESTROYABLE)) {
+        WP11_Lock_UnlockRW(&token->lock);
+        return WP11_OBJECT_NOT_DESTROYABLE;
+    }
+
+    if (onToken) {
         token->objCnt--;
         /* Id of first object on token. */
         id = token->objCnt;
         curr = &token->object;
-    }
-    else {
-        session->objCnt--;
-        /* Id of first object in session. */
-        id = session->objCnt;
-        curr = &session->object;
-        WP11_Lock_LockRW(&session->slot->token.lock);
-    }
-
-    /* walk list to get id for object to remove */
-    while (*curr != NULL) {
-        if (*curr == object) {
-            *curr = object->next;
-            break;
-        }
+        /* walk list to get id for object to remove */
+        while (*curr != NULL) {
+            if (*curr == object) {
+                *curr = object->next;
+                break;
+            }
 
-    #ifndef WOLFPKCS11_NO_STORE
-        if (object->onToken) {
+        #ifndef WOLFPKCS11_NO_STORE
             /* remove any id's with higher value */
             ret = wp11_Object_Unstore(*curr, (int)session->slotId, id);
         #ifdef DEBUG_WOLFPKCS11
@@ -8868,16 +8939,14 @@ int WP11_Session_RemoveObject(WP11_Session* session, WP11_Object* object)
                     (int)session->slotId, id, ret);
             }
         #endif
-        }
-    #endif
+        #endif
 
-        curr = &(*curr)->next;
-        /* Id of next object as it isn't the one being removed. */
-        id--;
-    }
+            curr = &(*curr)->next;
+            /* Id of next object as it isn't the one being removed. */
+            id--;
+        }
 
-    if (object->onToken) {
-#ifndef WOLFPKCS11_NO_STORE
+    #ifndef WOLFPKCS11_NO_STORE
         ret = wp11_Object_Unstore(object, (int)session->slotId, id);
     #ifdef DEBUG_WOLFPKCS11
         if (ret != 0) {
@@ -8893,16 +8962,42 @@ int WP11_Session_RemoveObject(WP11_Session* session, WP11_Object* object)
                 (int)session->slotId, ret);
         }
     #endif
-#endif
-        WP11_Lock_UnlockRW(object->lock);
+    #endif
     }
     else {
-        WP11_Lock_UnlockRW(&session->slot->token.lock);
+        owner->objCnt--;
+        for (curr = &owner->object; *curr != NULL; curr = &(*curr)->next) {
+            if (*curr == object) {
+                *curr = object->next;
+                break;
+            }
+        }
     }
+
+    WP11_Lock_UnlockRW(&token->lock);
     (void)id; /* set but not used with WOLFPKCS11_NO_STORE */
     return ret;
 }
 
+/**
+ * Remove a live, exclusively-owned object from its session or token list.
+ *
+ * Thin wrapper over WP11_Session_RemoveObjectByHandle() for cleanup paths that
+ * hold the only reference to the object (object-creation rollback, keygen
+ * failure). onToken is read from the object itself, which is safe because no
+ * other thread can be removing it, and supplying onToken this way removes any
+ * risk of passing the wrong object's onToken. CKA_DESTROYABLE is not enforced
+ * (internal cleanup is not a user destroy) and any store error is logged in the
+ * callee, so the return value is intentionally discarded.
+ *
+ * @param  session  [in]  Session performing the removal.
+ * @param  object   [in]  Object to remove; must be live and owned by the caller.
+ */
+void WP11_Session_RemoveObject(WP11_Session* session, WP11_Object* object)
+{
+    (void)WP11_Session_RemoveObjectByHandle(session, object, object->onToken, 0);
+}
+
 /**
  * Drop any session reference to an object that is about to be freed.
  *
@@ -10197,6 +10292,22 @@ int WP11_Object_SetClass(WP11_Object* object, CK_OBJECT_CLASS objClass)
     return 0;
 }
 
+/**
+ * Determine whether an object handle refers to a token object.
+ *
+ * The onToken bit is encoded in the handle value itself, so this is safe to
+ * call even when the WP11_Object has already been freed by another thread -
+ * useful when deciding which list a racing C_DestroyObject must consult.
+ *
+ * @param  handle  [in]  Object handle.
+ * @return  1 when the handle refers to a token object.
+ *          0 when the handle refers to a session object.
+ */
+int WP11_Object_HandleOnToken(CK_OBJECT_HANDLE handle)
+{
+    return OBJ_HANDLE_ON_TOKEN(handle);
+}
+
 /**
  * Find an object based on the handle.
  *
@@ -10221,13 +10332,13 @@ int WP11_Object_Find(WP11_Session* session, CK_OBJECT_HANDLE objHandle,
         /* The NSS cross-session walk below needs the slot lock so a concurrent
          * remove-session can't reclaim a session node. Acquire it before the
          * token lock to match the global lock order (slot then token) used by
-         * WP11_Session_RemoveObject / WP11_Slot_CloseSessions; acquiring them
-         * in the opposite order would risk an AB-BA deadlock. */
+         * WP11_Session_RemoveObjectByHandle / WP11_Slot_CloseSessions; acquiring
+         * them in the opposite order would risk an AB-BA deadlock. */
         WP11_Lock_LockRO(&session->slot->lock);
 #endif
         /* Hold the token lock across the session->object walk so a concurrent
-         * WP11_Session_RemoveObject (which holds the same lock around the
-         * unlink) can't free a node we're stepping through. */
+         * WP11_Session_RemoveObjectByHandle (which holds the same lock around
+         * the unlink) can't free a node we're stepping through. */
         WP11_Lock_LockRO(&session->slot->token.lock);
         obj = session->object;
         while (obj != NULL) {