Apply suggestions from code review

Co-authored-by: Lion <development@kortlepel.com>

CMakeLists.txt

@@ -28,6 +28,8 @@ set(SENTRY_BUILD_SHARED_LIBS OFF)
 
 add_compile_definitions(CPPHTTPLIB_OPENSSL_SUPPORT=1)
 
+option(WIN32_STATIC_RUNTIME "Build statically-linked runtime on windows (don't touch unless you know what you're doing)" ON)
+
 # ------------------------ APPLE ---------------------------------
 if(APPLE)
     if(IS_DIRECTORY /opt/homebrew/Cellar/lua@5.3/5.3.6)
@@ -46,18 +48,14 @@ if(APPLE)
         link_directories(/usr/local/opt/openssl@1.1/lib)
     endif()
 # ------------------------ WINDOWS ---------------------------------
-option(WIN32_STATIC_RUNTIME "Build statically-linked runtime on windows (don't touch unless you know what you're doing)" ON)
 elseif (WIN32)
-    # this has to happen before sentry, so that crashpad on windows links with these settings.
     if (WIN32_STATIC_RUNTIME)
-        message(STATUS "MSVC -> forcing use of statically-linked runtime.")
-        STRING(REPLACE "/MD" "/MT" CMAKE_CXX_FLAGS_RELEASE ${CMAKE_CXX_FLAGS_RELEASE})
-        STRING(REPLACE "/MDd" "/MTd" CMAKE_CXX_FLAGS_DEBUG ${CMAKE_CXX_FLAGS_DEBUG})
+        set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
     endif()
-# ------------------------ LINUX ---------------------------------
+# ------------------------ UNIX ------------------------------------
 elseif (UNIX)
     set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -O0 -g")
-    set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -O2 -fno-builtin")
+    set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -O2")
     option(SANITIZE "Turns on thread and UB sanitizers" OFF)
     if (SANITIZE)
         message(STATUS "sanitize is ON")
@@ -82,9 +80,6 @@ add_subdirectory("deps/sentry-native")
 
 # ------------------------ C++ SETUP ---------------------------------
 set(CMAKE_CXX_STANDARD 17)
-if (MSVC)
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj")
-endif ()
 
 # ------------------------ DEPENDENCIES ------------------------------
 message(STATUS "Adding local source dependencies")
@@ -177,6 +172,12 @@ if (UNIX)
         -fstack-protector
         -Wzero-as-null-pointer-constant
     )
+else()
+
+    set(BeamMP_CompileOptions
+        /bigobj
+        /INCREMENTAL:NO /NODEFAULTLIB:MSVCRT /NODEFAULTLIB:LIBCMT
+    )
 endif()
 
 set(BeamMP_Libraries

Changelog.md

@@ -1,3 +1,10 @@
+# v3.1.1
+
+- FIXED bug which caused GetPlayerIdentifiers, GetPlayerName, etc not to work in `onPlayerDisconnect`
+- FIXED some issues which could cause the server to crash when receiving malformed data
+- FIXED a bug which caused a server to crash during authentication when receiving malformed data
+- FIXED minor vulnerability in chat message handling
+- FIXED a minor formatting bug in the `status` command
 
 # v3.1.0
 

README.md

@@ -107,11 +107,6 @@ curl
 ##### Build Dependencies
 These are needed for you to *build* the server, in addition to the [runtime dependencies](#runtime-dependencies).
 
-**Ubuntu 22.04**
-```
-
-```
-
 <details>
 <summary>
 Ubuntu 22.04
@@ -122,7 +117,7 @@ Ubuntu 22.04
 git
 libz-dev
 rapidjson-dev
-liblua5.3
+liblua5.3-dev
 libssl-dev
 libwebsocketpp-dev
 libcurl4-openssl-dev
@@ -147,7 +142,6 @@ curl
 git
 cmake
 g++
-cmake
 zlib
 boost
 websocketpp

include/Common.h

@@ -49,6 +49,7 @@ public:
         std::string Resource { "Resources" };
         std::string MapName { "/levels/gridmap_v2/info.json" };
         std::string Key {};
+        std::string Password{};
         std::string SSLKeyPath { "./.ssl/HttpServer/key.pem" };
         std::string SSLCertPath { "./.ssl/HttpServer/cert.pem" };
         bool HTTPServerEnabled { false };
@@ -137,7 +138,7 @@ private:
     static inline std::mutex mShutdownHandlersMutex {};
     static inline std::deque<TShutdownHandler> mShutdownHandlers {};
 
-    static inline Version mVersion { 3, 1, 0 };
+    static inline Version mVersion { 3, 1, 1 };
 };
 
 std::string ThreadName(bool DebugModeOverride = false);

include/TLuaEngine.h

@@ -76,7 +76,7 @@ public:
     };
 
     TLuaEngine();
-    ~TLuaEngine() noexcept {
+    virtual ~TLuaEngine() noexcept {
         beammp_debug("Lua Engine terminated");
     }
 
@@ -198,7 +198,7 @@ private:
     public:
         StateThreadData(const std::string& Name, TLuaStateId StateId, TLuaEngine& Engine);
         StateThreadData(const StateThreadData&) = delete;
-        ~StateThreadData() noexcept { beammp_debug("\"" + mStateId + "\" destroyed"); }
+        virtual ~StateThreadData() noexcept { beammp_debug("\"" + mStateId + "\" destroyed"); }
         [[nodiscard]] std::shared_ptr<TLuaResult> EnqueueScript(const TLuaChunk& Script);
         [[nodiscard]] std::shared_ptr<TLuaResult> EnqueueFunctionCall(const std::string& FunctionName, const std::vector<TLuaArgTypes>& Args);
         [[nodiscard]] std::shared_ptr<TLuaResult> EnqueueFunctionCallFromCustomEvent(const std::string& FunctionName, const std::vector<TLuaArgTypes>& Args, const std::string& EventName, CallStrategy Strategy);

include/TNetwork.h

@@ -52,4 +52,5 @@ private:
     static const uint8_t* SendSplit(TClient& c, ip::tcp::socket& Socket, const uint8_t* DataPtr, size_t Size);
 };
 
+std::string HashPassword(const std::string& str);
 std::vector<uint8_t> StringToVector(const std::string& Str);

src/Client.cpp

@@ -6,6 +6,7 @@
 #include <optional>
 
 void TClient::DeleteCar(int Ident) {
+    // TODO: Send delete packets
     std::unique_lock lock(mVehicleDataMutex);
     auto iter = std::find_if(mVehicleData.begin(), mVehicleData.end(), [&](auto& elem) {
         return Ident == elem.ID();

src/TConfig.cpp

@@ -18,6 +18,7 @@ static constexpr std::string_view StrDescription = "Description";
 static constexpr std::string_view StrResourceFolder = "ResourceFolder";
 static constexpr std::string_view StrAuthKey = "AuthKey";
 static constexpr std::string_view StrLogChat = "LogChat";
+static constexpr std::string_view StrPassword = "Password";
 
 // Misc
 static constexpr std::string_view StrSendErrors = "SendErrors";
@@ -106,6 +107,8 @@ void TConfig::FlushToFile() {
     data["General"][StrMap.data()] = Application::Settings.MapName;
     data["General"][StrDescription.data()] = Application::Settings.ServerDesc;
     data["General"][StrResourceFolder.data()] = Application::Settings.Resource;
+    data["General"][StrPassword.data()] = Application::Settings.Password;
+    SetComment(data["General"][StrPassword.data()].comments(), " Sets a password on this server, which restricts people from joining. To join, a player must enter this exact password. Leave empty ("") to disable the password.");
     // Misc
     data["Misc"][StrHideUpdateMessages.data()] = Application::Settings.HideUpdateMessages;
     SetComment(data["Misc"][StrHideUpdateMessages.data()].comments(), " Hides the periodic update message which notifies you of a new server version. You should really keep this on and always update as soon as possible. For more information visit https://wiki.beammp.com/en/home/server-maintenance#updating-the-server. An update message will always appear at startup regardless.");
@@ -189,6 +192,7 @@ void TConfig::ParseFromFile(std::string_view name) {
         TryReadValue(data, "General", StrResourceFolder, Application::Settings.Resource);
         TryReadValue(data, "General", StrAuthKey, Application::Settings.Key);
         TryReadValue(data, "General", StrLogChat, Application::Settings.LogChat);
+        TryReadValue(data, "General", StrPassword, Application::Settings.Password);
         // Misc
         TryReadValue(data, "Misc", StrSendErrors, Application::Settings.SendErrors);
         TryReadValue(data, "Misc", StrHideUpdateMessages, Application::Settings.HideUpdateMessages);
@@ -240,6 +244,7 @@ void TConfig::PrintDebug() {
     beammp_debug(std::string(StrHTTPServerIP) + ": \"" + Application::Settings.HTTPServerIP + "\"");
     // special!
     beammp_debug("Key Length: " + std::to_string(Application::Settings.Key.length()) + "");
+    beammp_debug("Password Protected: " + std::string(Application::Settings.Password.empty() ? "false" : "true"));
 }
 
 void TConfig::ParseOldFormat() {

src/TConsole.cpp

@@ -198,10 +198,10 @@ bool TConsole::EnsureArgsCount(const std::vector<std::string>& args, size_t min,
         return EnsureArgsCount(args, min);
     } else {
         if (args.size() > max) {
-            Application::Console().WriteRaw("Too many arguments. At most " + std::to_string(max) + " arguments expected, got " + std::to_string(args.size()) + " instead.");
+            Application::Console().WriteRaw("Too many arguments. At most " + std::to_string(max) + " argument(s) expected, got " + std::to_string(args.size()) + " instead.");
             return false;
         } else if (args.size() < min) {
-            Application::Console().WriteRaw("Too few arguments. At least " + std::to_string(max) + " arguments expected, got " + std::to_string(args.size()) + " instead.");
+            Application::Console().WriteRaw("Too few arguments. At least " + std::to_string(min) + " argument(s) expected, got " + std::to_string(args.size()) + " instead.");
             return false;
         }
     }
@@ -350,9 +350,10 @@ std::tuple<std::string, std::vector<std::string>> TConsole::ParseCommand(const s
 }
 
 void TConsole::Command_Settings(const std::string&, const std::vector<std::string>& args) {
-    if (!EnsureArgsCount(args, 0)) {
+    if (!EnsureArgsCount(args, 1, 2)) {
         return;
     }
+    
 }
 
 void TConsole::Command_Say(const std::string& FullCmd) {
@@ -478,12 +479,12 @@ void TConsole::Command_Status(const std::string&, const std::vector<std::string>
            << "\t\tEvent handlers:              " << mLuaEngine->GetRegisteredEventHandlerCount() << "\n"
            << "\tSubsystems:\n"
            << "\t\tGood/Starting/Bad:           " << SystemsGood << "/" << SystemsStarting << "/" << SystemsBad << "\n"
-           << "\t\tShutting down/Shut down:      " << SystemsShuttingDown << "/" << SystemsShutdown << "\n"
+           << "\t\tShutting down/Shut down:     " << SystemsShuttingDown << "/" << SystemsShutdown << "\n"
            << "\t\tGood:                        [ " << SystemsGoodList << " ]\n"
            << "\t\tStarting:                    [ " << SystemsStartingList << " ]\n"
            << "\t\tBad:                         [ " << SystemsBadList << " ]\n"
            << "\t\tShutting down:               [ " << SystemsShuttingDownList << " ]\n"
-           << "\t\tShut down:                    [ " << SystemsShutdownList << " ]\n"
+           << "\t\tShut down:                   [ " << SystemsShutdownList << " ]\n"
            << "";
 
     Application::Console().WriteRaw(Status.str());

src/THeartbeatThread.cpp

@@ -149,7 +149,8 @@ std::string THeartbeatThread::GenerateCall() {
         << "&modstotalsize=" << mResourceManager.MaxModSize()
         << "&modstotal=" << mResourceManager.ModsLoaded()
         << "&playerslist=" << GetPlayers()
-        << "&desc=" << Application::Settings.ServerDesc;
+        << "&desc=" << Application::Settings.ServerDesc
+        << "&pass=" << (Application::Settings.Password.empty() ? "false" : "true");
     return Ret.str();
 }
 THeartbeatThread::THeartbeatThread(TResourceManager& ResourceManager, TServer& Server)

src/TNetwork.cpp

@@ -11,6 +11,8 @@
 #include <boost/asio/ip/address_v4.hpp>
 #include <cstring>
 
+typedef boost::asio::detail::socket_option::integer<SOL_SOCKET, SO_RCVTIMEO> rcv_timeout_option;
+
 std::vector<uint8_t> StringToVector(const std::string& Str) {
     return std::vector<uint8_t>(Str.data(), Str.data() + Str.size());
 }
@@ -151,6 +153,7 @@ void TNetwork::TCPServerMain() {
             if (ec) {
                 beammp_errorf("failed to accept: {}", ec.message());
             }
+            ClientSocket.set_option(rcv_timeout_option{ 120000 }); //timeout of 120seconds
             TConnection Conn { std::move(ClientSocket), ClientEp };
             std::thread ID(&TNetwork::Identify, this, std::move(Conn));
             ID.detach(); // TODO: Add to a queue and attempt to join periodically
@@ -212,6 +215,15 @@ void TNetwork::HandleDownload(TConnection&& Conn) {
     });
 }
 
+std::string HashPassword(const std::string& str) {
+    std::stringstream ret;
+    unsigned char* hash = SHA256(reinterpret_cast<const unsigned char*>(str.c_str()), str.length(), nullptr);
+    for (int i = 0; i < 32; i++) {
+        ret << std::hex << static_cast<int>(hash[i]);
+    }
+    return ret.str();
+}
+
 std::shared_ptr<TClient> TNetwork::Authentication(TConnection&& RawConnection) {
     auto Client = CreateClient(std::move(RawConnection.Socket));
     Client->SetIdentifier("ip", RawConnection.SockAddr.address().to_string());
@@ -235,7 +247,8 @@ std::shared_ptr<TClient> TNetwork::Authentication(TConnection&& RawConnection) {
         ClientKick(*Client, fmt::format("Invalid version header: '{}' ({})", std::string(reinterpret_cast<const char*>(Data.data()), Data.size()), Data.size()));
         return nullptr;
     }
-    if (!TCPSend(*Client, StringToVector("S"))) {
+
+    if (!TCPSend(*Client, StringToVector("A"))) { //changed to A for Accepted version
         // TODO: handle
     }
 
@@ -279,6 +292,22 @@ std::shared_ptr<TClient> TNetwork::Authentication(TConnection&& RawConnection) {
         return nullptr;
     }
 
+    if(!Application::Settings.Password.empty()) { // ask password
+        if(!TCPSend(*Client, StringToVector("S"))) {
+            // TODO: handle
+        }
+        beammp_info("Waiting for password");
+        Data = TCPRcv(*Client);
+        std::string Pass = std::string(reinterpret_cast<const char*>(Data.data()), Data.size());
+        if(Pass != HashPassword(Application::Settings.Password)) {
+            beammp_debug(Client->GetName() + " attempted to connect with a wrong password");
+            ClientKick(*Client, "Wrong password!");
+            return {};
+        } else {
+            beammp_debug(Client->GetName() + " used the correct password");
+        }
+    }
+
     beammp_debug("Name -> " + Client->GetName() + ", Guest -> " + std::to_string(Client->IsGuest()) + ", Roles -> " + Client->GetRoles());
     mServer.ForEachClient([&](const std::weak_ptr<TClient>& ClientPtr) -> bool {
         std::shared_ptr<TClient> Cl;
@@ -328,6 +357,7 @@ std::shared_ptr<TClient> TNetwork::Authentication(TConnection&& RawConnection) {
     } else {
         ClientKick(*Client, "Server full!");
     }
+
     return Client;
 }
 
@@ -393,6 +423,12 @@ std::vector<uint8_t> TNetwork::TCPRcv(TClient& c) {
     }
     Header = *reinterpret_cast<int32_t*>(HeaderData.data());
 
+    if (Header < 0) {
+        ClientKick(c, "Invalid packet - header negative");
+        beammp_errorf("Client {} send negative TCP header, ignoring packet", c.GetID());
+        return {};
+    }
+
     std::vector<uint8_t> Data;
     // TODO: This is arbitrary, this needs to be handled another way
     if (Header < int32_t(100 * MB)) {
@@ -425,7 +461,7 @@ std::vector<uint8_t> TNetwork::TCPRcv(TClient& c) {
 void TNetwork::ClientKick(TClient& c, const std::string& R) {
     beammp_info("Client kicked: " + R);
     if (!TCPSend(c, StringToVector("K" + R))) {
-        beammp_debugf("tried to kick player '{}' (id {}), but was already connected", c.GetName(), c.GetID());
+        beammp_debugf("tried to kick player '{}' (id {}), but was already disconnected", c.GetName(), c.GetID());
     }
     c.Disconnect("Kicked");
 }
@@ -522,8 +558,14 @@ void TNetwork::UpdatePlayer(TClient& Client) {
 }
 
 void TNetwork::OnDisconnect(const std::weak_ptr<TClient>& ClientPtr) {
-    beammp_assert(!ClientPtr.expired());
-    auto LockedClientPtr = ClientPtr.lock();
+    std::shared_ptr<TClient> LockedClientPtr { nullptr };
+    try {
+        LockedClientPtr = ClientPtr.lock();
+    } catch (const std::exception&) {
+        beammp_warn("Client expired in OnDisconnect, this is unexpected");
+        return;
+    }
+    beammp_assert(LockedClientPtr != nullptr);
     TClient& c = *LockedClientPtr;
     beammp_info(c.GetName() + (" Connection Terminated"));
     std::string Packet;
@@ -540,7 +582,7 @@ void TNetwork::OnDisconnect(const std::weak_ptr<TClient>& ClientPtr) {
     SendToAll(&c, StringToVector(Packet), false, true);
     Packet.clear();
     auto Futures = LuaAPI::MP::Engine->TriggerEvent("onPlayerDisconnect", "", c.GetID());
-    LuaAPI::MP::Engine->ReportErrors(Futures);
+    LuaAPI::MP::Engine->WaitForAll(Futures);
     c.Disconnect("Already Disconnected (OnDisconnect)");
     mServer.RemoveClient(ClientPtr);
 }
@@ -860,12 +902,13 @@ void TNetwork::SendToAll(TClient* c, const std::vector<uint8_t>& Data, bool Self
     bool ret = true;
     mServer.ForEachClient([&](std::weak_ptr<TClient> ClientPtr) -> bool {
         std::shared_ptr<TClient> Client;
-        {
+        try {
             ReadLock Lock(mServer.GetClientMutex());
-            if (!ClientPtr.expired()) {
-                Client = ClientPtr.lock();
-            } else
-                return true;
+            Client = ClientPtr.lock();
+        } catch (const std::exception&) {
+            // continue
+            beammp_warn("Client expired, shouldn't happen - if a client disconnected recently, you can ignore this");
+            return true;
         }
         if (Self || Client.get() != c) {
             if (Client->IsSynced() || Client->IsSyncing()) {

src/TServer.cpp

@@ -1,6 +1,7 @@
 #include "TServer.h"
 #include "Client.h"
 #include "Common.h"
+#include "CustomAssert.h"
 #include "TNetwork.h"
 #include "TPPSMonitor.h"
 #include <TLuaPlugin.h>
@@ -94,13 +95,20 @@ TServer::TServer(const std::vector<std::string_view>& Arguments) {
 }
 
 void TServer::RemoveClient(const std::weak_ptr<TClient>& WeakClientPtr) {
-    if (!WeakClientPtr.expired()) {
-        TClient& Client = *WeakClientPtr.lock();
-        beammp_debug("removing client " + Client.GetName() + " (" + std::to_string(ClientCount()) + ")");
-        Client.ClearCars();
-        WriteLock Lock(mClientsMutex);
-        mClients.erase(WeakClientPtr.lock());
+    std::shared_ptr<TClient> LockedClientPtr { nullptr };
+    try {
+        LockedClientPtr = WeakClientPtr.lock();
+    } catch (const std::exception&) {
+        // silently fail, as there's nothing to do
+        return;
     }
+    beammp_assert(LockedClientPtr != nullptr);
+    TClient& Client = *LockedClientPtr;
+    beammp_debug("removing client " + Client.GetName() + " (" + std::to_string(ClientCount()) + ")");
+    // TODO: Send delete packets for all cars
+    Client.ClearCars();
+    WriteLock Lock(mClientsMutex);
+    mClients.erase(WeakClientPtr.lock());
 }
 
 void TServer::ForEachClient(const std::function<bool(std::weak_ptr<TClient>)>& Fn) {
@@ -167,14 +175,20 @@ void TServer::GlobalParser(const std::weak_ptr<TClient>& Client, std::vector<uin
         }
         ParseVehicle(*LockedClient, StringPacket, Network);
         return;
-    case 'J':
-        Network.SendToAll(LockedClient.get(), Packet, false, true);
-        return;
     case 'C': {
         if (Packet.size() < 4 || std::find(Packet.begin() + 3, Packet.end(), ':') == Packet.end())
             break;
         const auto PacketAsString = std::string(reinterpret_cast<const char*>(Packet.data()), Packet.size());
-        auto Futures = LuaAPI::MP::Engine->TriggerEvent("onChatMessage", "", LockedClient->GetID(), LockedClient->GetName(), PacketAsString.substr(PacketAsString.find(':', 3) + 2));
+        std::string Message = "";
+        const auto ColonPos = PacketAsString.find(':', 3);
+        if (ColonPos != std::string::npos && ColonPos + 2 < PacketAsString.size()) {
+            Message = PacketAsString.substr(ColonPos + 2);
+        }
+        if (Message.empty()) {
+            beammp_debugf("Empty chat message received from '{}' ({}), ignoring it", LockedClient->GetName(), LockedClient->GetID());
+            return;
+        }
+        auto Futures = LuaAPI::MP::Engine->TriggerEvent("onChatMessage", "", LockedClient->GetID(), LockedClient->GetName(), Message);
         TLuaEngine::WaitForAll(Futures);
         LogChatMessage(LockedClient->GetName(), LockedClient->GetID(), PacketAsString.substr(PacketAsString.find(':', 3) + 1));
         if (std::any_of(Futures.begin(), Futures.end(),
@@ -185,7 +199,8 @@ void TServer::GlobalParser(const std::weak_ptr<TClient>& Client, std::vector<uin
                 })) {
             break;
         }
-        Network.SendToAll(nullptr, Packet, true, true);
+        std::string SanitizedPacket = fmt::format("C:{}: {}", LockedClient->GetName(), Message);
+        Network.SendToAll(nullptr, StringToVector(SanitizedPacket), true, true);
         return;
     }
     case 'E':
@@ -198,7 +213,6 @@ void TServer::GlobalParser(const std::weak_ptr<TClient>& Client, std::vector<uin
     case 'Z': // position packet
         PPSMonitor.IncrementInternalPPS();
         Network.SendToAll(LockedClient.get(), Packet, false, false);
-
         HandlePosition(*LockedClient, StringPacket);
     default:
         return;
@@ -208,6 +222,10 @@ void TServer::GlobalParser(const std::weak_ptr<TClient>& Client, std::vector<uin
 void TServer::HandleEvent(TClient& c, const std::string& RawData) {
     // E:Name:Data
     // Data is allowed to have ':'
+    if (RawData.size() < 2) {
+        beammp_debugf("Client '{}' ({}) tried to send an empty event, ignoring", c.GetName(), c.GetID());
+        return;
+    }
     auto NameDataSep = RawData.find(':', 2);
     if (NameDataSep == std::string::npos) {
         beammp_warn("received event in invalid format (missing ':'), got: '" + RawData + "'");