From b64d9c4498866f78fa0719ce1afceaecccd6b1c9 Mon Sep 17 00:00:00 2001 From: DcruBro Date: Mon, 25 May 2026 12:19:24 +0200 Subject: [PATCH] High priority and critical issues --- .gitignore | 1 + include/columnlynx/common/utils.hpp | 2 +- src/client/main.cpp | 16 +++- src/client/net/tcp/tcp_client.cpp | 16 +++- src/client/net/udp/udp_client.cpp | 21 ++++- src/common/session_registry.cpp | 21 ++++- src/common/tcp_message_handler.cpp | 12 ++- src/common/utils.cpp | 62 +++++++++++++-- src/common/virtual_interface.cpp | 108 ++++++++++++-------------- src/server/main.cpp | 18 +++++ src/server/net/tcp/tcp_connection.cpp | 16 ++-- 11 files changed, 213 insertions(+), 80 deletions(-) diff --git a/.gitignore b/.gitignore index 290af9a..92448fb 100644 --- a/.gitignore +++ b/.gitignore @@ -12,5 +12,6 @@ _deps CMakeUserPresets.json build/ +build*/ .vscode/ .DS_Store diff --git a/include/columnlynx/common/utils.hpp b/include/columnlynx/common/utils.hpp index 51d6f69..5ad2605 100644 --- a/include/columnlynx/common/utils.hpp +++ b/include/columnlynx/common/utils.hpp @@ -15,7 +15,7 @@ #include #include #include -#include + #ifdef _WIN32 #include diff --git a/src/client/main.cpp b/src/client/main.cpp index f29724d..be27d56 100644 --- a/src/client/main.cpp +++ b/src/client/main.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -90,7 +91,20 @@ int main(int argc, char** argv) { std::string configPath = optionsObj["config-dir"].as(); 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() != '\\') { diff --git a/src/client/net/tcp/tcp_client.cpp b/src/client/net/tcp/tcp_client.cpp index aac19c8..9766d86 100644 --- a/src/client/net/tcp/tcp_client.cpp +++ b/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."); diff --git a/src/client/net/udp/udp_client.cpp b/src/client/net/udp/udp_client.cpp index 9c12c8a..dc755f7 100644 --- a/src/client/net/udp/udp_client.cpp +++ b/src/client/net/udp/udp_client.cpp @@ -73,7 +73,7 @@ namespace ColumnLynx::Net::UDP { reinterpret_cast(&hdr) + sizeof(UDPPacketHeader) ); uint32_t sessionID = static_cast(ClientSession::getInstance().getSessionID()); - uint32_t sessionIDNet = sessionID; + uint32_t sessionIDNet = htonl(sessionID); packet.insert(packet.end(), reinterpret_cast(&sessionIDNet), reinterpret_cast(&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 ciphertext( - mRecvBuffer.begin() + sizeof(UDPPacketHeader) + sizeof(uint32_t), - mRecvBuffer.begin() + bytes + mRecvBuffer.begin() + headerLen, + mRecvBuffer.begin() + headerLen + ciphertextLen ); if (ClientSession::getInstance().getAESKey().empty()) { diff --git a/src/common/session_registry.cpp b/src/common/session_registry.cpp index 7511081..6df8019 100644 --- a/src/common/session_registry.cpp +++ b/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() { diff --git a/src/common/tcp_message_handler.cpp b/src/common/tcp_message_handler.cpp index 527d668..66e2fe3 100644 --- a/src/common/tcp_message_handler.cpp +++ b/src/common/tcp_message_handler.cpp @@ -20,10 +20,16 @@ namespace ColumnLynx::Net::TCP { auto data = std::make_shared>(); data->push_back(typeByte); - uint16_t length = payload.size(); + // Ensure payload fits into protocol's 16-bit length field + if (payload.size() > static_cast(std::numeric_limits::max())) { + Utils::error("sendMessage(): payload too large (>65535 bytes)"); + return; + } - data->push_back(length >> 8); - data->push_back(length & 0xFF); + uint16_t length = static_cast(payload.size()); + + data->push_back(static_cast(length >> 8)); + data->push_back(static_cast(length & 0xFF)); data->insert(data->end(), payload.begin(), payload.end()); auto self = shared_from_this(); diff --git a/src/common/utils.cpp b/src/common/utils.cpp index 209e0f3..9ba8d40 100644 --- a/src/common/utils.cpp +++ b/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 +#include namespace ColumnLynx::Utils { std::string unixMillisToISO8601(uint64_t unixMillis, bool local) { @@ -144,19 +145,49 @@ namespace ColumnLynx::Utils { std::vector 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(line.back()))) line.pop_back(); + size_t start = 0; + while (start < line.size() && isspace(static_cast(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++) { - line[i] = toupper(line[i]); + for (size_t i = 0; i < key.length(); ++i) { + key[i] = static_cast(toupper(static_cast(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 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; diff --git a/src/common/virtual_interface.cpp b/src/common/virtual_interface.cpp index 1807eec..c18b2dd 100644 --- a/src/common/virtual_interface.cpp +++ b/src/common/virtual_interface.cpp @@ -4,6 +4,11 @@ #include +#include +#include + +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& args) { + if (args.empty()) return false; + + std::vector argv; + argv.reserve(args.size() + 1); + for (const auto &s : args) { + argv.push_back(const_cast(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]; - snprintf(cmd, sizeof(cmd), - "ip addr flush dev %s", - mIfName.c_str() - ); - system(cmd); + runCommand({"ip", "addr", "flush", "dev", mIfName}); #elif defined(__APPLE__) - char cmd[512]; - snprintf(cmd, sizeof(cmd), - "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); + runCommand({"ifconfig", mIfName, "inet", "0.0.0.0", "delete"}); + runCommand({"ifconfig", mIfName, "inet6", "::", "delete"}); // 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), - "ip addr flush dev %s", - mIfName.c_str() - ); - system(cmd); + runCommand({"ip", "addr", "flush", "dev", mIfName}); - snprintf(cmd, sizeof(cmd), - "ip addr add %s/%d peer %s dev %s", - ipStr.c_str(), prefixLen, peerStr.c_str(), mIfName.c_str()); - system(cmd); - - snprintf(cmd, sizeof(cmd), - "ip link set dev %s up mtu %d", mIfName.c_str(), mtu); - system(cmd); + // Add address with peer + std::string addrArg = ipStr + "/" + std::to_string(prefixLen); + runCommand({"ip", "addr", "add", addrArg, "peer", peerStr, "dev", mIfName}); + + // Bring link up and set MTU + runCommand({"ip", "link", "set", "dev", mIfName, "up", "mtu", std::to_string(mtu)}); 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 - snprintf(cmd, sizeof(cmd), - "ifconfig %s inet 0.0.0.0 delete", - mIfName.c_str() - ); - system(cmd); + // Reset IPv4 and IPv6 addresses + runCommand({"ifconfig", mIfName, "inet", "0.0.0.0", "delete"}); + runCommand({"ifconfig", mIfName, "inet6", "::", "delete"}); - snprintf(cmd, sizeof(cmd), - "ifconfig %s inet6 :: delete", - mIfName.c_str() - ); - system(cmd); + // Set address and netmask + std::string netArg = ipStr + " " + peerStr; // ifconfig expects ip peer + runCommand({"ifconfig", mIfName, "inet", ipStr, peerStr, "mtu", std::to_string(mtu), "netmask", prefixStr, "up"}); - // Set - snprintf(cmd, sizeof(cmd), - "ifconfig %s inet %s %s mtu %d netmask %s up", - 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)); + // Add route for the network + std::string networkArg = ipStr + "/" + std::to_string(prefixLen); + runCommand({"route", "-n", "add", "-net", networkArg, "-interface", mIfName}); return true; } diff --git a/src/server/main.cpp b/src/server/main.cpp index c1ebf0e..441546c 100644 --- a/src/server/main.cpp +++ b/src/server/main.cpp @@ -145,6 +145,22 @@ int main(int argc, char** argv) { auto server = std::make_shared(io, serverPort()); auto udpServer = std::make_shared(io, serverPort()); + // Schedule periodic cleanup of expired sessions every 5 minutes + auto cleanupTimer = std::make_shared(io); + auto cleanupHandler = std::make_shared>(); + *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(); }); }); diff --git a/src/server/net/tcp/tcp_connection.cpp b/src/server/net/tcp/tcp_connection.cpp index 0ab4982..bcfd37d 100644 --- a/src/server/net/tcp/tcp_connection.cpp +++ b/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) { - Utils::warn("HANDSHAKE_INIT from " + reqAddr + " is too short."); + if (data.size() != 1 + crypto_sign_PUBLICKEYBYTES) { + 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(