Skip to content

Commit e910523

Browse files
author
integrator
committed
security: clear CodeQL findings (gmtime thread-safety + path-injection hardening)
Clears the 8 real CodeQL security alerts on frstrtr/c2pool. Triaged + independently re-reviewed: 1 CRITICAL is a genuine fix, 2 HIGH get defense-in-depth, 5 HIGH are getenv-rooted false positives (dismissed separately with justification). - #490 CRITICAL cpp/potentially-dangerous-function (src/core/log.cpp:163): std::gmtime returns a pointer into a shared static tm and is not reentrant. Replace with gmtime_r (POSIX) / gmtime_s (MSVC) into a local tm. Only gmtime/localtime call in src/. - #502/#503 HIGH cpp/path-injection (coin_peer_manager.hpp db_path()): m_symbol is a hardcoded ticker literal at every production ctor site (traced through all callers; no network->filename path exists), but add an alphanumeric-only guard so a future caller cannot inject a path component. Legitimate tickers (LTC/DOGE/DGB/DASH/BTC/NMC/...) are all alnum -> no on-disk filename change. - Rider: core::filesystem::config_path() dereferenced getenv() without a null check -> std::filesystem::path(nullptr) is UB if HOME/APPDATA is unset. Fall back to '.'. The other 5 path-injection alerts (#507 node.cpp:2199, #506/#505/#504 payout_manager.cpp:224/261/284, #501 auto_ratchet.hpp:309) all build a path from config_path() (getenv HOME/APPDATA) + a hardcoded basename, never from network/peer/miner input -> dismissed as false positive.
1 parent 3f261dd commit e910523

3 files changed

Lines changed: 33 additions & 4 deletions

File tree

src/c2pool/merged/coin_peer_manager.hpp

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <cmath>
1818
#include <random>
1919
#include <algorithm>
20+
#include <cctype>
2021
#include <array>
2122
#include <sstream>
2223

@@ -698,7 +699,20 @@ class CoinPeerManager
698699
else
699700
dir = m_data_dir;
700701
std::filesystem::create_directories(dir);
701-
return (dir / ("peers_" + m_symbol + ".json")).string();
702+
// Defense-in-depth (CodeQL cpp/path-injection): m_symbol is a hardcoded
703+
// ticker literal at every production ctor site, but never let it become a
704+
// path component. Reject anything non-alphanumeric so no '/','\\','.' or
705+
// NUL can survive into the filename.
706+
std::string sym = m_symbol;
707+
const bool clean = !sym.empty() && sym.size() <= 16 &&
708+
std::all_of(sym.begin(), sym.end(),
709+
[](unsigned char c) { return std::isalnum(c) != 0; });
710+
if (!clean) {
711+
LOG_WARNING << "[PeerManager] non-alphanumeric coin symbol '"
712+
<< m_symbol << "' -> using peers_unknown.json";
713+
sym = "unknown";
714+
}
715+
return (dir / ("peers_" + sym + ".json")).string();
702716
}
703717

704718
void save_peers()

src/core/filesystem.hpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
#include <string>
44
#include <filesystem>
5+
#include <cstdlib>
56

67
namespace core
78
{
@@ -11,10 +12,15 @@ namespace filesystem
1112

1213
inline std::filesystem::path config_path()
1314
{
15+
// getenv may return nullptr if the var is unset; std::filesystem::path(nullptr)
16+
// is UB. Fall back to the current directory so an unset HOME/APPDATA degrades
17+
// gracefully instead of crashing.
1418
#ifdef _WIN32
15-
return std::filesystem::path(std::getenv("APPDATA")) / "c2pool";
19+
const char* base = std::getenv("APPDATA");
20+
return std::filesystem::path(base ? base : ".") / "c2pool";
1621
#else // Linux + macOS
17-
return std::filesystem::path(std::getenv("HOME")) / ".c2pool";
22+
const char* base = std::getenv("HOME");
23+
return std::filesystem::path(base ? base : ".") / ".c2pool";
1824
#endif
1925
}
2026

src/core/log.cpp

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,16 @@ void Logger::init(const std::string& log_file_name, int rotation_size_mb, int ma
160160
if (!fec && sz > static_cast<uintmax_t>(rotation_size_mb) * 1024 * 1024) {
161161
std::time_t t = std::time(nullptr);
162162
char ts[32];
163-
std::strftime(ts, sizeof(ts), "%Y%m%d-%H%M%S", std::gmtime(&t));
163+
// Thread-safe gmtime: std::gmtime returns a pointer into a shared
164+
// static tm and is not reentrant (CodeQL cpp/potentially-dangerous-
165+
// function). Use the reentrant per-platform variant into a local tm.
166+
std::tm tmv{};
167+
#ifdef _WIN32
168+
gmtime_s(&tmv, &t); // MSVC secure-CRT: (tm*, time_t*)
169+
#else
170+
gmtime_r(&t, &tmv); // POSIX GCC/Clang: (time_t*, tm*)
171+
#endif
172+
std::strftime(ts, sizeof(ts), "%Y%m%d-%H%M%S", &tmv);
164173
std::filesystem::path rotated =
165174
rotated_dir / (debug_log.filename().string() + std::string(".") + ts);
166175
std::filesystem::rename(debug_log, rotated, fec);

0 commit comments

Comments
 (0)