Don't adopt unknown session IDs from the request cookie

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) {

lib/src/SessionManager.h

@@ -14,6 +14,7 @@
 
 #pragma once
 
+#include <drogon/exports.h>
 #include <drogon/Session.h>
 #include <drogon/drogon_callbacks.h>
 #include <drogon/CacheMap.h>
@@ -27,7 +28,7 @@
 
 namespace drogon
 {
-class SessionManager : public trantor::NonCopyable
+class DROGON_EXPORT SessionManager : public trantor::NonCopyable
 {
   public:
     using IdGeneratorCallback = std::function<std::string()>;

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

lib/tests/unittests/SessionManagerTest.cc

@@ -0,0 +1,40 @@
+#include "../../lib/src/SessionManager.h"
+#include <drogon/drogon_test.h>
+#include <trantor/net/EventLoopThread.h>
+
+using namespace drogon;
+
+DROGON_TEST(SessionManagerTest)
+{
+    trantor::EventLoopThread loopThread;
+    loopThread.run();
+    std::vector<AdviceStartSessionCallback> startAdvices;
+    std::vector<AdviceDestroySessionCallback> 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);
+}