From 9270d6814cd25da0ebd71cfdbb6315665964e358 Mon Sep 17 00:00:00 2001 From: emeric Date: Sun, 28 Jul 2019 16:09:44 +0200 Subject: [PATCH] Simplified the login throttler, corrected races on throttler access, recreate the auth cookie with the remaining duration --- src/auth/AuthService.cpp | 129 +++++++++++++++++++++++++++++++----- src/auth/AuthService.hpp | 32 ++++++++- src/auth/LoginThrottler.cpp | 30 ++------- src/auth/LoginThrottler.hpp | 10 +-- src/database/Session.cpp | 1 + src/database/User.hpp | 5 +- src/ui/Auth.cpp | 65 ++++++------------ src/ui/Auth.hpp | 5 +- src/ui/LmsApplication.cpp | 2 +- 9 files changed, 179 insertions(+), 100 deletions(-) diff --git a/src/auth/AuthService.cpp b/src/auth/AuthService.cpp index 7418fba8..c91795a9 100644 --- a/src/auth/AuthService.cpp +++ b/src/auth/AuthService.cpp @@ -26,46 +26,66 @@ #include #include "database/Session.hpp" +#include "utils/Exception.hpp" #include "utils/Utils.hpp" #include "utils/Logger.hpp" namespace Auth { AuthService::AuthService(std::size_t maxThrottlerEntries) -: _loginThrottler {maxThrottlerEntries} +: _passwordLoginThrottler {maxThrottlerEntries} +, _tokenLoginThrottler {maxThrottlerEntries} { } -AuthService::PasswordCheckResult -AuthService::checkUserPassword(Database::Session& session, const boost::asio::ip::address& clientAddress, const std::string& loginName, const std::string& password) +static +bool +checkUserPassword(Database::Session& session, const std::string& loginName, const std::string& password) { - if (_loginThrottler.isClientThrottled(clientAddress)) - return PasswordCheckResult::Throttled; - Database::User::PasswordHash passwordHash; { auto transaction {session.createSharedTransaction()}; const Database::User::pointer user {Database::User::getByLoginName(session, loginName)}; if (!user) - { - _loginThrottler.onBadClientAttempt(clientAddress); - return PasswordCheckResult::Mismatch; - } + return false; passwordHash = user->getPasswordHash(); } const Wt::Auth::BCryptHashFunction hashFunc {6}; - if (hashFunc.verify(password, passwordHash.salt, passwordHash.hash)) + return hashFunc.verify(password, passwordHash.salt, passwordHash.hash); +} + + +AuthService::PasswordCheckResult +AuthService::checkUserPassword(Database::Session& session, const boost::asio::ip::address& clientAddress, const std::string& loginName, const std::string& password) +{ + // Do not waste too much resource on brute force attacks (optim) { - _loginThrottler.onGoodClientAttempt(clientAddress); - return PasswordCheckResult::Match; + std::shared_lock lock {_passwordCheckMutex}; + + if (_passwordLoginThrottler.isClientThrottled(clientAddress)) + return PasswordCheckResult::Throttled; } - else + + const bool match {Auth::checkUserPassword(session, loginName, password)}; { - _loginThrottler.onBadClientAttempt(clientAddress); - return PasswordCheckResult::Mismatch; + std::unique_lock lock {_passwordCheckMutex}; + + if (_passwordLoginThrottler.isClientThrottled(clientAddress)) + return PasswordCheckResult::Throttled; + + if (match) + { + _passwordLoginThrottler.onGoodClientAttempt(clientAddress); + return PasswordCheckResult::Match; + } + else + { + _passwordLoginThrottler.onBadClientAttempt(clientAddress); + return PasswordCheckResult::Mismatch; + } } } @@ -93,5 +113,82 @@ AuthService::evaluatePasswordStrength(const std::string& loginName, const std::s return validator.evaluateStrength(password, loginName, "").isValid(); } + +std::string +AuthService::createAuthToken(Database::Session& session, Database::IdType userId, const Wt::WDateTime& expiry) +{ + const std::string secret {Wt::WRandom::generateId(64)}; + + auto transaction {session.createUniqueTransaction()}; + + Database::User::pointer user {Database::User::getById(session, userId)}; + if (!user) + throw LmsException {"User deleted"}; + + Database::AuthToken::pointer authToken {Database::AuthToken::create(session, secret, expiry, user)}; + + LMS_LOG(UI, DEBUG) << "Created auth token for user '" << user->getLoginName() << "', expiry = " << expiry.toString(); + + if (user->getAuthTokensCount() >= 50) + Database::AuthToken::removeExpiredTokens(session, Wt::WDateTime::currentDateTime()); + + return secret; +} + +static +boost::optional +processAuthToken(Database::Session& session, const std::string& tokenValue) +{ + auto transaction {session.createUniqueTransaction()}; + + Database::AuthToken::pointer authToken {Database::AuthToken::getByValue(session, tokenValue)}; + if (!authToken) + return boost::none; + + if (authToken->getExpiry() < Wt::WDateTime::currentDateTime()) + { + authToken.remove(); + return boost::none; + } + + LMS_LOG(UI, DEBUG) << "Found auth token for user '" << authToken->getUser()->getLoginName() << "'!"; + + AuthService::AuthTokenProcessResult::AuthTokenInfo res {authToken->getUser().id(), authToken->getExpiry()}; + authToken.remove(); + + return res; +} + + +AuthService::AuthTokenProcessResult +AuthService::processAuthToken(Database::Session& session, const boost::asio::ip::address& clientAddress, const std::string& tokenValue) +{ + // Do not waste too much resource on brute force attacks (optim) + { + std::shared_lock lock {_tokenCheckMutex}; + + if (_tokenLoginThrottler.isClientThrottled(clientAddress)) + return AuthTokenProcessResult {AuthTokenProcessResult::State::Throttled}; + } + + auto res {Auth::processAuthToken(session, tokenValue)}; + { + std::unique_lock lock {_tokenCheckMutex}; + + if (_tokenLoginThrottler.isClientThrottled(clientAddress)) + return AuthTokenProcessResult {AuthTokenProcessResult::State::Throttled}; + + if (!res) + { + _tokenLoginThrottler.onBadClientAttempt(clientAddress); + return AuthTokenProcessResult {AuthTokenProcessResult::State::NotFound}; + } + + _tokenLoginThrottler.onGoodClientAttempt(clientAddress); + return AuthTokenProcessResult {AuthTokenProcessResult::State::Found, std::move(*res)}; + } +} + + } // namespace Auth diff --git a/src/auth/AuthService.hpp b/src/auth/AuthService.hpp index 7aecea54..4968d64c 100644 --- a/src/auth/AuthService.hpp +++ b/src/auth/AuthService.hpp @@ -23,10 +23,12 @@ #include +#include #include #include "LoginThrottler.hpp" #include "database/User.hpp" +#include "database/Types.hpp" namespace Database { @@ -62,9 +64,37 @@ namespace Auth { Database::User::PasswordHash hashPassword(const std::string& password) const; bool evaluatePasswordStrength(const std::string& loginName, const std::string& password) const; + // Auth Token services + struct AuthTokenProcessResult + { + enum class State + { + Found, + Throttled, + NotFound, + }; + + struct AuthTokenInfo + { + Database::IdType userId; + Wt::WDateTime expiry; + }; + + State state; + boost::optional authTokenInfo; + }; + + // Removed if found + AuthTokenProcessResult processAuthToken(Database::Session& session, const boost::asio::ip::address& clientAddress, const std::string& tokenValue); + std::string createAuthToken(Database::Session& session, Database::IdType userid, const Wt::WDateTime& expiry); + private: - LoginThrottler _loginThrottler; + std::shared_timed_mutex _passwordCheckMutex; + std::shared_timed_mutex _tokenCheckMutex; + + LoginThrottler _passwordLoginThrottler; + LoginThrottler _tokenLoginThrottler; }; } diff --git a/src/auth/LoginThrottler.cpp b/src/auth/LoginThrottler.cpp index 55087e5e..71df61dd 100644 --- a/src/auth/LoginThrottler.cpp +++ b/src/auth/LoginThrottler.cpp @@ -54,7 +54,7 @@ LoginThrottler::removeOutdatedEntries() for (auto it {std::begin(_attemptsInfo)}; it != std::end(_attemptsInfo); ) { - if (it->second.nextValidAttempt <= now) + if (it->second <= now) it = _attemptsInfo.erase(it); else ++it; @@ -68,32 +68,14 @@ LoginThrottler::onBadClientAttempt(const boost::asio::ip::address& address) const Wt::WDateTime now {Wt::WDateTime::currentDateTime()}; - std::unique_lock lock {_mutex}; - if (_attemptsInfo.size() >= _maxEntries) removeOutdatedEntries(); - // If still full, kill one random entry if (_attemptsInfo.size() >= _maxEntries) _attemptsInfo.erase(pickRandom(_attemptsInfo)); - AttemptsInfo& attemptsInfo {_attemptsInfo[address]}; + _attemptsInfo[address] = now.addSecs(3); - attemptsInfo.nbSuccessiveBadAttempts++; - - if (attemptsInfo.nbSuccessiveBadAttempts >= 50) - attemptsInfo.nextValidAttempt = now.addSecs(60); - if (attemptsInfo.nbSuccessiveBadAttempts >= 20) - attemptsInfo.nextValidAttempt = now.addSecs(10); - else if (attemptsInfo.nbSuccessiveBadAttempts >= 10) - attemptsInfo.nextValidAttempt = now.addSecs(5); - else if (attemptsInfo.nbSuccessiveBadAttempts >= 5) - attemptsInfo.nextValidAttempt = now.addSecs(2); - else if (attemptsInfo.nbSuccessiveBadAttempts >= 2) - attemptsInfo.nextValidAttempt = now.addSecs(1); - else - attemptsInfo.nextValidAttempt = now; - - LMS_LOG(AUTH, INFO) << "Registering bad attempt for '" << clientAddress.to_string() << "' (" << attemptsInfo.nbSuccessiveBadAttempts << " successive bad attempts)"; + LMS_LOG(AUTH, INFO) << "Registering bad attempt for '" << clientAddress.to_string() << "'"; } void @@ -101,8 +83,6 @@ LoginThrottler::onGoodClientAttempt(const boost::asio::ip::address& address) { const boost::asio::ip::address clientAddress {getAddressToThrottle(address)}; - std::unique_lock lock {_mutex}; - _attemptsInfo.erase(address); } @@ -111,13 +91,11 @@ LoginThrottler::isClientThrottled(const boost::asio::ip::address& address) const { const boost::asio::ip::address clientAddress {getAddressToThrottle(address)}; - std::shared_lock lock {_mutex}; - auto it {_attemptsInfo.find(address)}; if (it == _attemptsInfo.end()) return false; - return it->second.nextValidAttempt > Wt::WDateTime::currentDateTime(); + return it->second > Wt::WDateTime::currentDateTime(); } } // Auth diff --git a/src/auth/LoginThrottler.hpp b/src/auth/LoginThrottler.hpp index ea2ca433..401e89ee 100644 --- a/src/auth/LoginThrottler.hpp +++ b/src/auth/LoginThrottler.hpp @@ -37,6 +37,7 @@ class LoginThrottler public: LoginThrottler(std::size_t maxEntries) : _maxEntries {maxEntries} {} + // user must lock these calls to avoid races bool isClientThrottled(const boost::asio::ip::address& address) const; void onBadClientAttempt(const boost::asio::ip::address& address); void onGoodClientAttempt(const boost::asio::ip::address& address); @@ -45,16 +46,9 @@ class LoginThrottler void removeOutdatedEntries(); - struct AttemptsInfo - { - std::size_t nbSuccessiveBadAttempts {}; - Wt::WDateTime nextValidAttempt; - }; - const std::size_t _maxEntries; - mutable std::shared_timed_mutex _mutex; - std::unordered_map _attemptsInfo; + std::unordered_map _attemptsInfo; }; diff --git a/src/database/Session.cpp b/src/database/Session.cpp index 73426482..93ead53e 100644 --- a/src/database/Session.cpp +++ b/src/database/Session.cpp @@ -213,6 +213,7 @@ Session::prepareTables() _session.execute("CREATE INDEX IF NOT EXISTS artist_sort_name_nocase_idx ON artist(sort_name COLLATE NOCASE)"); _session.execute("CREATE INDEX IF NOT EXISTS artist_mbid_idx ON artist(mbid)"); _session.execute("CREATE INDEX IF NOT EXISTS auth_token_user_idx ON auth_token(user_id)"); + _session.execute("CREATE INDEX IF NOT EXISTS auth_token_value_idx ON auth_token(value)"); _session.execute("CREATE INDEX IF NOT EXISTS cluster_name_idx ON cluster(name)"); _session.execute("CREATE INDEX IF NOT EXISTS cluster_cluster_type_idx ON cluster(cluster_type_id)"); _session.execute("CREATE INDEX IF NOT EXISTS cluster_type_name_idx ON cluster_type(name)"); diff --git a/src/database/User.hpp b/src/database/User.hpp index a0376078..4d61c13a 100644 --- a/src/database/User.hpp +++ b/src/database/User.hpp @@ -61,10 +61,12 @@ class AuthToken static pointer create(Session& session, const std::string& value, const Wt::WDateTime&expiry, Wt::Dbo::ptr user); static void removeExpiredTokens(Session& session, Wt::WDateTime now); static pointer getByValue(Session& session, const std::string& value); + static pointer getById(Session& session, IdType tokenId); // Accessors - Wt::Dbo::ptr getUser() const { return _user; } const Wt::WDateTime& getExpiry() const { return _expiry; } + Wt::Dbo::ptr getUser() const { return _user; } + const std::string& getValue() const { return _value; } template void persist(Action& a) @@ -121,6 +123,7 @@ class User : public Wt::Dbo::Dbo const std::string& getLoginName() const { return _loginName; } PasswordHash getPasswordHash() const { return PasswordHash {_passwordSalt, _passwordHash}; } Wt::WDateTime getLastLogin() const { return _lastLogin; } + std::size_t getAuthTokensCount() const { return _authTokens.size(); } // write void setLastLogin(const Wt::WDateTime& dateTime) { _lastLogin = dateTime; } diff --git a/src/ui/Auth.cpp b/src/ui/Auth.cpp index 0c21424e..910670ad 100644 --- a/src/ui/Auth.cpp +++ b/src/ui/Auth.cpp @@ -40,31 +40,13 @@ static const std::string authCookieName {"LmsAuth"}; static void -createAuthToken(Database::IdType userId) +createAuthToken(Database::IdType userId, const Wt::WDateTime& expiry) { - - const std::string secret {Wt::WRandom::generateId(48)}; - const Wt::WDateTime now {Wt::WDateTime::currentDateTime()}; - Wt::WDateTime expiry; - - { - auto transaction {LmsApp->getDbSession().createUniqueTransaction()}; - - Database::User::pointer user {Database::User::getById(LmsApp->getDbSession(), userId)}; - if (!user) - return; - - expiry = user->isDemo() ? now.addDays(7) : now.addYears(1); - Database::AuthToken::create(LmsApp->getDbSession(), secret, expiry, user); - - LMS_LOG(UI, DEBUG) << "Created auth token for user '" << user->getLoginName() << "', expiry = " << expiry.toString(); - - Database::AuthToken::removeExpiredTokens(LmsApp->getDbSession(), now); - } + const std::string secret {getService<::Auth::AuthService>()->createAuthToken(LmsApp->getDbSession(), userId, expiry)}; LmsApp->setCookie(authCookieName, secret, - expiry.toTime_t() - now.toTime_t(), + expiry.toTime_t() - Wt::WDateTime::currentDateTime().toTime_t(), "", "", LmsApp->environment().urlScheme() == "https"); @@ -78,33 +60,20 @@ processAuthToken(const Wt::WEnvironment& env) if (!authCookie) return boost::none; - Database::IdType userId {}; + const auto res {getService<::Auth::AuthService>()->processAuthToken(LmsApp->getDbSession(), boost::asio::ip::address::from_string(env.clientAddress()), *authCookie)}; + switch (res.state) { - auto transaction {LmsApp->getDbSession().createUniqueTransaction()}; - - Database::AuthToken::pointer authToken {Database::AuthToken::getByValue(LmsApp->getDbSession(), *authCookie)}; - if (!authToken) - { - LMS_LOG(UI, INFO) << "Client '" << env.clientAddress() << "' presented a token that has not been found"; + case ::Auth::AuthService::AuthTokenProcessResult::State::NotFound: + case ::Auth::AuthService::AuthTokenProcessResult::State::Throttled: + LmsApp->setCookie(authCookieName, std::string {}, 0, "", "", env.urlScheme() == "https"); return boost::none; - } - if (authToken->getExpiry() < Wt::WDateTime::currentDateTime()) - { - LMS_LOG(UI, INFO) << "Expired auth token for user '" << authToken->getUser()->getLoginName() << "'!"; - authToken.remove(); - return boost::none; - } - - LMS_LOG(UI, DEBUG) << "Found auth token for user '" << authToken->getUser()->getLoginName() << "'!"; - userId = authToken->getUser().id(); - - authToken.remove(); + case ::Auth::AuthService::AuthTokenProcessResult::State::Found: + createAuthToken(res.authTokenInfo->userId, res.authTokenInfo->expiry); + break; } - createAuthToken(userId); - - return userId; + return res.authTokenInfo->userId; } class AuthModel : public Wt::WFormModel @@ -129,16 +98,24 @@ class AuthModel : public Wt::WFormModel void saveData() { + bool isDemo; { auto transaction {LmsApp->getDbSession().createUniqueTransaction()}; Database::User::pointer user {Database::User::getByLoginName(LmsApp->getDbSession(), valueText(LoginNameField).toUTF8())}; user.modify()->setLastLogin(Wt::WDateTime::currentDateTime()); _userId = user.id(); + + const Wt::WDateTime now {Wt::WDateTime::currentDateTime()}; + isDemo = user->isDemo(); } if (Wt::asNumber(value(RememberMeField))) - createAuthToken(*_userId); + { + const Wt::WDateTime now {Wt::WDateTime::currentDateTime()}; + + createAuthToken(*_userId, isDemo ? now.addDays(3) : now.addYears(1)); + } } bool validateField(Field field) diff --git a/src/ui/Auth.hpp b/src/ui/Auth.hpp index 1d94e2f4..3fee4b3f 100644 --- a/src/ui/Auth.hpp +++ b/src/ui/Auth.hpp @@ -27,9 +27,8 @@ namespace UserInterface { - -// If success, returns the authenticated user id -boost::optional processAuthToken(const Wt::WEnvironment& env); +boost::optional +processAuthToken(const Wt::WEnvironment& env); class Auth : public Wt::WTemplateFormView { diff --git a/src/ui/LmsApplication.cpp b/src/ui/LmsApplication.cpp index 80d27985..b5c57b53 100644 --- a/src/ui/LmsApplication.cpp +++ b/src/ui/LmsApplication.cpp @@ -168,7 +168,7 @@ LmsApplication::LmsApplication(const Wt::WEnvironment& env, return; } - auto userId {processAuthToken(env)}; + const auto userId {processAuthToken(env)}; if (userId) { handleUserLoggedIn(*userId);