Refactor login packet handler for more flexible auth (#298)

This PR enables auth cookies to be used simultaneously with plaintext paasswords sent in the cookie authID field.

* Hoist a bunch of checks from the login packet handler into helper functions.
* Rename the LoginType enum to AuthMethod and distinguish it from the iLoginType packet field (see comment in code for why these should be decoupled).
* If the provided token does not pass the cookie check and password auth is enabled, treat it as a plaintext password and authenticate if it is correct.

Author: yungcomputerchair

config.ini

@@ -18,8 +18,8 @@ acceptallcustomnames=true
 # 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
+# password = allow logging in with plaintext passwords
+# cookie = allow logging in with one-shot auth cookies
 authmethods=password
 # how often should everything be flushed to the database?
 # the default is 4 minutes

src/core/CNStructs.hpp

@@ -49,6 +49,7 @@ std::string U16toU8(char16_t* src, size_t max);
 size_t U8toU16(std::string src, char16_t* des, size_t max); // returns number of char16_t that was written at des
 time_t getTime();
 time_t getTimestamp();
+int timingSafeStrcmp(const char* a, const char* b);
 void terminate(int);
 
 // The PROTOCOL_VERSION definition can be defined by the build system.

src/db/login.cpp

@@ -1,3 +1,5 @@
+#include "core/CNStructs.hpp"
+
 #include "db/internal.hpp"
 
 #include "bcrypt/BCrypt.hpp"
@@ -130,19 +132,21 @@ bool Database::checkCookie(int accountId, const char *tryCookie) {
         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);
+    bool match = (timingSafeStrcmp(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;
+    /*
+     * Only invalidate the cookie if it was correct. This prevents
+     * replay attacks without enabling DOS attacks on accounts.
+     */
+    if (match) {
+        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;
 }

src/main.cpp

@@ -222,6 +222,17 @@ time_t getTimestamp() {
     return (time_t)value.count();
 }
 
+// timing safe strcmp implementation for e.g. cookie validation
+int timingSafeStrcmp(const char* a, const char* b) {
+    int diff = 0;
+    while (*a && *b) {
+        diff |= *a++ ^ *b++;
+    }
+    diff |= *a;
+    diff |= *b;
+    return diff;
+}
+
 // convert integer timestamp (in s) to FF systime struct
 sSYSTEMTIME timeStampToStruct(uint64_t time) {
 

src/servers/CNLoginServer.cpp

@@ -109,111 +109,59 @@ void CNLoginServer::login(CNSocket* sock, CNPacketData* data) {
     std::string userLogin;
     std::string userToken; // could be password or auth cookie
 
+    /*
+     * In this context, "cookie auth" just means the credentials were sent
+     * in the szCookie fields instead of szID and szPassword.
+     */
+    bool isCookieAuth = login->iLoginType == USE_COOKIE_FIELDS;
+
     /*
      * The std::string -> char* -> std::string maneuver should remove any
      * trailing garbage after the null terminator.
      */
-    if (login->iLoginType == (int32_t)LoginType::COOKIE) {
+    if (isCookieAuth) {
         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());
         userToken = std::string(AUTOU16TOU8(login->szPassword).c_str());
     }
 
-    // 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\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 = 10;
-        sock->sendPacket(msg, P_FE2CL_GM_REP_PC_ANNOUNCE);
-
-        // we still have to send login fail to prevent softlock
+    if (!CNLoginServer::checkUsername(sock, userLogin)) {
         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
+        // password was sent in plaintext
+        if (!CNLoginServer::checkPassword(sock, userToken)) {
             return loginFail(LoginError::LOGIN_ERROR, userLogin, sock);
         }
     }
 
     Database::Account findUser = {};
     Database::findAccount(&findUser, userLogin);
-    
+
     // account was not found
     if (findUser.AccountID == 0) {
-        // don't auto-create an account if it's a cookie auth for whatever reason
-        if (settings::AUTOCREATEACCOUNTS && !isCookieAuth)
+        /*
+         * Don't auto-create accounts if it's a cookie login.
+         * It'll either be a bad cookie or a plaintext password sent by auto-login;
+         * either way, we only want to allow auto-creation if the user explicitly entered their credentials.
+         */
+        if (settings::AUTOCREATEACCOUNTS && !isCookieAuth) {
             return newAccount(sock, userLogin, userToken, login->iClientVerC);
+        }
 
         return loginFail(LoginError::ID_DOESNT_EXIST, 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);
+    // make sure either a valid cookie or password was sent
+    if (!CNLoginServer::checkToken(sock, findUser, userToken, isCookieAuth)) {
+        return loginFail(LoginError::ID_AND_PASSWORD_DO_NOT_MATCH, userLogin, sock);
     }
 
-    // is the account banned
-    if (findUser.BannedUntil > getTimestamp()) {
-        // send a custom error message
-        INITSTRUCT(sP_FE2CL_GM_REP_PC_ANNOUNCE, msg);
-
-        // ceiling devision
-        int64_t remainingDays = (findUser.BannedUntil-getTimestamp()) / 86400 + ((findUser.BannedUntil - getTimestamp()) % 86400 != 0);
-
-        std::string text = "Your account has been banned. \nReason: ";
-        text += findUser.BanReason;
-        text += "\nBan expires in " + std::to_string(remainingDays) + " day";
-        if (remainingDays > 1)
-            text += "s";
-
-        U8toU16(text, msg.szAnnounceMsg, sizeof(msg.szAnnounceMsg));
-        msg.iDuringTime = 99999999;
-        sock->sendPacket(msg, P_FE2CL_GM_REP_PC_ANNOUNCE);
-        // don't send fail packet
-        return;
+    if (CNLoginServer::checkBan(sock, findUser)) {
+        return; // don't send fail packet
     }
 
     /* 
@@ -659,12 +607,12 @@ bool CNLoginServer::exitDuplicate(int accountId) {
     return false;
 }
 
-bool CNLoginServer::isUsernameGood(std::string login) {
+bool CNLoginServer::isUsernameGood(std::string& login) {
     const std::regex loginRegex("[a-zA-Z0-9_-]{4,32}");
     return (std::regex_match(login, loginRegex));
 }
 
-bool CNLoginServer::isPasswordGood(std::string password) {
+bool CNLoginServer::isPasswordGood(std::string& password) {
     const std::regex passwordRegex("[a-zA-Z0-9!@#$%^&*()_+]{8,32}");
     return (std::regex_match(password, passwordRegex));
 }
@@ -680,16 +628,107 @@ bool CNLoginServer::isCharacterNameGood(std::string Firstname, std::string Lastn
     return (std::regex_match(Firstname, firstnamecheck) && std::regex_match(Lastname, lastnamecheck));
 }
 
-bool CNLoginServer::isLoginTypeAllowed(LoginType loginType) {
+bool CNLoginServer::isAuthMethodAllowed(AuthMethod authMethod) {
     // the config file specifies "comma-separated" but tbh we don't care
-    switch (loginType) {
-    case LoginType::PASSWORD:
+    switch (authMethod) {
+    case AuthMethod::PASSWORD:
         return settings::AUTHMETHODS.find("password") != std::string::npos;
-    case LoginType::COOKIE:
+    case AuthMethod::COOKIE:
         return settings::AUTHMETHODS.find("cookie") != std::string::npos;
     default:
         break;
     }
     return false;
 }
+
+bool CNLoginServer::checkPassword(CNSocket* sock, std::string& password) {
+    // check password auth allowed
+    if (!CNLoginServer::isAuthMethodAllowed(AuthMethod::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);
+
+        return false;
+    }
+
+    // check regex
+    if (!CNLoginServer::isPasswordGood(password)) {
+        // 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);
+
+        return false;
+    }
+
+    return true;
+}
+
+bool CNLoginServer::checkUsername(CNSocket* sock, std::string& username) {
+    // check username regex
+    if (!CNLoginServer::isUsernameGood(username)) {
+        // send a custom error message
+        INITSTRUCT(sP_FE2CL_GM_REP_PC_ANNOUNCE, msg);
+        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 = 10;
+        sock->sendPacket(msg, P_FE2CL_GM_REP_PC_ANNOUNCE);
+
+        return false;
+    }
+
+    return true;
+}
+
+bool CNLoginServer::checkToken(CNSocket* sock, Database::Account& account, std::string& token, bool isCookieAuth) {
+    // check for valid cookie first
+    if (isCookieAuth && CNLoginServer::isAuthMethodAllowed(AuthMethod::COOKIE)) {
+        const char *cookie = token.c_str();
+        if (Database::checkCookie(account.AccountID, cookie)) {
+            return true;
+        }
+    }
+
+    // cookie check disabled or failed; check to see if it's a plaintext password
+    if (CNLoginServer::isAuthMethodAllowed(AuthMethod::PASSWORD)
+        && CNLoginServer::isPasswordCorrect(account.Password, token)) {
+        return true;
+    }
+
+    return false;
+}
+
+bool CNLoginServer::checkBan(CNSocket* sock, Database::Account& account) {
+    // check if the account is banned
+    if (account.BannedUntil > getTimestamp()) {
+        // send a custom error message
+        INITSTRUCT(sP_FE2CL_GM_REP_PC_ANNOUNCE, msg);
+
+        // ceiling devision
+        int64_t remainingDays = (account.BannedUntil-getTimestamp()) / 86400 + ((account.BannedUntil - getTimestamp()) % 86400 != 0);
+
+        std::string text = "Your account has been banned. \nReason: ";
+        text += account.BanReason;
+        text += "\nBan expires in " + std::to_string(remainingDays) + " day";
+        if (remainingDays > 1)
+            text += "s";
+
+        U8toU16(text, msg.szAnnounceMsg, sizeof(msg.szAnnounceMsg));
+        msg.iDuringTime = 99999999;
+        sock->sendPacket(msg, P_FE2CL_GM_REP_PC_ANNOUNCE);
+
+        return true;
+    }
+
+    return false;
+}
 #pragma endregion

src/servers/CNLoginServer.hpp

@@ -1,6 +1,7 @@
 #pragma once
 
 #include "core/Core.hpp"
+#include "db/Database.hpp"
 
 #include "Player.hpp"
 
@@ -23,7 +24,9 @@ enum class LoginError {
     UPDATED_EUALA_REQUIRED = 9
 };
 
-enum class LoginType {
+#define USE_COOKIE_FIELDS 2
+
+enum class AuthMethod {
     PASSWORD = 1,
     COOKIE = 2
 };
@@ -44,12 +47,17 @@ class CNLoginServer : public CNServer {
     static void changeName(CNSocket* sock, CNPacketData* data);
     static void duplicateExit(CNSocket* sock, CNPacketData* data);
 
-    static bool isUsernameGood(std::string login);
-    static bool isPasswordGood(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 bool isAuthMethodAllowed(AuthMethod authMethod);
+    static bool checkUsername(CNSocket* sock, std::string& username);
+    static bool checkPassword(CNSocket* sock, std::string& password);
+    static bool checkToken(CNSocket* sock, Database::Account& account, std::string& token, bool isCookieAuth);
+    static bool checkBan(CNSocket* sock, Database::Account& account);
+
     static void newAccount(CNSocket* sock, std::string userLogin, std::string userPassword, int32_t clientVerC);
     // returns true if success
     static bool exitDuplicate(int accountId);