From ff53bac71ec32f1d6113cba855cb220cae89c989 Mon Sep 17 00:00:00 2001 From: Jakub Zelenka Date: Sun, 17 May 2026 19:03:19 +0200 Subject: [PATCH] Fix leak and use-after-free of session objects on session close SessionObjects were kept alive in allObjects as tombstones with valid=false after their session closed, so every object ever created lingered until C_Finalize, holding its Mutex*. Long-running clients saw unbounded memory growth dominated by mutex objects. Add refcounting to OSObject and have SessionObjectStore, HandleManager, and C_FindObjectsInit acquire/release across the boundaries where session-object pointers escape the store mutex. Releases happen outside the relevant lock to avoid widening critical sections. Also erase from allObjects in the store cleanup paths to avoid a double-free against the destructor. --- src/lib/SoftHSM.cpp | 18 +- src/lib/handle_mgr/CMakeLists.txt | 1 + src/lib/handle_mgr/Handle.cpp | 9 +- src/lib/handle_mgr/Handle.h | 5 +- src/lib/handle_mgr/HandleManager.cpp | 255 ++++++++++-------- src/lib/handle_mgr/HandleManager.h | 24 +- src/lib/handle_mgr/Makefile.am | 1 + .../handle_mgr/test/HandleManagerTests.cpp | 104 ++++--- src/lib/object_store/OSObject.h | 23 +- src/lib/object_store/SessionObjectStore.cpp | 166 ++++++------ .../test/SessionObjectStoreTests.cpp | 140 +++++----- 11 files changed, 434 insertions(+), 312 deletions(-) diff --git a/src/lib/SoftHSM.cpp b/src/lib/SoftHSM.cpp index 4d7d1acad..85c36bf90 100644 --- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ -2048,9 +2048,23 @@ CK_RV SoftHSM::C_FindObjectsInit(CK_SESSION_HANDLE hSession, CK_ATTRIBUTE_PTR pT // Check if we are out of memory if (findOp == NULL_PTR) return CKR_HOST_MEMORY; + std::set tokenObjects; + std::set sessionObjects; + token->getObjects(tokenObjects); + sessionObjectStore->getObjects(slot->getSlotID(), sessionObjects); + + struct SessionObjReleaser { + std::set& s; + ~SessionObjReleaser() + { + for (auto* o : s) + o->release(); + } + } sessionObjectGuard{sessionObjects}; + std::set allObjects; - token->getObjects(allObjects); - sessionObjectStore->getObjects(slot->getSlotID(),allObjects); + allObjects.insert(tokenObjects.begin(), tokenObjects.end()); + allObjects.insert(sessionObjects.begin(), sessionObjects.end()); std::set handles; std::set::iterator it; diff --git a/src/lib/handle_mgr/CMakeLists.txt b/src/lib/handle_mgr/CMakeLists.txt index 71269519b..770b0fd08 100644 --- a/src/lib/handle_mgr/CMakeLists.txt +++ b/src/lib/handle_mgr/CMakeLists.txt @@ -6,6 +6,7 @@ set(INCLUDE_DIRS ${PROJECT_SOURCE_DIR} ${PROJECT_SOURCE_DIR}/../data_mgr ${PROJECT_SOURCE_DIR}/../object_store ${PROJECT_SOURCE_DIR}/../pkcs11 + ${PROJECT_SOURCE_DIR}/../session_mgr ${PROJECT_SOURCE_DIR}/../slot_mgr ) diff --git a/src/lib/handle_mgr/Handle.cpp b/src/lib/handle_mgr/Handle.cpp index 127b4c744..79a77e222 100644 --- a/src/lib/handle_mgr/Handle.cpp +++ b/src/lib/handle_mgr/Handle.cpp @@ -34,17 +34,20 @@ // Constructor Handle::Handle(CK_HANDLE_KIND _kind, CK_SLOT_ID _slotID, CK_SESSION_HANDLE _hSession) - : kind(_kind), slotID(_slotID), hSession(_hSession), object(NULL_PTR), isPrivate(false) + : kind(_kind), slotID(_slotID), hSession(_hSession), object(nullptr), session(nullptr), + isPrivate(false) { } Handle::Handle(CK_HANDLE_KIND _kind, CK_SLOT_ID _slotID) - : kind(_kind), slotID(_slotID), hSession(CK_INVALID_HANDLE), object(NULL_PTR), isPrivate(false) + : kind(_kind), slotID(_slotID), hSession(CK_INVALID_HANDLE), object(nullptr), session(nullptr), + isPrivate(false) { } Handle::Handle() - : kind(CKH_INVALID), slotID(0), hSession(CK_INVALID_HANDLE), object(NULL_PTR), isPrivate(false) + : kind(CKH_INVALID), slotID(0), hSession(CK_INVALID_HANDLE), object(nullptr), session(nullptr), + isPrivate(false) { } diff --git a/src/lib/handle_mgr/Handle.h b/src/lib/handle_mgr/Handle.h index 94b42f9df..a8d5c2746 100644 --- a/src/lib/handle_mgr/Handle.h +++ b/src/lib/handle_mgr/Handle.h @@ -34,6 +34,8 @@ #define _SOFTHSM_V2_HANDLE_H #include "cryptoki.h" +#include "OSObject.h" +#include "Session.h" enum { CKH_INVALID, @@ -54,7 +56,8 @@ class Handle CK_SLOT_ID slotID; CK_SESSION_HANDLE hSession; - CK_VOID_PTR object; + OSObject *object; + Session *session; bool isPrivate; }; diff --git a/src/lib/handle_mgr/HandleManager.cpp b/src/lib/handle_mgr/HandleManager.cpp index d887c1545..f920735f5 100644 --- a/src/lib/handle_mgr/HandleManager.cpp +++ b/src/lib/handle_mgr/HandleManager.cpp @@ -56,40 +56,46 @@ HandleManager::HandleManager() // Destructor HandleManager::~HandleManager() { + // Safety net to release any session-object handles that weren't explicitly cleaned up. + // In normal C_Finalize flow this should be empty. + for (auto& kv : handles) { + if (kv.second.kind == CKH_OBJECT && kv.second.hSession != CK_INVALID_HANDLE) + kv.second.object->release(); + } MutexFactory::i()->recycleMutex(handlesMutex); } -CK_SESSION_HANDLE HandleManager::addSession(CK_SLOT_ID slotID, CK_VOID_PTR session) +CK_SESSION_HANDLE HandleManager::addSession(CK_SLOT_ID slotID, Session *session) { MutexLocker lock(handlesMutex); Handle h( CKH_SESSION, slotID ); - h.object = session; + h.session = session; handles[++handleCounter] = h; slotHandles[slotID].insert(handleCounter); slotSessionCount[slotID]++; return (CK_SESSION_HANDLE)handleCounter; } -CK_VOID_PTR HandleManager::getSession(const CK_SESSION_HANDLE hSession) +Session *HandleManager::getSession(const CK_SESSION_HANDLE hSession) { MutexLocker lock(handlesMutex); std::map< CK_ULONG, Handle>::iterator it = handles.find(hSession); if (it == handles.end() || CKH_SESSION != it->second.kind) - return NULL_PTR; - return it->second.object; + return nullptr; + return it->second.session; } -CK_OBJECT_HANDLE HandleManager::addSessionObject(CK_SLOT_ID slotID, CK_SESSION_HANDLE hSession, bool isPrivate, CK_VOID_PTR object) +CK_OBJECT_HANDLE HandleManager::addSessionObject(CK_SLOT_ID slotID, CK_SESSION_HANDLE hSession, bool isPrivate, OSObject *object) { MutexLocker lock(handlesMutex); // Return existing handle when the object has already been registered. - std::map< CK_VOID_PTR, CK_ULONG>::iterator oit = objects.find(object); + auto oit = objects.find(object); if (oit != objects.end()) { - std::map< CK_ULONG, Handle>::iterator hit = handles.find(oit->second); + auto hit = handles.find(oit->second); if (hit == handles.end() || CKH_OBJECT != hit->second.kind || slotID != hit->second.slotID) { objects.erase(oit); return CK_INVALID_HANDLE; @@ -103,22 +109,25 @@ CK_OBJECT_HANDLE HandleManager::addSessionObject(CK_SLOT_ID slotID, CK_SESSION_H handles[++handleCounter] = h; objects[object] = handleCounter; + sessionObjectHandles[hSession].insert(handleCounter); + slotHandles[slotID].insert(handleCounter); + + object->acquire(); + DEBUG_MSG("Added session object - handle: %lu, slot ID: %lu, session ID: %lu, private: %d, obj: %p", handleCounter, slotID, hSession, isPrivate, object); - sessionObjectHandles[hSession].insert(handleCounter); - slotHandles[slotID].insert(handleCounter); return (CK_OBJECT_HANDLE)handleCounter; } -CK_OBJECT_HANDLE HandleManager::addTokenObject(CK_SLOT_ID slotID, bool isPrivate, CK_VOID_PTR object) +CK_OBJECT_HANDLE HandleManager::addTokenObject(CK_SLOT_ID slotID, bool isPrivate, OSObject *object) { MutexLocker lock(handlesMutex); // Return existing handle when the object has already been registered. - std::map< CK_VOID_PTR, CK_ULONG>::iterator oit = objects.find(object); + auto oit = objects.find(object); if (oit != objects.end()) { - std::map< CK_ULONG, Handle>::iterator hit = handles.find(oit->second); + auto hit = handles.find(oit->second); if (hit == handles.end() || CKH_OBJECT != hit->second.kind || slotID != hit->second.slotID) { objects.erase(oit); return CK_INVALID_HANDLE; @@ -133,28 +142,29 @@ CK_OBJECT_HANDLE HandleManager::addTokenObject(CK_SLOT_ID slotID, bool isPrivate handles[++handleCounter] = h; objects[object] = handleCounter; + slotHandles[slotID].insert(handleCounter); + DEBUG_MSG("Added token object - handle: %lu, slot ID: %lu, private: %d, obj: %p", handleCounter, slotID, isPrivate, object); - slotHandles[slotID].insert(handleCounter); return (CK_OBJECT_HANDLE)handleCounter; } -CK_VOID_PTR HandleManager::getObject(const CK_OBJECT_HANDLE hObject) +OSObject *HandleManager::getObject(const CK_OBJECT_HANDLE hObject) { MutexLocker lock(handlesMutex); - std::map< CK_ULONG, Handle>::iterator it = handles.find(hObject); + auto it = handles.find(hObject); if (it == handles.end() || CKH_OBJECT != it->second.kind ) - return NULL_PTR; + return nullptr; return it->second.object; } -CK_OBJECT_HANDLE HandleManager::getObjectHandle(CK_VOID_PTR object) +CK_OBJECT_HANDLE HandleManager::getObjectHandle(OSObject *object) { MutexLocker lock(handlesMutex); - std::map< CK_VOID_PTR, CK_ULONG>::iterator it = objects.find(object); + auto it = objects.find(object); if (it == objects.end()) return CK_INVALID_HANDLE; return it->second; @@ -162,125 +172,152 @@ CK_OBJECT_HANDLE HandleManager::getObjectHandle(CK_VOID_PTR object) void HandleManager::destroyObject(const CK_OBJECT_HANDLE hObject) { - MutexLocker lock(handlesMutex); - - std::map< CK_ULONG, Handle>::iterator it = handles.find(hObject); - if (it != handles.end() && CKH_OBJECT == it->second.kind) { - // Remove from secondary indexes - if (it->second.hSession != CK_INVALID_HANDLE) - sessionObjectHandles[it->second.hSession].erase(hObject); - slotHandles[it->second.slotID].erase(hObject); + OSObject* toRelease = NULL; + { + MutexLocker lock(handlesMutex); + + auto it = handles.find(hObject); + if (it != handles.end() && CKH_OBJECT == it->second.kind) { + // Remove from secondary indexes + if (it->second.hSession != CK_INVALID_HANDLE) { + sessionObjectHandles[it->second.hSession].erase(hObject); + toRelease = it->second.object; + } + slotHandles[it->second.slotID].erase(hObject); - objects.erase(it->second.object); - handles.erase(it); - DEBUG_MSG("Destroyed object handle: %lu", hObject); - } else { - DEBUG_MSG("Cannot destroy handle %lu that is not found or not an object handle", hObject); + objects.erase(it->second.object); + handles.erase(it); + DEBUG_MSG("Destroyed object handle: %lu", hObject); + } else { + DEBUG_MSG("Cannot destroy handle %lu that is not found or not an object handle", hObject); + } } + if (toRelease != NULL) + toRelease->release(); } void HandleManager::sessionClosed(const CK_SESSION_HANDLE hSession) { - MutexLocker lock(handlesMutex); + std::set toRelease; - std::map< CK_ULONG, Handle>::iterator it = handles.find(hSession); - if (it == handles.end() || CKH_SESSION != it->second.kind) { - DEBUG_MSG("Session %lu not found and cannot be closed", hSession); - return; // Unable to find the specified session. - } - CK_SLOT_ID slotID = it->second.slotID; - - // session closed, so we can erase information about it. - slotHandles[slotID].erase(hSession); - handles.erase(it); - - DEBUG_MSG("Session %lu (slot %lu) closed", hSession, slotID); - - // Erase all session object handles associated with the given session handle - // using the secondary index instead of scanning the entire handles map. - std::map< CK_SESSION_HANDLE, std::set >::iterator soit = sessionObjectHandles.find(hSession); - if (soit != sessionObjectHandles.end()) { - std::set& objHandles = soit->second; - for (std::set::iterator oit = objHandles.begin(); oit != objHandles.end(); ++oit) { - std::map< CK_ULONG, Handle>::iterator hit = handles.find(*oit); - if (hit != handles.end()) { - objects.erase(hit->second.object); - DEBUG_MSG("Erasing object %lu for closed session %lu", hit->first, hSession); - slotHandles[slotID].erase(*oit); - handles.erase(hit); + MutexLocker lock(handlesMutex); + + std::map< CK_ULONG, Handle>::iterator it = handles.find(hSession); + if (it == handles.end() || CKH_SESSION != it->second.kind) + { + DEBUG_MSG("Session %lu not found and cannot be closed", hSession); + return; // Unable to find the specified session. + } + CK_SLOT_ID slotID = it->second.slotID; + + // session closed, so we can erase information about it. + slotHandles[slotID].erase(hSession); + handles.erase(it); + + DEBUG_MSG("Session %lu (slot %lu) closed", hSession, slotID); + + // Erase all session object handles associated with the given session handle + // using the secondary index instead of scanning the entire handles map. + auto soit = sessionObjectHandles.find(hSession); + if (soit != sessionObjectHandles.end()) { + std::set& objHandles = soit->second; + for (auto oit = objHandles.begin(); oit != objHandles.end(); ++oit) { + auto hit = handles.find(*oit); + if (hit != handles.end()) { + toRelease.insert(hit->second.object); + objects.erase(hit->second.object); + DEBUG_MSG("Erasing object %lu for closed session %lu", hit->first, hSession); + slotHandles[slotID].erase(*oit); + handles.erase(hit); + } } + sessionObjectHandles.erase(soit); } - sessionObjectHandles.erase(soit); - } - // Use the session counter to check if there are remaining open sessions. - CK_ULONG& count = slotSessionCount[slotID]; - if (count > 0) - count--; + // Use the session counter to check if there are remaining open sessions. + CK_ULONG& count = slotSessionCount[slotID]; + if (count > 0) + count--; - if (count > 0) - return; + if (count == 0) + allSessionsClosed(slotID, true); + } - // No more sessions open for this token, so remove all remaining object handles (token objects) - // for the given slotID. - slotSessionCount.erase(slotID); - allSessionsClosed(slotID, true); + for (auto* obj : toRelease) + obj->release(); } void HandleManager::allSessionsClosed(const CK_SLOT_ID slotID, bool isLocked) { - MutexLocker lock(isLocked ? NULL : handlesMutex); - + std::set toRelease; DEBUG_MSG("Closing all sessions for slot %lu", slotID); - // Erase all "session", "session object" and "token object" handles for a given slot id - // using the per-slot index instead of scanning the entire handles map. - std::map< CK_SLOT_ID, std::set >::iterator sit = slotHandles.find(slotID); - if (sit != slotHandles.end()) { - std::set& handleSet = sit->second; - for (std::set::iterator it = handleSet.begin(); it != handleSet.end(); ++it) { - std::map< CK_ULONG, Handle>::iterator hit = handles.find(*it); - if (hit != handles.end()) { - if (CKH_OBJECT == hit->second.kind) { - DEBUG_MSG("Erasing object %lu for empty slot %lu", hit->first, slotID); - objects.erase(hit->second.object); + { + MutexLocker lock(isLocked ? NULL : handlesMutex); + + // Erase all "session", "session object" and "token object" handles for a given slot id + // using the per-slot index instead of scanning the entire handles map. + auto sit = slotHandles.find(slotID); + if (sit != slotHandles.end()) { + auto& handleSet = sit->second; + for (auto it = handleSet.begin(); it != handleSet.end(); ++it) { + auto hit = handles.find(*it); + if (hit != handles.end()) { + if (CKH_OBJECT == hit->second.kind) { + DEBUG_MSG("Erasing object %lu for empty slot %lu", hit->first, slotID); + if (hit->second.hSession != CK_INVALID_HANDLE) + toRelease.insert(hit->second.object); + objects.erase(hit->second.object); + } + if (CKH_SESSION == hit->second.kind) + sessionObjectHandles.erase(*it); + handles.erase(hit); } - if (CKH_SESSION == hit->second.kind) - sessionObjectHandles.erase(*it); - handles.erase(hit); } + slotHandles.erase(sit); } - slotHandles.erase(sit); + + slotSessionCount.erase(slotID); } - slotSessionCount.erase(slotID); + for (auto* obj : toRelease) + obj->release(); } void HandleManager::tokenLoggedOut(const CK_SLOT_ID slotID) { - MutexLocker lock(handlesMutex); - - // Erase all private "token object" or "session object" handles for a given slot id - // using the per-slot index instead of scanning the entire handles map. - std::map< CK_SLOT_ID, std::set >::iterator sit = slotHandles.find(slotID); - if (sit == slotHandles.end()) - return; - - std::set& handleSet = sit->second; - for (std::set::iterator it = handleSet.begin(); it != handleSet.end(); ) { - std::map< CK_ULONG, Handle>::iterator hit = handles.find(*it); - if (hit != handles.end() && CKH_OBJECT == hit->second.kind && hit->second.isPrivate) { - // A private object is present for the given slotID so we need to remove it. - objects.erase(hit->second.object); - DEBUG_MSG("Erasing private object %lu for logged out token slot %lu", hit->first, slotID); - if (hit->second.hSession != CK_INVALID_HANDLE) - sessionObjectHandles[hit->second.hSession].erase(*it); - handles.erase(hit); - handleSet.erase(it++); - continue; + std::set toRelease; + { + MutexLocker lock(handlesMutex); + + // Erase all private "token object" or "session object" handles for a given slot id + // using the per-slot index instead of scanning the entire handles map. + auto sit = slotHandles.find(slotID); + if (sit == slotHandles.end()) + return; + + auto& handleSet = sit->second; + for (auto it = handleSet.begin(); it != handleSet.end(); ) { + auto hit = handles.find(*it); + if (hit != handles.end() && CKH_OBJECT == hit->second.kind && hit->second.isPrivate) { + // A private object is present for the given slotID so we need to remove it. + DEBUG_MSG("Erasing private object %lu for logged out token slot %lu", hit->first, slotID); + if (hit->second.hSession != CK_INVALID_HANDLE) { + sessionObjectHandles[hit->second.hSession].erase(*it); + toRelease.insert(hit->second.object); + } + objects.erase(hit->second.object); + handles.erase(hit); + handleSet.erase(it++); + continue; + } + ++it; } - ++it; } + + + for (auto* obj : toRelease) + obj->release(); } diff --git a/src/lib/handle_mgr/HandleManager.h b/src/lib/handle_mgr/HandleManager.h index 9fde65b95..486d72a34 100644 --- a/src/lib/handle_mgr/HandleManager.h +++ b/src/lib/handle_mgr/HandleManager.h @@ -35,6 +35,8 @@ #include "MutexFactory.h" #include "Handle.h" +#include "OSObject.h" +#include "Session.h" #include "cryptoki.h" #include @@ -49,23 +51,23 @@ class HandleManager virtual ~HandleManager(); - CK_SESSION_HANDLE addSession(CK_SLOT_ID slotID, CK_VOID_PTR session); - CK_VOID_PTR getSession(const CK_SESSION_HANDLE hSession); + CK_SESSION_HANDLE addSession(CK_SLOT_ID slotID, Session *session); + Session *getSession(const CK_SESSION_HANDLE hSession); // Add the session object and return a handle. For objects that have already been registered, check that the // slotID matches. The hSession may be different as the object may be added as part of a find objects operation. - CK_OBJECT_HANDLE addSessionObject(CK_SLOT_ID slotID, CK_SESSION_HANDLE hSession, bool isPrivate, CK_VOID_PTR object); + CK_OBJECT_HANDLE addSessionObject(CK_SLOT_ID slotID, CK_SESSION_HANDLE hSession, bool isPrivate, OSObject *object); // Add the token object and return a handle. For objects that have already been registered, check that the // slotID mathces. - CK_OBJECT_HANDLE addTokenObject(CK_SLOT_ID slotID, bool isPrivate, CK_VOID_PTR object); + CK_OBJECT_HANDLE addTokenObject(CK_SLOT_ID slotID, bool isPrivate, OSObject *object); // Get the object pointer associated with the given object handle. - CK_VOID_PTR getObject(const CK_OBJECT_HANDLE hObject); + OSObject *getObject(const CK_OBJECT_HANDLE hObject); // Get the object handle for the object pointer that has been previously registered. // When the object is not found CK_INVALID_HANDLE is returned. - CK_OBJECT_HANDLE getObjectHandle(CK_VOID_PTR object); + CK_OBJECT_HANDLE getObjectHandle(OSObject *object); // Remove the given object handle. void destroyObject(const CK_OBJECT_HANDLE hObject); @@ -86,17 +88,17 @@ class HandleManager private: Mutex* handlesMutex; - std::map< CK_ULONG, Handle> handles; - std::map< CK_VOID_PTR, CK_ULONG> objects; + std::map handles; + std::map objects; CK_ULONG handleCounter; // Secondary indexes for efficient cleanup without full-map scans. // Maps a session handle to the set of object handles created in that session. - std::map< CK_SESSION_HANDLE, std::set > sessionObjectHandles; + std::map > sessionObjectHandles; // Maps a slot ID to the set of all handles (sessions + objects) for that slot. - std::map< CK_SLOT_ID, std::set > slotHandles; + std::map > slotHandles; // Tracks the number of open sessions per slot to avoid counting scans. - std::map< CK_SLOT_ID, CK_ULONG> slotSessionCount; + std::map slotSessionCount; }; #endif // !_SOFTHSM_V2_HANDLEMANAGER_H diff --git a/src/lib/handle_mgr/Makefile.am b/src/lib/handle_mgr/Makefile.am index b588009fa..9b6b36158 100644 --- a/src/lib/handle_mgr/Makefile.am +++ b/src/lib/handle_mgr/Makefile.am @@ -6,6 +6,7 @@ AM_CPPFLAGS = -I$(srcdir)/.. \ -I$(srcdir)/../data_mgr \ -I$(srcdir)/../object_store \ -I$(srcdir)/../pkcs11 \ + -I$(srcdir)/../session_mgr \ -I$(srcdir)/../slot_mgr noinst_LTLIBRARIES = libsofthsm_handlemgr.la diff --git a/src/lib/handle_mgr/test/HandleManagerTests.cpp b/src/lib/handle_mgr/test/HandleManagerTests.cpp index fbf040363..415459342 100644 --- a/src/lib/handle_mgr/test/HandleManagerTests.cpp +++ b/src/lib/handle_mgr/test/HandleManagerTests.cpp @@ -34,9 +34,35 @@ #include #include #include "HandleManagerTests.h" +#include "OSObject.h" +#include "Session.h" CPPUNIT_TEST_SUITE_REGISTRATION(HandleManagerTests); +namespace { + +// Minimal OSObject for tests. We only need acquire()/release() to behave +// correctly and the pointer itself to serve as a unique identity. +class TestOSObject : public OSObject +{ +public: + bool attributeExists(CK_ATTRIBUTE_TYPE) { return false; } + OSAttribute getAttribute(CK_ATTRIBUTE_TYPE) { abort(); } + bool getBooleanValue(CK_ATTRIBUTE_TYPE, bool v) { return v; } + unsigned long getUnsignedLongValue(CK_ATTRIBUTE_TYPE, unsigned long v) { return v; } + ByteString getByteStringValue(CK_ATTRIBUTE_TYPE) { abort(); } + CK_ATTRIBUTE_TYPE nextAttributeType(CK_ATTRIBUTE_TYPE) { return CKA_CLASS; } + bool setAttribute(CK_ATTRIBUTE_TYPE, const OSAttribute&) { return true; } + bool deleteAttribute(CK_ATTRIBUTE_TYPE) { return true; } + bool isValid() { return true; } + bool startTransaction(Access) { return true; } + bool commitTransaction() { return true; } + bool abortTransaction() { return true; } + bool destroyObject() { return true; } +}; + +} // anonymous namespace + void HandleManagerTests::setUp() { handleManager = new HandleManager(); @@ -51,21 +77,27 @@ void HandleManagerTests::testHandleManager() { CPPUNIT_ASSERT(handleManager != NULL); - CK_SLOT_ID slotID = 1234; // we need a unique value + CK_SLOT_ID slotID = 1234; CK_SESSION_HANDLE hSession; - CK_VOID_PTR session = &hSession; // we need a unique value CK_SESSION_HANDLE hSession2; - CK_VOID_PTR session2 = &hSession2; // we need a unique value CK_OBJECT_HANDLE hObject; - CK_VOID_PTR object = &hObject; // we need a unique value CK_OBJECT_HANDLE hObject2; - CK_VOID_PTR object2 = &hObject2; // we need a unique value CK_OBJECT_HANDLE hObject3; - CK_VOID_PTR object3 = &hObject3; // we need a unique value CK_OBJECT_HANDLE hObject4; - CK_VOID_PTR object4 = &hObject4; // we need a unique value CK_OBJECT_HANDLE hObject5; - CK_VOID_PTR object5 = &hObject5; // we need a unique value + + // Sessions are passed as Session* but HandleManager never dereferences + // them, so casts from any unique pointer work. + Session* session = reinterpret_cast(&hSession); + Session* session2 = reinterpret_cast(&hSession2); + + // Objects must be real OSObjects because addSessionObject acquires and + // the cleanup paths release. Each starts at refcount=1. + OSObject* object = new TestOSObject(); + OSObject* object2 = new TestOSObject(); + OSObject* object3 = new TestOSObject(); + OSObject* object4 = new TestOSObject(); + OSObject* object5 = new TestOSObject(); // Check session object management. hSession = handleManager->addSession(slotID, session); @@ -75,7 +107,7 @@ void HandleManagerTests::testHandleManager() handleManager->sessionClosed(hSession); CPPUNIT_ASSERT(NULL == handleManager->getSession(hSession)); - // Add an object, hSession doesn't have to exists + // Add an object, hSession doesn't have to exist hObject = handleManager->addSessionObject(slotID, 4412412, true, object); CPPUNIT_ASSERT(hObject != CK_INVALID_HANDLE); CPPUNIT_ASSERT(object == handleManager->getObject(hObject)); @@ -103,76 +135,68 @@ void HandleManagerTests::testHandleManager() CPPUNIT_ASSERT(session == handleManager->getSession(hSession)); // Now some magic with a couple of objects - // First add a public object hObject = handleManager->addTokenObject(slotID, false, object); CPPUNIT_ASSERT(hObject != CK_INVALID_HANDLE); CPPUNIT_ASSERT(object == handleManager->getObject(hObject)); - // Now add a private object hObject2 = handleManager->addTokenObject(slotID, true, object2); CPPUNIT_ASSERT(hObject2 != CK_INVALID_HANDLE); CPPUNIT_ASSERT(object2 == handleManager->getObject(hObject2)); - // Now add another private object hObject3 = handleManager->addTokenObject(slotID, true, object3); CPPUNIT_ASSERT(hObject3 != CK_INVALID_HANDLE); CPPUNIT_ASSERT(object3 == handleManager->getObject(hObject3)); - // Adding the same object will return the same handle whether the object is marked private or public. + // Re-registering returns the same handle whether marked private or public. CPPUNIT_ASSERT(hObject2 == handleManager->addTokenObject(slotID, true, object2)); - // Because the private state of an object cannot be changed it won't be marked as public, it remains private CPPUNIT_ASSERT(hObject2 == handleManager->addTokenObject(slotID, false, object2)); - // It is not allowed to migrate an object from one slot to another, so here we return an invalid handle. + // Not allowed to migrate an object from one slot to another. CPPUNIT_ASSERT(CK_INVALID_HANDLE == handleManager->addTokenObject(124121, false, object2)); - // Now add another private session object hObject4 = handleManager->addSessionObject(slotID, hSession, true, object4); CPPUNIT_ASSERT(hObject4 != CK_INVALID_HANDLE); CPPUNIT_ASSERT(object4 == handleManager->getObject(hObject4)); - // Now add another public session object hObject5 = handleManager->addSessionObject(slotID, hSession, false, object5); CPPUNIT_ASSERT(hObject5 != CK_INVALID_HANDLE); CPPUNIT_ASSERT(object5 == handleManager->getObject(hObject5)); - // Logout, now private objects should be gone. + // Logout: private objects (both token and session) should be gone. handleManager->tokenLoggedOut(slotID); - CPPUNIT_ASSERT(object == handleManager->getObject(hObject)); - CPPUNIT_ASSERT(NULL == handleManager->getObject(hObject2)); // should still be private and removed. - CPPUNIT_ASSERT(NULL == handleManager->getObject(hObject3)); - CPPUNIT_ASSERT(NULL == handleManager->getObject(hObject4)); - CPPUNIT_ASSERT(object5 == handleManager->getObject(hObject5)); + CPPUNIT_ASSERT(object == handleManager->getObject(hObject)); // public token + CPPUNIT_ASSERT(NULL == handleManager->getObject(hObject2)); // private token + CPPUNIT_ASSERT(NULL == handleManager->getObject(hObject3)); // private token + CPPUNIT_ASSERT(NULL == handleManager->getObject(hObject4)); // private session + CPPUNIT_ASSERT(object5 == handleManager->getObject(hObject5)); // public session - // Create another valid session for the slot hSession2 = handleManager->addSession(slotID, session2); CPPUNIT_ASSERT(hSession2 != CK_INVALID_HANDLE); CPPUNIT_ASSERT(session2 == handleManager->getSession(hSession2)); handleManager->sessionClosed(hSession); - CPPUNIT_ASSERT(object == handleManager->getObject(hObject)); // token object should still be there. - CPPUNIT_ASSERT(NULL == handleManager->getObject(hObject5)); // session object should be gone. + CPPUNIT_ASSERT(object == handleManager->getObject(hObject)); // token object stays + CPPUNIT_ASSERT(NULL == handleManager->getObject(hObject5)); // session object gone - // Removing the last remaining session should kill the remaining handle. + // Closing the last session cascades into allSessionsClosed. handleManager->sessionClosed(hSession2); - CPPUNIT_ASSERT(NULL == handleManager->getObject(hObject)); // should be gone now. - + CPPUNIT_ASSERT(NULL == handleManager->getObject(hObject)); CPPUNIT_ASSERT(NULL == handleManager->getSession(hSession)); CPPUNIT_ASSERT(NULL == handleManager->getSession(hSession2)); - - // Create a valid session again - hSession = handleManager->addSession(slotID, session); - CPPUNIT_ASSERT(hSession != CK_INVALID_HANDLE); - CPPUNIT_ASSERT(session == handleManager->getSession(hSession)); - - // Create another valid session for the slot + // Bulk close + hSession = handleManager->addSession(slotID, session); hSession2 = handleManager->addSession(slotID, session2); - CPPUNIT_ASSERT(hSession2 != CK_INVALID_HANDLE); - CPPUNIT_ASSERT(session2 == handleManager->getSession(hSession2)); - handleManager->allSessionsClosed(slotID); - CPPUNIT_ASSERT(NULL == handleManager->getSession(hSession)); CPPUNIT_ASSERT(NULL == handleManager->getSession(hSession2)); + + // At this point HandleManager holds no references to any of our test + // objects; refcount on each should be 1 (the "creating" ref we still own). + // Release once to delete each. + object->release(); + object2->release(); + object3->release(); + object4->release(); + object5->release(); } diff --git a/src/lib/object_store/OSObject.h b/src/lib/object_store/OSObject.h index 6efaa6de5..e1bf4b8e0 100644 --- a/src/lib/object_store/OSObject.h +++ b/src/lib/object_store/OSObject.h @@ -38,12 +38,29 @@ #include "config.h" #include "OSAttribute.h" #include "cryptoki.h" +#include class OSObject { public: - // Destructor - virtual ~OSObject() { } + OSObject() : refCount(1) {} + + virtual ~OSObject() = default; + + // Acquire object - increase reference count + void acquire() noexcept + { + refCount.fetch_add(1, std::memory_order_relaxed); + } + + // Release object - decrement reference count + void release() noexcept + { + if (refCount.fetch_sub(1, std::memory_order_acq_rel) == 1) + { + delete this; + } + } // Check if the specified attribute exists virtual bool attributeExists(CK_ATTRIBUTE_TYPE type) = 0; @@ -89,6 +106,8 @@ class OSObject // Destroys the object (warning, any pointers to the object are no longer // valid after this call because delete is called!) virtual bool destroyObject() = 0; +private: + std::atomic refCount; }; #endif // !_SOFTHSM_V2_OSOBJECT_H diff --git a/src/lib/object_store/SessionObjectStore.cpp b/src/lib/object_store/SessionObjectStore.cpp index d8d8da6f8..f7c47ead1 100644 --- a/src/lib/object_store/SessionObjectStore.cpp +++ b/src/lib/object_store/SessionObjectStore.cpp @@ -59,12 +59,10 @@ SessionObjectStore::~SessionObjectStore() std::set cleanUp = allObjects; allObjects.clear(); - for (std::set::iterator i = cleanUp.begin(); i != cleanUp.end(); i++) + for (auto* obj : cleanUp) { - if ((*i) == NULL) continue; - - SessionObject* that = *i; - delete that; + if (obj == NULL) continue; + obj->release(); } MutexFactory::i()->recycleMutex(storeMutex); @@ -72,19 +70,18 @@ SessionObjectStore::~SessionObjectStore() int SessionObjectStore::getObjectCount() { - return objects.size(); + return objects.size(); } void SessionObjectStore::getObjects(std::set &inObjects) { - // Make sure that no other thread is in the process of changing - // the object list when we return it - MutexLocker lock(storeMutex); - - std::set::iterator it; - for (it=objects.begin(); it!=objects.end(); ++it) { - inObjects.insert(*it); - } + // Make sure that no other thread is in the process of changing + // the object list when we return it + MutexLocker lock(storeMutex); + for (auto* obj : objects) { + obj->acquire(); + inObjects.insert(obj); + } } void SessionObjectStore::getObjects(CK_SLOT_ID slotID, std::set &inObjects) @@ -92,11 +89,11 @@ void SessionObjectStore::getObjects(CK_SLOT_ID slotID, std::set &inOb // Make sure that no other thread is in the process of changing // the object list when we return it MutexLocker lock(storeMutex); - - std::set::iterator it; - for (it=objects.begin(); it!=objects.end(); ++it) { - if ((*it)->hasSlotID(slotID)) - inObjects.insert(*it); + for (auto* obj : objects) { + if (obj->hasSlotID(slotID)) { + obj->acquire(); + inObjects.insert(obj); + } } } @@ -129,90 +126,109 @@ SessionObject* SessionObjectStore::createObject(CK_SLOT_ID slotID, CK_SESSION_HA // Delete an object bool SessionObjectStore::deleteObject(SessionObject* object) { - MutexLocker lock(storeMutex); - - if (objects.find(object) == objects.end()) + SessionObject* toRelease = NULL; { - ERROR_MSG("Cannot delete non-existent object 0x%08X", object); - - return false; + MutexLocker lock(storeMutex); + if (objects.find(object) == objects.end()) + { + ERROR_MSG("Cannot delete non-existent object 0x%08X", object); + return false; + } + object->invalidate(); + objects.erase(object); + allObjects.erase(object); + toRelease = object; + } + if (toRelease) + { + toRelease->release(); } - - // Invalidate the object instance - object->invalidate(); - - objects.erase(object); - return true; } -// Indicate that a session has been closed; invalidates all objects -// associated with this session +// Indicate that a session has been closed - remove all its objects void SessionObjectStore::sessionClosed(CK_SESSION_HANDLE hSession) { - MutexLocker lock(storeMutex); - - std::set checkObjects = objects; - - for (std::set::iterator i = checkObjects.begin(); i != checkObjects.end(); i++) + std::set toRelease; { - if ((*i)->removeOnSessionClose(hSession)) - { - // Since the object remains in the allObjects set, any pointers to it will - // remain valid but it will no longer be returned when the set of objects - // is requested - objects.erase(*i); + MutexLocker lock(storeMutex); + for (auto it = objects.begin(); it != objects.end(); ) { + if ((*it)->removeOnSessionClose(hSession)) + { + toRelease.insert(*it); + allObjects.erase(*it); + it = objects.erase(it); + } + else + { + ++it; + } } - } + } + for (auto* obj : toRelease) + { + obj->release(); + } } void SessionObjectStore::allSessionsClosed(CK_SLOT_ID slotID) { - MutexLocker lock(storeMutex); - - std::set checkObjects = objects; - - for (std::set::iterator i = checkObjects.begin(); i != checkObjects.end(); i++) - { - if ((*i)->removeOnAllSessionsClose(slotID)) + std::set toRelease; { - // Since the object remains in the allObjects set, any pointers to it will - // remain valid but it will no longer be returned when the set of objects - // is requested - objects.erase(*i); + MutexLocker lock(storeMutex); + for (auto it = objects.begin(); it != objects.end(); ) { + if ((*it)->removeOnAllSessionsClose(slotID)) + { + toRelease.insert(*it); + allObjects.erase(*it); + it = objects.erase(it); + } + else + { + ++it; + } + } } + for (auto* obj : toRelease) + { + obj->release(); } } void SessionObjectStore::tokenLoggedOut(CK_SLOT_ID slotID) { - MutexLocker lock(storeMutex); - - std::set checkObjects = objects; - - for (std::set::iterator i = checkObjects.begin(); i != checkObjects.end(); i++) + std::set toRelease; { - if ((*i)->removeOnTokenLogout(slotID)) - { - // Since the object remains in the allObjects set, any pointers to it will - // remain valid but it will no longer be returned when the set of objects - // is requested - objects.erase(*i); + MutexLocker lock(storeMutex); + for (auto it = objects.begin(); it != objects.end(); ) { + if ((*it)->removeOnTokenLogout(slotID)) { + toRelease.insert(*it); + allObjects.erase(*it); + it = objects.erase(it); + } + else + { + ++it; + } } } + for (auto* obj : toRelease) + { + obj->release(); + } } // Clear the whole store void SessionObjectStore::clearStore() { - MutexLocker lock(storeMutex); - - objects.clear(); - std::set clearObjects = allObjects; - allObjects.clear(); - - for (std::set::iterator i = clearObjects.begin(); i != clearObjects.end(); i++) + std::set clearObjects; + { + MutexLocker lock(storeMutex); + objects.clear(); + clearObjects.swap(allObjects); + } + for (auto* obj : clearObjects) { - delete *i; + obj->release(); } } diff --git a/src/lib/object_store/test/SessionObjectStoreTests.cpp b/src/lib/object_store/test/SessionObjectStoreTests.cpp index 62439aeb4..710d3e1c3 100644 --- a/src/lib/object_store/test/SessionObjectStoreTests.cpp +++ b/src/lib/object_store/test/SessionObjectStoreTests.cpp @@ -43,6 +43,18 @@ CPPUNIT_TEST_SUITE_REGISTRATION(SessionObjectStoreTests); +namespace { + +// Helper to release all objects +void releaseAll(std::set& s) +{ + for (auto* o : s) + o->release(); + s.clear(); +} + +} // anonymous namespace + void SessionObjectStoreTests::setUp() { } @@ -121,10 +133,12 @@ void SessionObjectStoreTests::testCreateDeleteObjects() } } + releaseAll(objects); + obj2 = NULL; + // Verify that it was indeed removed CPPUNIT_ASSERT(testStore->getObjectCount() == 2); - objects.clear(); testStore->getObjects(objects); bool present3[2] = { false, false }; @@ -150,6 +164,8 @@ void SessionObjectStoreTests::testCreateDeleteObjects() CPPUNIT_ASSERT(present3[j] == true); } + releaseAll(objects); + delete testStore; } @@ -181,115 +197,102 @@ void SessionObjectStoreTests::testMultiSession() // Check that the store contains 3 objects CPPUNIT_ASSERT(store->getObjectCount() == 3); - // Check that all three objects are distinct and present + // Check that all objects are distinct and present std::set objects; - store->getObjects(objects); - bool present1[3] = { false, false, false }; - - for (std::set::iterator i = objects.begin(); i != objects.end(); i++) + store->getObjects(objects); + bool present[5] = { false, false, false, false, false }; + for (auto* obj : objects) { - ByteString retrievedId; - - CPPUNIT_ASSERT((*i)->isValid()); - CPPUNIT_ASSERT((*i)->attributeExists(CKA_ID)); - - CPPUNIT_ASSERT((*i)->getAttribute(CKA_ID).isByteStringAttribute()); - - for (int j = 0; j < 3; j++) + CPPUNIT_ASSERT(obj->isValid()); + CPPUNIT_ASSERT(obj->attributeExists(CKA_ID)); + CPPUNIT_ASSERT(obj->getAttribute(CKA_ID).isByteStringAttribute()); + for (int j = 0; j < 5; ++j) { - if ((*i)->getAttribute(CKA_ID).getByteStringValue() == id[j]) + if (obj->getAttribute(CKA_ID).getByteStringValue() == id[j]) { - present1[j] = true; + present[j] = true; } } } + CPPUNIT_ASSERT(present[0] && present[1] && present[2]); + CPPUNIT_ASSERT(!present[3] && !present[4]); + releaseAll(objects); - for (int j = 0; j < 3; j++) - { - CPPUNIT_ASSERT(present1[j] == true); - } - - // Now indicate that the second session has been closed + // Close session 2 -> obj2 is destroyed. Its pointer is now stale. store->sessionClosed(2); + obj2 = NULL; // hygiene: prevent accidental use-after-free in the test - // Verify that it was indeed removed CPPUNIT_ASSERT(store->getObjectCount() == 2); - objects.clear(); store->getObjects(objects); - bool present3[2] = { false, false }; - - for (std::set::iterator i = objects.begin(); i != objects.end(); i++) + memset(present, 0, sizeof(present)); + for (auto* obj : objects) { - ByteString retrievedId; - - CPPUNIT_ASSERT((*i)->isValid()); - CPPUNIT_ASSERT((*i)->attributeExists(CKA_ID)); - - CPPUNIT_ASSERT((*i)->getAttribute(CKA_ID).isByteStringAttribute()); - - if ((*i)->getAttribute(CKA_ID).getByteStringValue() == id[0]) + for (int j = 0; j < 5; ++j) { - present3[0] = true; - } - if ((*i)->getAttribute(CKA_ID).getByteStringValue() == id[2]) - { - present3[1] = true; + if (obj->getAttribute(CKA_ID).getByteStringValue() == id[j]) + { + present[j] = true; + } } } + CPPUNIT_ASSERT(present[0] && !present[1] && present[2]); + releaseAll(objects); - for (int j = 0; j < 2; j++) - { - CPPUNIT_ASSERT(present3[j] == true); - } - - // Create two more objects for session 7 + // Create two more in session 7, with distinct IDs so we can identify them by attribute SessionObject* obj4 = store->createObject(1, 7); CPPUNIT_ASSERT(obj4 != NULL); + obj4->setAttribute(CKA_ID, idAtt[3]); SessionObject* obj5 = store->createObject(1, 7); CPPUNIT_ASSERT(obj5 != NULL); + obj5->setAttribute(CKA_ID, idAtt[4]); CPPUNIT_ASSERT(store->getObjectCount() == 4); - // Close session 1 + // Close session 1 -> obj1 destroyed. store->sessionClosed(1); + obj1 = NULL; CPPUNIT_ASSERT(store->getObjectCount() == 3); - objects.clear(); store->getObjects(objects); + memset(present, 0, sizeof(present)); + for (auto* obj : objects) + { + for (int j = 0; j < 5; ++j) + { + if (obj->getAttribute(CKA_ID).getByteStringValue() == id[j]) + { + present[j] = true; + } + } + } + CPPUNIT_ASSERT(!present[0] && !present[1]); + CPPUNIT_ASSERT(present[2] && present[3] && present[4]); - CPPUNIT_ASSERT(objects.find(obj1) == objects.end()); - CPPUNIT_ASSERT(objects.find(obj2) == objects.end()); - CPPUNIT_ASSERT(objects.find(obj3) != objects.end()); - CPPUNIT_ASSERT(objects.find(obj4) != objects.end()); - CPPUNIT_ASSERT(objects.find(obj5) != objects.end()); - - CPPUNIT_ASSERT(!obj1->isValid()); - CPPUNIT_ASSERT(!obj2->isValid()); + // The objects whose sessions are still open should still be usable CPPUNIT_ASSERT(obj3->isValid()); CPPUNIT_ASSERT(obj4->isValid()); CPPUNIT_ASSERT(obj5->isValid()); - // Close session 7 + releaseAll(objects); + + // Close session 7 -> obj4, obj5 destroyed store->sessionClosed(7); + obj4 = NULL; + obj5 = NULL; CPPUNIT_ASSERT(store->getObjectCount() == 1); - objects.clear(); store->getObjects(objects); + CPPUNIT_ASSERT(objects.size() == 1); - CPPUNIT_ASSERT(objects.find(obj1) == objects.end()); - CPPUNIT_ASSERT(objects.find(obj2) == objects.end()); - CPPUNIT_ASSERT(objects.find(obj3) != objects.end()); - CPPUNIT_ASSERT(objects.find(obj4) == objects.end()); - CPPUNIT_ASSERT(objects.find(obj5) == objects.end()); - - CPPUNIT_ASSERT(!obj1->isValid()); - CPPUNIT_ASSERT(!obj2->isValid()); + std::set::iterator it = objects.begin(); + CPPUNIT_ASSERT((*it)->isValid()); + CPPUNIT_ASSERT((*it)->getAttribute(CKA_ID).getByteStringValue() == id[2]); CPPUNIT_ASSERT(obj3->isValid()); - CPPUNIT_ASSERT(!obj4->isValid()); - CPPUNIT_ASSERT(!obj5->isValid()); + + releaseAll(objects); delete store; } @@ -318,4 +321,3 @@ void SessionObjectStoreTests::testWipeStore() delete store; } -