Skip to content

Prevent double free in concurrent C_DestroyObject on shared token object #205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LinuxJedi wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Prevent double free in concurrent C_DestroyObject on shared token object #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
518d2b9
Prevent double free in concurrent C_DestroyObject on shared token object
LinuxJedi Jul 10, 2026
6d3d241
Harden C_DestroyObject removal against races and surface store errors
LinuxJedi Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 36 additions & 12 deletions src/crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1279,7 +1279,8 @@ static CK_RV AddRSAPrivateKeyObject(WP11_Session* session,
if (rv != CKR_OK) {
if (*phKey != CK_INVALID_HANDLE) {
/* ignore return value, logged in function */
(void)WP11_Session_RemoveObject(session, privKeyObject);
(void)WP11_Session_RemoveObject(session, privKeyObject,
WP11_Object_OnToken(privKeyObject), 0);
*phKey = CK_INVALID_HANDLE;
}
if (privKeyObject != NULL) {
Expand Down Expand Up @@ -1476,7 +1477,8 @@ 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);
(void)WP11_Session_RemoveObject(session, object,
WP11_Object_OnToken(object), 0);
WP11_Object_Free(object);
}

Expand Down Expand Up @@ -1664,6 +1666,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;
Expand Down Expand Up @@ -1692,29 +1695,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_RemoveObject 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_RemoveObject, once
* the object is confirmed still linked (and therefore alive), so it cannot
* race a concurrent free of the same handle. */
ret = WP11_Session_RemoveObject(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;
}
Expand Down Expand Up @@ -7915,12 +7936,14 @@ 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);
(void)WP11_Session_RemoveObject(session, pub,
WP11_Object_OnToken(pub), 0);
WP11_Object_Free(pub);
}
if (rv != CKR_OK && priv != NULL) {
if (*phPrivateKey != CK_INVALID_HANDLE)
(void)WP11_Session_RemoveObject(session, priv);
(void)WP11_Session_RemoveObject(session, priv,
WP11_Object_OnToken(priv), 0);
WP11_Object_Free(priv);
}

Expand Down Expand Up @@ -8418,7 +8441,8 @@ 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);
(void)WP11_Session_RemoveObject(session, keyObj,
WP11_Object_OnToken(keyObj), 0);
*phKey = CK_INVALID_HANDLE;
}
if (keyObj != NULL) {
Expand Down
146 changes: 108 additions & 38 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
(void)WP11_Session_RemoveObject(session, obj, obj->onToken, 0);
WP11_Object_Free(obj);
}
session->inUse = 0;
Expand Down Expand Up @@ -8822,44 +8822,93 @@ int WP11_Session_AddObject(WP11_Session* session, int onToken,
* @return -ve on failure.
* 0 on success.
*/
int WP11_Session_RemoveObject(WP11_Session* session, WP11_Object* object)
int WP11_Session_RemoveObject(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
Expand All @@ -8868,16 +8917,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) {
Expand All @@ -8893,12 +8940,19 @@ 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;
}
Expand Down Expand Up @@ -10197,6 +10251,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.
*
Expand Down
Loading
Loading