diff --git a/lib/src/SessionManager.cc b/lib/src/SessionManager.cc index 39c9682f3b..2cd3fb1d4c 100644 --- a/lib/src/SessionManager.cc +++ b/lib/src/SessionManager.cc @@ -94,6 +94,19 @@ SessionPtr SessionManager::getSession(const std::string &sessionID, { assert(!sessionID.empty()); SessionPtr sessionPtr; + if (!needToSet) + { + // The ID came from the request's cookie. Only reuse it when it names a + // live session; otherwise start a new session under a server-generated + // ID. Adopting the client's value would let anyone choose session keys + // (session fixation, unbounded cache growth, and chosen keys that all + // land in the same hash bucket). + if (sessionMapPtr_->findAndFetch(sessionID, sessionPtr) && sessionPtr) + { + return sessionPtr; + } + return getSession(idGeneratorCallback_(), true); + } sessionMapPtr_->modify( sessionID, [&sessionPtr, &sessionID, needToSet](SessionPtr &sessionInCache) { diff --git a/lib/src/SessionManager.h b/lib/src/SessionManager.h index 70f300e458..9b2a8d8dcc 100644 --- a/lib/src/SessionManager.h +++ b/lib/src/SessionManager.h @@ -14,6 +14,7 @@ #pragma once +#include #include #include #include @@ -27,7 +28,7 @@ namespace drogon { -class SessionManager : public trantor::NonCopyable +class DROGON_EXPORT SessionManager : public trantor::NonCopyable { public: using IdGeneratorCallback = std::function; diff --git a/lib/tests/CMakeLists.txt b/lib/tests/CMakeLists.txt index 514e8692e7..5f95af65a8 100644 --- a/lib/tests/CMakeLists.txt +++ b/lib/tests/CMakeLists.txt @@ -26,6 +26,7 @@ set(UNITTEST_SOURCES unittests/HttpFullDateTest.cc unittests/MainLoopTest.cc unittests/CacheMapTest.cc + unittests/SessionManagerTest.cc unittests/StringOpsTest.cc unittests/ControllerCreationTest.cc unittests/MultiPartParserTest.cc diff --git a/lib/tests/unittests/SessionManagerTest.cc b/lib/tests/unittests/SessionManagerTest.cc new file mode 100644 index 0000000000..3e5d83d610 --- /dev/null +++ b/lib/tests/unittests/SessionManagerTest.cc @@ -0,0 +1,40 @@ +#include "../../lib/src/SessionManager.h" +#include +#include + +using namespace drogon; + +DROGON_TEST(SessionManagerTest) +{ + trantor::EventLoopThread loopThread; + loopThread.run(); + std::vector startAdvices; + std::vector destroyAdvices; + int generated = 0; + SessionManager manager( + loopThread.getLoop(), 0, startAdvices, destroyAdvices, [&generated]() { + return "server-id-" + std::to_string(++generated); + }); + + // A new session under a server-generated ID is stored. + auto session = manager.getSession("server-id-0", true); + CHECK(session->sessionId() == "server-id-0"); + + // The client sends that ID back: the same session is returned. + CHECK(manager.getSession("server-id-0", false) == session); + CHECK(generated == 0); + + // An ID the server never issued is not adopted: the client gets a new + // session with a fresh server-generated ID. + auto other = manager.getSession("attacker-chosen", false); + CHECK(other != session); + CHECK(other->sessionId() == "server-id-1"); + CHECK(generated == 1); + + // The new session is reachable under its own ID, and the unknown ID still + // isn't. + CHECK(manager.getSession("server-id-1", false) == other); + auto again = manager.getSession("attacker-chosen", false); + CHECK(again->sessionId() == "server-id-2"); + CHECK(again != other); +}