High priority and critical issues

.gitignore

@@ -12,5 +12,6 @@ _deps
 CMakeUserPresets.json
 
 build/
+build*/
 .vscode/
 .DS_Store

include/columnlynx/common/utils.hpp

@@ -15,7 +15,7 @@
 #include <unordered_map>
 #include <unordered_set>
 #include <algorithm>
-#include <bits/stdc++.h>
+
 
 #ifdef _WIN32
     #include <winsock2.h>

src/client/main.cpp

@@ -5,6 +5,7 @@
 #include <asio.hpp>
 #include <csignal>
 #include <iostream>
+#include <filesystem>
 #include <columnlynx/common/utils.hpp>
 #include <columnlynx/common/panic_handler.hpp>
 #include <columnlynx/client/net/tcp/tcp_client.hpp>
@@ -90,7 +91,20 @@ int main(int argc, char** argv) {
         std::string configPath = optionsObj["config-dir"].as<std::string>();
         const char* envConfigPath = std::getenv("COLUMNLYNX_CONFIG_DIR");
         if (envConfigPath != nullptr) {
-            configPath = std::string(envConfigPath);
+            // Validate and canonicalize environment-provided path
+            try {
+                namespace fs = std::filesystem;
+                std::error_code ec;
+                fs::path candidate(envConfigPath);
+                fs::path abs = fs::absolute(candidate, ec);
+                if (!ec) {
+                    configPath = abs.string();
+                } else {
+                    warn(std::string("Invalid COLUMNLYNX_CONFIG_DIR value: ") + envConfigPath + " - using default");
+                }
+            } catch (const std::exception& e) {
+                warn(std::string("Failed to canonicalize COLUMNLYNX_CONFIG_DIR: ") + e.what());
+            }
         }
 
         if (configPath.back() != '/' && configPath.back() != '\\') {

src/client/net/tcp/tcp_client.cpp

@@ -159,7 +159,13 @@ namespace ColumnLynx::Net::TCP {
     void TCPClient::mHandleMessage(ServerMessageType type, const std::string& data) {
         switch (type) {
             case ServerMessageType::HANDSHAKE_IDENTIFY: {
-                    std::memcpy(mServerPublicKey, data.data(), std::min(data.size(), sizeof(mServerPublicKey)));
+                    if (data.size() != sizeof(mServerPublicKey)) {
+                        Utils::warn("HANDSHAKE_IDENTIFY has invalid size: " + std::to_string(data.size()));
+                        disconnect();
+                        return;
+                    }
+
+                    std::memcpy(mServerPublicKey, data.data(), sizeof(mServerPublicKey));
                     std::string hexServerPub = Utils::bytesToHexString(mServerPublicKey, 32);
                     Utils::log("Received server identity. Public Key: " + hexServerPub);
 
@@ -188,7 +194,13 @@ namespace ColumnLynx::Net::TCP {
                 {
                     // Verify the signature
                     Signature sig{};
-                    std::memcpy(sig.data(), data.data(), std::min(data.size(), sig.size()));
+                    if (data.size() != sig.size()) {
+                        Utils::warn("HANDSHAKE_CHALLENGE_RESPONSE has invalid size: " + std::to_string(data.size()));
+                        disconnect();
+                        return;
+                    }
+
+                    std::memcpy(sig.data(), data.data(), sig.size());
                     if (Utils::LibSodiumWrapper::verifyMessage(mSubmittedChallenge.data(), mSubmittedChallenge.size(), sig, mServerPublicKey)) {
                         Utils::log("Challenge response verified successfully.");
                         

src/client/net/udp/udp_client.cpp

@@ -73,7 +73,7 @@ namespace ColumnLynx::Net::UDP {
             reinterpret_cast<uint8_t*>(&hdr) + sizeof(UDPPacketHeader)
         );
         uint32_t sessionID = static_cast<uint32_t>(ClientSession::getInstance().getSessionID());
-        uint32_t sessionIDNet = sessionID;
+        uint32_t sessionIDNet = htonl(sessionID);
         packet.insert(packet.end(),
             reinterpret_cast<uint8_t*>(&sessionIDNet),
             reinterpret_cast<uint8_t*>(&sessionIDNet) + sizeof(uint32_t)
@@ -136,9 +136,24 @@ namespace ColumnLynx::Net::UDP {
         }
 
         // Decrypt payload
+        // Extract ciphertext safely
+        size_t headerLen = sizeof(UDPPacketHeader) + sizeof(uint32_t);
+        if (bytes < headerLen) {
+            Utils::warn("UDP Client received packet too small after header check.");
+            return;
+        }
+
+        size_t ciphertextLen = bytes - headerLen;
+        // Enforce reasonable maximum (UDP payload practical limit)
+        const size_t MAX_UDP_PAYLOAD = 65507; // 65535 - UDP/IP headers
+        if (ciphertextLen > MAX_UDP_PAYLOAD) {
+            Utils::warn("UDP Client received packet with excessive payload size: " + std::to_string(ciphertextLen));
+            return;
+        }
+
         std::vector<uint8_t> ciphertext(
-            mRecvBuffer.begin() + sizeof(UDPPacketHeader) + sizeof(uint32_t),
+            mRecvBuffer.begin() + headerLen,
-            mRecvBuffer.begin() + bytes
+            mRecvBuffer.begin() + headerLen + ciphertextLen
         );
 
         if (ClientSession::getInstance().getAESKey().empty()) {

src/common/session_registry.cpp

@@ -32,7 +32,26 @@ namespace ColumnLynx::Net {
 
     void SessionRegistry::erase(uint32_t sessionID) {
         std::unique_lock lock(mMutex);
-        mSessions.erase(sessionID);
+        auto it = mSessions.find(sessionID);
+        if (it != mSessions.end()) {
+            // If the session has a client IP mapping, remove it to avoid stale entries
+            if (it->second) {
+                uint32_t ip = it->second->clientTunIP;
+                auto ipIt = mIPSessions.find(ip);
+                if (ipIt != mIPSessions.end()) {
+                    // Only erase if it points to the same session
+                    if (ipIt->second == it->second) {
+                        mIPSessions.erase(ipIt);
+                    }
+                }
+            }
+
+            // Remove any session->ip bookkeeping
+            mSessionIPs.erase(sessionID);
+
+            // Finally erase the session
+            mSessions.erase(it);
+        }
     }
 
     void SessionRegistry::cleanupExpired() {

src/common/tcp_message_handler.cpp

@@ -20,10 +20,16 @@ namespace ColumnLynx::Net::TCP {
 
         auto data = std::make_shared<std::vector<uint8_t>>();
         data->push_back(typeByte);
-        uint16_t length = payload.size();
+        // Ensure payload fits into protocol's 16-bit length field
+        if (payload.size() > static_cast<size_t>(std::numeric_limits<uint16_t>::max())) {
+            Utils::error("sendMessage(): payload too large (>65535 bytes)");
+            return;
+        }
 
-        data->push_back(length >> 8);
+        uint16_t length = static_cast<uint16_t>(payload.size());
-        data->push_back(length & 0xFF);
+
+        data->push_back(static_cast<uint8_t>(length >> 8));
+        data->push_back(static_cast<uint8_t>(length & 0xFF));
 
         data->insert(data->end(), payload.begin(), payload.end());
         auto self = shared_from_this();

src/common/utils.cpp

@@ -3,6 +3,7 @@
 // Distributed under the terms of the GNU General Public License, either version 2 only or version 3. See LICENSES/ for details.
 
 #include <columnlynx/common/utils.hpp>
+#include <filesystem>
 
 namespace ColumnLynx::Utils {
     std::string unixMillisToISO8601(uint64_t unixMillis, bool local) {
@@ -144,19 +145,49 @@ namespace ColumnLynx::Utils {
 
         std::vector<std::string> out;
 
-        std::ifstream file(basePath + "whitelisted_keys");
+        namespace fs = std::filesystem;
+        std::error_code ec;
+
+        fs::path base(basePath);
+        fs::path absBase = fs::absolute(base, ec);
+        if (ec) {
+            warn("getWhitelistedKeys(): failed to resolve base path: " + basePath + " - " + ec.message());
+            return out;
+        }
+
+        fs::path whitelist = absBase / "whitelisted_keys";
+        if (!fs::exists(whitelist, ec) || ec) {
+            warn("getWhitelistedKeys(): whitelist file not found: " + whitelist.string());
+            return out;
+        }
+
+        // Canonicalize to avoid symlink tricks
+        fs::path canon = fs::canonical(whitelist, ec);
+        if (ec) {
+            warn("getWhitelistedKeys(): failed to canonicalize path: " + whitelist.string());
+            return out;
+        }
+
+        std::ifstream file(canon);
         if (!file.is_open()) {
-            warn("Failed to open whitelisted_keys file at path: " + basePath + "whitelisted_keys");
+            warn("getWhitelistedKeys(): failed to open whitelist file: " + canon.string());
             return out;
         }
 
         std::string line;
         while (std::getline(file, line)) {
+            // Trim whitespace
+            while (!line.empty() && isspace(static_cast<unsigned char>(line.back()))) line.pop_back();
+            size_t start = 0;
+            while (start < line.size() && isspace(static_cast<unsigned char>(line[start]))) ++start;
+            if (start >= line.size()) continue;
+            std::string key = line.substr(start);
+
             // Convert to upper case to align with the bytesToHexString() output
-            for (int i = 0; i < line.length(); i++) {
+            for (size_t i = 0; i < key.length(); ++i) {
-                line[i] = toupper(line[i]);
+                key[i] = static_cast<char>(toupper(static_cast<unsigned char>(key[i])));
             }
-            out.push_back(line);
+            out.push_back(key);
         }
 
         return out;
@@ -166,9 +197,26 @@ namespace ColumnLynx::Utils {
         // TODO: Currently re-reads every time.
         std::vector<std::string> readLines;
 
-        std::ifstream file(path);
+        namespace fs = std::filesystem;
+        std::error_code ec;
+        fs::path p(path);
+        fs::path abs = fs::absolute(p, ec);
+        if (ec) {
+            throw std::runtime_error("getConfigMap(): failed to resolve path: " + path + " - " + ec.message());
+        }
+
+        if (!fs::exists(abs, ec) || ec) {
+            throw std::runtime_error("getConfigMap(): config file does not exist: " + abs.string());
+        }
+
+        fs::path canon = fs::canonical(abs, ec);
+        if (ec) {
+            throw std::runtime_error("getConfigMap(): failed to canonicalize config path: " + abs.string());
+        }
+
+        std::ifstream file(canon);
         if (!file.is_open()) {
-            throw std::runtime_error("Failed to open config file at path: " + path);
+            throw std::runtime_error("Failed to open config file at path: " + canon.string());
         }
 
         std::string line;

src/common/virtual_interface.cpp

@@ -4,6 +4,11 @@
 
 #include <columnlynx/common/net/virtual_interface.hpp>
 
+#include <spawn.h>
+#include <sys/wait.h>
+
+extern char **environ;
+
 // This is all fucking voodoo dark magic.
 
 #if defined(_WIN32)
@@ -56,6 +61,33 @@ static void InitializeWintun()
 #endif // _WIN32
 
 namespace ColumnLynx::Net {
+
+// Run a command without invoking a shell. Arguments are passed directly
+// to the underlying process to avoid shell injection vulnerabilities.
+static bool runCommand(const std::vector<std::string>& args) {
+    if (args.empty()) return false;
+
+    std::vector<char*> argv;
+    argv.reserve(args.size() + 1);
+    for (const auto &s : args) {
+        argv.push_back(const_cast<char*>(s.c_str()));
+    }
+    argv.push_back(nullptr);
+
+    pid_t pid;
+    int rc = posix_spawnp(&pid, argv[0], nullptr, nullptr, argv.data(), environ);
+    if (rc != 0) {
+        return false;
+    }
+
+    int status = 0;
+    if (waitpid(pid, &status, 0) == -1) {
+        return false;
+    }
+
+    return WIFEXITED(status) && WEXITSTATUS(status) == 0;
+}
+
     // ------------------------------ Constructor ------------------------------
     VirtualInterface::VirtualInterface(const std::string& ifName)
         : mIfName(ifName), mFd(-1)
@@ -307,25 +339,10 @@ namespace ColumnLynx::Net {
 
     void VirtualInterface::resetIP() {
     #if defined(__linux__)
-        char cmd[512];
+        runCommand({"ip", "addr", "flush", "dev", mIfName});
-        snprintf(cmd, sizeof(cmd),
-                 "ip addr flush dev %s",
-                 mIfName.c_str()
-        );
-        system(cmd);
     #elif defined(__APPLE__)
-        char cmd[512];
+        runCommand({"ifconfig", mIfName, "inet", "0.0.0.0", "delete"});
-        snprintf(cmd, sizeof(cmd),
+        runCommand({"ifconfig", mIfName, "inet6", "::", "delete"});
-                 "ifconfig %s inet 0.0.0.0 delete",
-                 mIfName.c_str()
-        );
-        system(cmd);
-
-        snprintf(cmd, sizeof(cmd),
-                 "ifconfig %s inet6 :: delete",
-                 mIfName.c_str()
-        );
-        system(cmd);
 
         // Wipe old routes
         //snprintf(cmd, sizeof(cmd),
@@ -357,26 +374,19 @@ namespace ColumnLynx::Net {
     bool VirtualInterface::mApplyLinuxIP(uint32_t clientIP, uint32_t serverIP,
                                         uint8_t prefixLen, uint16_t mtu)
     {
-        char cmd[512];
     
         std::string ipStr = ipv4ToString(clientIP);
         std::string peerStr = ipv4ToString(serverIP);
     
         // Wipe the current config
-        snprintf(cmd, sizeof(cmd),
+        runCommand({"ip", "addr", "flush", "dev", mIfName});
-                 "ip addr flush dev %s",
-                 mIfName.c_str()
-        );
-        system(cmd);
 
-        snprintf(cmd, sizeof(cmd),
+        // Add address with peer
-                 "ip addr add %s/%d peer %s dev %s",
+        std::string addrArg = ipStr + "/" + std::to_string(prefixLen);
-                 ipStr.c_str(), prefixLen, peerStr.c_str(), mIfName.c_str());
+        runCommand({"ip", "addr", "add", addrArg, "peer", peerStr, "dev", mIfName});
-        system(cmd);
 
-        snprintf(cmd, sizeof(cmd),
+        // Bring link up and set MTU
-                 "ip link set dev %s up mtu %d", mIfName.c_str(), mtu);
+        runCommand({"ip", "link", "set", "dev", mIfName, "up", "mtu", std::to_string(mtu)});
-        system(cmd);
     
         return true;
     }
@@ -387,39 +397,23 @@ namespace ColumnLynx::Net {
     bool VirtualInterface::mApplyMacOSIP(uint32_t clientIP, uint32_t serverIP,
                                         uint8_t prefixLen, uint16_t mtu)
     {
-        char cmd[512];
     
         std::string ipStr = ipv4ToString(clientIP);
         std::string peerStr = ipv4ToString(serverIP);
         std::string prefixStr = ipv4ToString(prefixLengthToNetmask(prefixLen), false);
         Utils::debug("Prefix string: " + prefixStr);
     
-        // Reset
+        // Reset IPv4 and IPv6 addresses
-        snprintf(cmd, sizeof(cmd),
+        runCommand({"ifconfig", mIfName, "inet", "0.0.0.0", "delete"});
-                 "ifconfig %s inet 0.0.0.0 delete",
+        runCommand({"ifconfig", mIfName, "inet6", "::", "delete"});
-                mIfName.c_str()
-        );
-        system(cmd);
 
-        snprintf(cmd, sizeof(cmd),
+        // Set address and netmask
-                 "ifconfig %s inet6 :: delete",
+        std::string netArg = ipStr + " " + peerStr; // ifconfig expects ip peer
-                mIfName.c_str()
+        runCommand({"ifconfig", mIfName, "inet", ipStr, peerStr, "mtu", std::to_string(mtu), "netmask", prefixStr, "up"});
-        );
-        system(cmd);
 
-        // Set
+        // Add route for the network
-        snprintf(cmd, sizeof(cmd),
+        std::string networkArg = ipStr + "/" + std::to_string(prefixLen);
-                "ifconfig %s inet %s %s mtu %d netmask %s up",
+        runCommand({"route", "-n", "add", "-net", networkArg, "-interface", mIfName});
-                 mIfName.c_str(), ipStr.c_str(), peerStr.c_str(), mtu, prefixStr.c_str());
-        system(cmd);
-
-        // Host bits are auto-normalized by the kernel on macOS, so we don't need to worry about them not being zeroed out.
-        snprintf(cmd, sizeof(cmd),
-                 "route -n add -net %s/%d -interface %s",
-                 ipStr.c_str(), prefixLen, mIfName.c_str());
-        system(cmd);
-
-        Utils::log("Executed command: " + std::string(cmd));
     
         return true;
     }

src/server/main.cpp

@@ -145,6 +145,22 @@ int main(int argc, char** argv) {
         auto server = std::make_shared<TCPServer>(io, serverPort());
         auto udpServer = std::make_shared<UDPServer>(io, serverPort());
 
+        // Schedule periodic cleanup of expired sessions every 5 minutes
+        auto cleanupTimer = std::make_shared<asio::steady_timer>(io);
+        auto cleanupHandler = std::make_shared<std::function<void(const asio::error_code&)>>();
+        *cleanupHandler = [cleanupTimer, cleanupHandler](const asio::error_code& ec) {
+            if (ec == asio::error::operation_aborted) return; // Timer cancelled
+            try {
+                SessionRegistry::getInstance().cleanupExpired();
+            } catch (const std::exception& e) {
+                Utils::warn(std::string("SessionRegistry::cleanupExpired() threw: ") + e.what());
+            }
+            cleanupTimer->expires_after(std::chrono::minutes(5));
+            cleanupTimer->async_wait(*cleanupHandler);
+        };
+        cleanupTimer->expires_after(std::chrono::minutes(5));
+        cleanupTimer->async_wait(*cleanupHandler);
+
         asio::signal_set signals(io, SIGINT, SIGTERM);
         signals.async_wait([&](const std::error_code&, int) {
             log("Received termination signal. Shutting down server gracefully.");
@@ -153,6 +169,8 @@ int main(int argc, char** argv) {
                 ServerSession::getInstance().setHostRunning(false);
                 server->stop();
                 udpServer->stop();
+                // Cancel cleanup timer
+                cleanupTimer->cancel();
             });
         });
 

src/server/net/tcp/tcp_connection.cpp

@@ -124,8 +124,8 @@ namespace ColumnLynx::Net::TCP {
             case ClientMessageType::HANDSHAKE_INIT: {
                 Utils::log("Received HANDSHAKE_INIT from " + reqAddr);
 
-                if (data.size() < 1 + crypto_box_PUBLICKEYBYTES) {
+                if (data.size() != 1 + crypto_sign_PUBLICKEYBYTES) {
-                    Utils::warn("HANDSHAKE_INIT from " + reqAddr + " is too short.");
+                    Utils::warn("HANDSHAKE_INIT from " + reqAddr + " has invalid size: " + std::to_string(data.size()));
                     disconnect();
                     return;
                 }
@@ -141,7 +141,7 @@ namespace ColumnLynx::Net::TCP {
                 Utils::log("Client protocol version " + std::to_string(clientProtoVer) + " accepted from " + reqAddr + ".");
 
                 PublicKey signPk;
-                std::memcpy(signPk.data(), data.data() + 1, std::min(data.size() - 1, sizeof(signPk)));
+                std::memcpy(signPk.data(), data.data() + 1, sizeof(signPk));
 
                 // We can safely store this without further checking, the client will need to send the encrypted AES key in a way where they must possess the corresponding private key anyways.
                 int r = crypto_sign_ed25519_pk_to_curve25519(mConnectionPublicKey.data(), signPk.data()); // Store the client's public encryption key key (for identification)
@@ -173,9 +173,15 @@ namespace ColumnLynx::Net::TCP {
             case ClientMessageType::HANDSHAKE_CHALLENGE: {
                 Utils::log("Received HANDSHAKE_CHALLENGE from " + reqAddr);
                 
-                // Convert to byte array
+                // Convert to byte array - require exact size
+                if (data.size() != 32) {
+                    Utils::warn("HANDSHAKE_CHALLENGE has invalid size: " + std::to_string(data.size()));
+                    disconnect();
+                    return;
+                }
+
                 uint8_t challengeData[32];
-                std::memcpy(challengeData, data.data(), std::min(data.size(), sizeof(challengeData)));
+                std::memcpy(challengeData, data.data(), sizeof(challengeData));
 
                 // Sign the challenge
                 Signature sig = Utils::LibSodiumWrapper::signMessage(