PR feedback

This allows plaintext password auth to be disabled altogether

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

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;

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)
+);

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

src/db/Database.hpp

@@ -5,7 +5,7 @@
 #include <string>
 #include <vector>
 
-#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);

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<std::mutex> 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<const char*>(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<std::mutex> lock(dbCrit);
 

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;
-     * Sometimes the client sends garbage cookie data.
+    std::string userToken; // could be password or auth cookie
-     * Validate it as normal credentials instead of using a length check before falling back.
+
-     */
-    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
+    // check username regex
-    // (not at the start or the end of the password field)
+    if (!CNLoginServer::isUsernameGood(userLogin)) {
-    if (int(userPassword.find("\n")) > 0)
-        userPassword.erase(userPassword.find("\n"), 1);
-
-    // check regex
-    if (!CNLoginServer::isLoginDataGood(userLogin, userPassword)) {
         // send a custom error message
         INITSTRUCT(sP_FE2CL_GM_REP_PC_ANNOUNCE, msg);
-        std::string text = "Invalid login or password\n";
+        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\n";
+        text += "Login has to be 4 - 32 characters long and can't contain special characters other than dash and underscore";
-        text += "Password has to be 8 - 32 characters long";          
         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)
+        // don't auto-create an account if it's a cookie auth for whatever reason
-            return newAccount(sock, userLogin, userPassword, login->iClientVerC);
+        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))
+    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) {
+bool CNLoginServer::isUsernameGood(std::string login) {
-    std::regex loginRegex("[a-zA-Z0-9_-]{4,32}");
+    const std::regex loginRegex("[a-zA-Z0-9_-]{4,32}");
-    std::regex passwordRegex("[a-zA-Z0-9!@#$%^&*()_+]{8,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

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

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

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;