From a6eb0e2349e3dac7befa021bf3481ecafc545ab1 Mon Sep 17 00:00:00 2001 From: Gent Semaj Date: Tue, 17 Sep 2024 20:41:48 -0700 Subject: [PATCH] Auth Cookie Support (#285) * Auth cookie support * Add config option for auth cookie support * Safe handling of TEGid/auth_id strings * Fix bad size calculation due to pointer cast * Expiration timestamp instead of valid bit * Change setting to "allowed auth methods" This allows plaintext password auth to be disabled altogether * PR feedback --- config.ini | 4 ++ sql/migration4.sql | 19 ++++++ sql/tables.sql | 14 +++- src/core/CNStructs.hpp | 1 + src/db/Database.hpp | 6 +- src/db/login.cpp | 49 ++++++++++++++ src/servers/CNLoginServer.cpp | 116 +++++++++++++++++++++++++--------- src/servers/CNLoginServer.hpp | 9 ++- src/settings.cpp | 2 + src/settings.hpp | 1 + 10 files changed, 185 insertions(+), 36 deletions(-) create mode 100644 sql/migration4.sql diff --git a/config.ini b/config.ini index f6ac838..5247d81 100644 --- a/config.ini +++ b/config.ini @@ -17,6 +17,10 @@ acceptallcustomnames=true # should attempts to log into non-existent accounts # automatically create them? autocreateaccounts=true +# list of supported authentication methods (comma-separated) +# password = allow login type 1 with plaintext passwords +# cookie = allow login type 2 with one-shot auth cookies +authmethods=password # how often should everything be flushed to the database? # the default is 4 minutes dbsaveinterval=240 diff --git a/sql/migration4.sql b/sql/migration4.sql new file mode 100644 index 0000000..e195ce7 --- /dev/null +++ b/sql/migration4.sql @@ -0,0 +1,19 @@ +/* + It is recommended in the SQLite manual to turn off + foreign keys when making schema changes that involve them +*/ +PRAGMA foreign_keys=OFF; +BEGIN TRANSACTION; +-- New table to store auth cookies +CREATE TABLE Auth ( + AccountID INTEGER NOT NULL, + Cookie TEXT NOT NULL, + Expires INTEGER DEFAULT 0 NOT NULL, + FOREIGN KEY(AccountID) REFERENCES Accounts(AccountID) ON DELETE CASCADE, + UNIQUE (AccountID) +); +-- Update DB Version +UPDATE Meta SET Value = 5 WHERE Key = 'DatabaseVersion'; +UPDATE Meta SET Value = strftime('%s', 'now') WHERE Key = 'LastMigration'; +COMMIT; +PRAGMA foreign_keys=ON; \ No newline at end of file diff --git a/sql/tables.sql b/sql/tables.sql index 564e2ac..70b3169 100644 --- a/sql/tables.sql +++ b/sql/tables.sql @@ -143,7 +143,7 @@ CREATE TABLE IF NOT EXISTS EmailItems ( UNIQUE (PlayerID, MsgIndex, Slot) ); -CREATE TABLE IF NOT EXISTS RaceResults( +CREATE TABLE IF NOT EXISTS RaceResults ( EPID INTEGER NOT NULL, PlayerID INTEGER NOT NULL, Score INTEGER NOT NULL, @@ -153,9 +153,17 @@ CREATE TABLE IF NOT EXISTS RaceResults( FOREIGN KEY(PlayerID) REFERENCES Players(PlayerID) ON DELETE CASCADE ); -CREATE TABLE IF NOT EXISTS RedeemedCodes( +CREATE TABLE IF NOT EXISTS RedeemedCodes ( PlayerID INTEGER NOT NULL, Code TEXT NOT NULL, FOREIGN KEY(PlayerID) REFERENCES Players(PlayerID) ON DELETE CASCADE, UNIQUE (PlayerID, Code) -) +); + +CREATE TABLE IF NOT EXISTS Auth ( + AccountID INTEGER NOT NULL, + Cookie TEXT NOT NULL, + Expires INTEGER DEFAULT 0 NOT NULL, + FOREIGN KEY(AccountID) REFERENCES Accounts(AccountID) ON DELETE CASCADE, + UNIQUE (AccountID) +); diff --git a/src/core/CNStructs.hpp b/src/core/CNStructs.hpp index db59f2f..539c89f 100644 --- a/src/core/CNStructs.hpp +++ b/src/core/CNStructs.hpp @@ -40,6 +40,7 @@ // wrapper for U16toU8 #define ARRLEN(x) (sizeof(x)/sizeof(*x)) +#define AUTOU8(x) std::string((char*)x, ARRLEN(x)) #define AUTOU16TOU8(x) U16toU8(x, ARRLEN(x)) // TODO: rewrite U16toU8 & U8toU16 to not use codecvt diff --git a/src/db/Database.hpp b/src/db/Database.hpp index 8883137..3bd1a0a 100644 --- a/src/db/Database.hpp +++ b/src/db/Database.hpp @@ -5,7 +5,7 @@ #include #include -#define DATABASE_VERSION 4 +#define DATABASE_VERSION 5 namespace Database { @@ -53,6 +53,10 @@ namespace Database { void updateAccountLevel(int accountId, int accountLevel); + // return true if cookie is valid for the account. + // invalidates the stored cookie afterwards + bool checkCookie(int accountId, const char *cookie); + // interface for the /ban command bool banPlayer(int playerId, std::string& reason); bool unbanPlayer(int playerId); diff --git a/src/db/login.cpp b/src/db/login.cpp index f3493ba..0d1e198 100644 --- a/src/db/login.cpp +++ b/src/db/login.cpp @@ -98,6 +98,55 @@ void Database::updateAccountLevel(int accountId, int accountLevel) { sqlite3_finalize(stmt); } +bool Database::checkCookie(int accountId, const char *tryCookie) { + std::lock_guard lock(dbCrit); + + const char* sql_get = R"( + SELECT Cookie + FROM Auth + WHERE AccountID = ? AND Expires > ?; + )"; + + const char* sql_invalidate = R"( + UPDATE Auth + SET Expires = 0 + WHERE AccountID = ?; + )"; + + sqlite3_stmt* stmt; + + sqlite3_prepare_v2(db, sql_get, -1, &stmt, NULL); + sqlite3_bind_int(stmt, 1, accountId); + sqlite3_bind_int(stmt, 2, getTimestamp()); + int rc = sqlite3_step(stmt); + if (rc != SQLITE_ROW) { + sqlite3_finalize(stmt); + return false; + } + + const char *cookie = reinterpret_cast(sqlite3_column_text(stmt, 0)); + if (strlen(cookie) != strlen(tryCookie)) { + sqlite3_finalize(stmt); + return false; + } + + /* + * since cookies are immediately invalidated, we don't need to be concerned about + * timing-related side channel attacks, so strcmp is fine here + */ + bool match = (strcmp(cookie, tryCookie) == 0); + sqlite3_finalize(stmt); + + sqlite3_prepare_v2(db, sql_invalidate, -1, &stmt, NULL); + sqlite3_bind_int(stmt, 1, accountId); + rc = sqlite3_step(stmt); + sqlite3_finalize(stmt); + if (rc != SQLITE_DONE) + std::cout << "[WARN] Database fail on checkCookie(): " << sqlite3_errmsg(db) << std::endl; + + return match; +} + void Database::updateSelected(int accountId, int slot) { std::lock_guard lock(dbCrit); diff --git a/src/servers/CNLoginServer.cpp b/src/servers/CNLoginServer.cpp index 427e7bb..3bba720 100644 --- a/src/servers/CNLoginServer.cpp +++ b/src/servers/CNLoginServer.cpp @@ -105,57 +105,95 @@ void loginFail(LoginError errorCode, std::string userLogin, CNSocket* sock) { void CNLoginServer::login(CNSocket* sock, CNPacketData* data) { auto login = (sP_CL2LS_REQ_LOGIN*)data->buf; - // TODO: implement better way of sending credentials - std::string userLogin((char*)login->szCookie_TEGid); - std::string userPassword((char*)login->szCookie_authid); + + std::string userLogin; + std::string userToken; // could be password or auth cookie /* - * Sometimes the client sends garbage cookie data. - * Validate it as normal credentials instead of using a length check before falling back. + * The std::string -> char* -> std::string maneuver should remove any + * trailing garbage after the null terminator. */ - if (!CNLoginServer::isLoginDataGood(userLogin, userPassword)) { - /* - * The std::string -> char* -> std::string maneuver should remove any - * trailing garbage after the null terminator. - */ + if (login->iLoginType == (int32_t)LoginType::COOKIE) { + userLogin = std::string(AUTOU8(login->szCookie_TEGid).c_str()); + userToken = std::string(AUTOU8(login->szCookie_authid).c_str()); + } else { userLogin = std::string(AUTOU16TOU8(login->szID).c_str()); - userPassword = std::string(AUTOU16TOU8(login->szPassword).c_str()); + userToken = std::string(AUTOU16TOU8(login->szPassword).c_str()); } - // the client inserts a "\n" in the password if you press enter key in the middle of the password - // (not at the start or the end of the password field) - if (int(userPassword.find("\n")) > 0) - userPassword.erase(userPassword.find("\n"), 1); - - // check regex - if (!CNLoginServer::isLoginDataGood(userLogin, userPassword)) { + // check username regex + if (!CNLoginServer::isUsernameGood(userLogin)) { // send a custom error message INITSTRUCT(sP_FE2CL_GM_REP_PC_ANNOUNCE, msg); - std::string text = "Invalid login or password\n"; - text += "Login has to be 4 - 32 characters long and can't contain special characters other than dash and underscore\n"; - text += "Password has to be 8 - 32 characters long"; + std::string text = "Invalid login\n"; + text += "Login has to be 4 - 32 characters long and can't contain special characters other than dash and underscore"; U8toU16(text, msg.szAnnounceMsg, sizeof(msg.szAnnounceMsg)); - msg.iDuringTime = 15; + msg.iDuringTime = 10; sock->sendPacket(msg, P_FE2CL_GM_REP_PC_ANNOUNCE); // we still have to send login fail to prevent softlock return loginFail(LoginError::LOGIN_ERROR, userLogin, sock); } - + + // we only interpret the token as a cookie if cookie login was used and it's allowed. + // otherwise we interpret it as a password, and this maintains compatibility with + // the auto-login trick used on older clients + bool isCookieAuth = login->iLoginType == (int32_t)LoginType::COOKIE + && CNLoginServer::isLoginTypeAllowed(LoginType::COOKIE); + + // password login checks + if (!isCookieAuth) { + // bail if password auth isn't allowed + if (!CNLoginServer::isLoginTypeAllowed(LoginType::PASSWORD)) { + // send a custom error message + INITSTRUCT(sP_FE2CL_GM_REP_PC_ANNOUNCE, msg); + std::string text = "Password login disabled\n"; + text += "This server has disabled logging in with plaintext passwords.\n"; + text += "Please contact an admin for assistance."; + U8toU16(text, msg.szAnnounceMsg, sizeof(msg.szAnnounceMsg)); + msg.iDuringTime = 12; + sock->sendPacket(msg, P_FE2CL_GM_REP_PC_ANNOUNCE); + + // we still have to send login fail to prevent softlock + return loginFail(LoginError::LOGIN_ERROR, userLogin, sock); + } + + // check regex + if (!CNLoginServer::isPasswordGood(userToken)) { + // send a custom error message + INITSTRUCT(sP_FE2CL_GM_REP_PC_ANNOUNCE, msg); + std::string text = "Invalid password\n"; + text += "Password has to be 8 - 32 characters long"; + U8toU16(text, msg.szAnnounceMsg, sizeof(msg.szAnnounceMsg)); + msg.iDuringTime = 10; + sock->sendPacket(msg, P_FE2CL_GM_REP_PC_ANNOUNCE); + + // we still have to send login fail to prevent softlock + return loginFail(LoginError::LOGIN_ERROR, userLogin, sock); + } + } Database::Account findUser = {}; Database::findAccount(&findUser, userLogin); // account was not found if (findUser.AccountID == 0) { - if (settings::AUTOCREATEACCOUNTS) - return newAccount(sock, userLogin, userPassword, login->iClientVerC); + // don't auto-create an account if it's a cookie auth for whatever reason + if (settings::AUTOCREATEACCOUNTS && !isCookieAuth) + return newAccount(sock, userLogin, userToken, login->iClientVerC); return loginFail(LoginError::ID_DOESNT_EXIST, userLogin, sock); } - if (!CNLoginServer::isPasswordCorrect(findUser.Password, userPassword)) - return loginFail(LoginError::ID_AND_PASSWORD_DO_NOT_MATCH, userLogin, sock); + if (isCookieAuth) { + const char *cookie = userToken.c_str(); + if (!Database::checkCookie(findUser.AccountID, cookie)) + return loginFail(LoginError::ID_AND_PASSWORD_DO_NOT_MATCH, userLogin, sock); + } else { + // simple password check + if (!CNLoginServer::isPasswordCorrect(findUser.Password, userToken)) + return loginFail(LoginError::ID_AND_PASSWORD_DO_NOT_MATCH, userLogin, sock); + } // is the account banned if (findUser.BannedUntil > getTimestamp()) { @@ -621,11 +659,14 @@ bool CNLoginServer::exitDuplicate(int accountId) { return false; } -bool CNLoginServer::isLoginDataGood(std::string login, std::string password) { - std::regex loginRegex("[a-zA-Z0-9_-]{4,32}"); - std::regex passwordRegex("[a-zA-Z0-9!@#$%^&*()_+]{8,32}"); +bool CNLoginServer::isUsernameGood(std::string login) { + const std::regex loginRegex("[a-zA-Z0-9_-]{4,32}"); + return (std::regex_match(login, loginRegex)); +} - return (std::regex_match(login, loginRegex) && std::regex_match(password, passwordRegex)); +bool CNLoginServer::isPasswordGood(std::string password) { + const std::regex passwordRegex("[a-zA-Z0-9!@#$%^&*()_+]{8,32}"); + return (std::regex_match(password, passwordRegex)); } bool CNLoginServer::isPasswordCorrect(std::string actualPassword, std::string tryPassword) { @@ -638,4 +679,17 @@ bool CNLoginServer::isCharacterNameGood(std::string Firstname, std::string Lastn std::regex lastnamecheck(R"(((?! )(?!\.)[a-zA-Z0-9]*\.{0,1}(?!\.+ +)[a-zA-Z0-9]* {0,1}(?! +))*$)"); return (std::regex_match(Firstname, firstnamecheck) && std::regex_match(Lastname, lastnamecheck)); } + +bool CNLoginServer::isLoginTypeAllowed(LoginType loginType) { + // the config file specifies "comma-separated" but tbh we don't care + switch (loginType) { + case LoginType::PASSWORD: + return settings::AUTHMETHODS.find("password") != std::string::npos; + case LoginType::COOKIE: + return settings::AUTHMETHODS.find("cookie") != std::string::npos; + default: + break; + } + return false; +} #pragma endregion diff --git a/src/servers/CNLoginServer.hpp b/src/servers/CNLoginServer.hpp index 9b13829..4e392cb 100644 --- a/src/servers/CNLoginServer.hpp +++ b/src/servers/CNLoginServer.hpp @@ -23,6 +23,11 @@ enum class LoginError { UPDATED_EUALA_REQUIRED = 9 }; +enum class LoginType { + PASSWORD = 1, + COOKIE = 2 +}; + // WARNING: THERE CAN ONLY BE ONE OF THESE SERVERS AT A TIME!!!!!! TODO: change loginSessions & packet handlers to be non-static class CNLoginServer : public CNServer { private: @@ -39,10 +44,12 @@ private: static void changeName(CNSocket* sock, CNPacketData* data); static void duplicateExit(CNSocket* sock, CNPacketData* data); - static bool isLoginDataGood(std::string login, std::string password); + static bool isUsernameGood(std::string login); + static bool isPasswordGood(std::string password); static bool isPasswordCorrect(std::string actualPassword, std::string tryPassword); static bool isAccountInUse(int accountId); static bool isCharacterNameGood(std::string Firstname, std::string Lastname); + static bool isLoginTypeAllowed(LoginType loginType); static void newAccount(CNSocket* sock, std::string userLogin, std::string userPassword, int32_t clientVerC); // returns true if success static bool exitDuplicate(int accountId); diff --git a/src/settings.cpp b/src/settings.cpp index 5949f52..9815a40 100644 --- a/src/settings.cpp +++ b/src/settings.cpp @@ -13,6 +13,7 @@ bool settings::SANDBOX = true; int settings::LOGINPORT = 23000; bool settings::APPROVEALLNAMES = true; bool settings::AUTOCREATEACCOUNTS = true; +std::string settings::AUTHMETHODS = "password"; int settings::DBSAVEINTERVAL = 240; int settings::SHARDPORT = 23001; @@ -87,6 +88,7 @@ void settings::init() { LOGINPORT = reader.GetInteger("login", "port", LOGINPORT); APPROVEALLNAMES = reader.GetBoolean("login", "acceptallcustomnames", APPROVEALLNAMES); AUTOCREATEACCOUNTS = reader.GetBoolean("login", "autocreateaccounts", AUTOCREATEACCOUNTS); + AUTHMETHODS = reader.Get("login", "authmethods", AUTHMETHODS); DBSAVEINTERVAL = reader.GetInteger("login", "dbsaveinterval", DBSAVEINTERVAL); SHARDPORT = reader.GetInteger("shard", "port", SHARDPORT); SHARDSERVERIP = reader.Get("shard", "ip", SHARDSERVERIP); diff --git a/src/settings.hpp b/src/settings.hpp index 85fad68..302c4f6 100644 --- a/src/settings.hpp +++ b/src/settings.hpp @@ -8,6 +8,7 @@ namespace settings { extern int LOGINPORT; extern bool APPROVEALLNAMES; extern bool AUTOCREATEACCOUNTS; + extern std::string AUTHMETHODS; extern int DBSAVEINTERVAL; extern int SHARDPORT; extern std::string SHARDSERVERIP;