Harden OTP system: brute-force protection, memory cleanup, and key collision fix

include/server/chat/PrivateChatDirector.hpp

@@ -14,7 +14,7 @@ namespace server
 {
 
 //! Chat client OTP constant
-constexpr uint32_t PrivateChatOtpConstant = 0x14E05CE5;
+constexpr uint32_t PrivateChatOtpConstant = 0x7C3D91A2;
 
 class ServerInstance;
 

include/server/system/OtpSystem.hpp

@@ -6,8 +6,8 @@
 #define OTPSYSTEM_HPP
 
 #include <chrono>
-#include <random>
 #include <mutex>
+#include <random>
 #include <unordered_map>
 
 namespace server
@@ -16,21 +16,60 @@ namespace server
 class OtpSystem
 {
 public:
+  struct Settings
+  {
+    std::chrono::seconds codeTtl{30};
+    uint32_t maxFailedAttempts{5};
+    std::chrono::seconds lockoutDuration{60};
+    std::chrono::seconds purgeInterval{60};
+
+    Settings() = default;
+  };
+
+  OtpSystem();
+  explicit OtpSystem(Settings settings);
+
+  //! Grants a one-time code for the given key.
+  //! If a code already exists for this key, it is replaced.
+  //! @param key Identity key the code is bound to.
+  //! @return The generated one-time code.
   uint32_t GrantCode(size_t key);
+
+  //! Authorizes a one-time code for the given key.
+  //! Enforces brute-force protection: after too many failed attempts,
+  //! the key is temporarily locked out.
+  //! @param key Identity key the code is bound to.
+  //! @param code The code to verify.
+  //! @param consume If true, the code is consumed (erased) on success.
+  //! @return True if the code is valid and the key is not locked out.
   bool AuthorizeCode(size_t key, uint32_t code, bool consume = true);
 
+  //! Removes all expired codes and stale lockout entries.
+  //! Called automatically on a periodic basis, but can be invoked manually.
+  void PurgeExpired();
+
 private:
+  Settings _settings;
+
   struct Code
   {
     std::chrono::steady_clock::time_point expiry{};
     uint32_t code{};
   };
 
+  struct AttemptRecord
+  {
+    uint32_t failedAttempts{0};
+    std::chrono::steady_clock::time_point lockoutExpiry{};
+  };
+
   std::mutex _codesMutex;
-  std::random_device _rd;
+  std::mt19937 _rng;
   std::unordered_map<size_t, Code> _codes;
+  std::unordered_map<size_t, AttemptRecord> _attempts;
+  std::chrono::steady_clock::time_point _lastPurge;
 };
 
 } // namespace server
 
-#endif //OTPSYSTEM_HPP
+#endif // OTPSYSTEM_HPP

src/server/system/OtpSystem.cpp

@@ -4,37 +4,136 @@
 
 #include "server/system/OtpSystem.hpp"
 
+#include <spdlog/spdlog.h>
+
 namespace server
 {
 
+OtpSystem::OtpSystem()
+  : OtpSystem(Settings{})
+{
+}
+
+OtpSystem::OtpSystem(Settings settings)
+  : _settings(std::move(settings))
+  , _rng(std::random_device{}())
+  , _lastPurge(std::chrono::steady_clock::now())
+{
+}
+
 uint32_t OtpSystem::GrantCode(const size_t key)
 {
   std::scoped_lock lock(_codesMutex);
-  const auto [iter, inserted] = _codes.insert_or_assign(
+
+  PurgeExpired();
+
+  _codes.insert_or_assign(
     key,
     Code{
-      .expiry = std::chrono::steady_clock::now() + std::chrono::seconds(30),
-      .code = _rd()});
+      .expiry = std::chrono::steady_clock::now() + _settings.codeTtl,
+      .code = static_cast<uint32_t>(_rng())});
 
-  return iter->second.code;
+  // Reset failed attempts when a new code is granted for this key,
+  // since the legitimate server is issuing a fresh code.
+  _attempts.erase(key);
+
+  return _codes.at(key).code;
 }
 
 bool OtpSystem::AuthorizeCode(const size_t key, const uint32_t code, bool consume)
 {
   std::scoped_lock lock(_codesMutex);
 
+  const auto now = std::chrono::steady_clock::now();
+
+  // Check brute-force lockout.
+  if (const auto attemptIter = _attempts.find(key); attemptIter != _attempts.end())
+  {
+    const auto& record = attemptIter->second;
+    if (record.failedAttempts >= _settings.maxFailedAttempts
+        && now < record.lockoutExpiry)
+    {
+      spdlog::warn("OTP authorization for key {} denied: locked out after {} failed attempts",
+        key, record.failedAttempts);
+      return false;
+    }
+  }
+
   const auto codeIter = _codes.find(key);
   if (codeIter == _codes.cend())
+  {
+    auto& record = _attempts[key];
+    record.failedAttempts++;
+    if (record.failedAttempts >= _settings.maxFailedAttempts)
+      record.lockoutExpiry = now + _settings.lockoutDuration;
     return false;
+  }
 
   const Code& ctx = codeIter->second;
 
-  const bool expired = std::chrono::steady_clock::now() > ctx.expiry;
+  const bool expired = now > ctx.expiry;
   const bool authorized = not expired && ctx.code == code;
-  if (authorized && consume)
-    _codes.erase(codeIter);
+
+  if (authorized)
+  {
+    if (consume)
+      _codes.erase(codeIter);
+
+    _attempts.erase(key);
+  }
+  else
+  {
+    auto& record = _attempts[key];
+    record.failedAttempts++;
+
+    if (record.failedAttempts >= _settings.maxFailedAttempts)
+    {
+      record.lockoutExpiry = now + _settings.lockoutDuration;
+      spdlog::warn("OTP key {} locked out for {}s after {} failed attempts",
+        key,
+        _settings.lockoutDuration.count(),
+        record.failedAttempts);
+    }
+
+    if (expired)
+      _codes.erase(codeIter);
+  }
 
   return authorized;
 }
 
+void OtpSystem::PurgeExpired()
+{
+  const auto now = std::chrono::steady_clock::now();
+
+  if (now - _lastPurge < _settings.purgeInterval)
+    return;
+
+  _lastPurge = now;
+
+  // Purge expired codes.
+  for (auto it = _codes.begin(); it != _codes.end();)
+  {
+    if (now > it->second.expiry)
+      it = _codes.erase(it);
+    else
+      ++it;
+  }
+
+  // Purge stale lockout records whose lockout has expired
+  // and whose failure count is below the threshold (i.e. old records).
+  for (auto it = _attempts.begin(); it != _attempts.end();)
+  {
+    const auto& record = it->second;
+    if (record.failedAttempts >= _settings.maxFailedAttempts
+        && now > record.lockoutExpiry)
+      it = _attempts.erase(it);
+    else if (record.failedAttempts < _settings.maxFailedAttempts
+             && now > record.lockoutExpiry)
+      it = _attempts.erase(it);
+    else
+      ++it;
+  }
+}
+
 } // namespace server