diff --git a/miner/proxy/src/App_unix.cpp b/miner/proxy/src/App_unix.cpp index f2a9841..a31f265 100644 --- a/miner/proxy/src/App_unix.cpp +++ b/miner/proxy/src/App_unix.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include @@ -43,6 +44,8 @@ bool xmrig::App::background(int &rc) int i = fork(); if (i < 0) { + // SECURITY FIX (MED-004): Log fork() failure for debugging + LOG_ERR("fork() failed (errno = %d: %s)", errno, strerror(errno)); rc = 1; return true; diff --git a/miner/proxy/src/App_win.cpp b/miner/proxy/src/App_win.cpp index e803080..31d4eb6 100644 --- a/miner/proxy/src/App_win.cpp +++ b/miner/proxy/src/App_win.cpp @@ -41,8 +41,8 @@ bool xmrig::App::background(int &) if (hcon) { ShowWindow(hcon, SW_HIDE); } else { - HANDLE h = GetStdHandle(STD_OUTPUT_HANDLE); - CloseHandle(h); + // SECURITY FIX (HIGH-025): Don't call CloseHandle on standard handles + // Standard handles from GetStdHandle() should not be closed by the application FreeConsole(); } diff --git a/miner/proxy/src/base/net/stratum/Client.cpp b/miner/proxy/src/base/net/stratum/Client.cpp index b86819e..cd795ca 100644 --- a/miner/proxy/src/base/net/stratum/Client.cpp +++ b/miner/proxy/src/base/net/stratum/Client.cpp @@ -293,17 +293,35 @@ void xmrig::Client::tick(uint64_t now) else if (m_keepAlive && now > m_keepAlive) { ping(); } - - return; } - - if (m_state == ReconnectingState && m_expire && now > m_expire) { + else if (m_state == ReconnectingState && m_expire && now > m_expire) { return connect(); } - - if (m_state == ConnectingState && m_expire && now > m_expire) { + else if (m_state == ConnectingState && m_expire && now > m_expire) { close(); } + + // SECURITY FIX (HIGH-017): Clean up stale entries from results and callbacks maps + // to prevent unbounded memory growth if pool never responds + constexpr uint64_t kStaleTimeout = 60000; // 60 seconds + + // Clean up stale submit results + for (auto it = m_results.begin(); it != m_results.end(); ) { + if (now > it->second.start() + kStaleTimeout) { + it = m_results.erase(it); + } else { + ++it; + } + } + + // Clean up stale callbacks + for (auto it = m_callbacks.begin(); it != m_callbacks.end(); ) { + if (now > it->second.ts + kStaleTimeout) { + it = m_callbacks.erase(it); + } else { + ++it; + } + } } diff --git a/miner/proxy/src/base/net/stratum/EthStratumClient.cpp b/miner/proxy/src/base/net/stratum/EthStratumClient.cpp index fd877d7..3368ad5 100644 --- a/miner/proxy/src/base/net/stratum/EthStratumClient.cpp +++ b/miner/proxy/src/base/net/stratum/EthStratumClient.cpp @@ -283,27 +283,34 @@ void xmrig::EthStratumClient::parseNotification(const char *method, const rapidj Buffer buf = Cvt::fromHex(blob.c_str(), blob.length()); // Get height from coinbase + // SECURITY FIX (HIGH-022): Add bounds checking for buffer access { - uint8_t* p = buf.data() + 32; - uint8_t* m = p + 128; + job.setHeight(0); // Default if parsing fails - while ((p < m) && (*p != 0xff)) ++p; - while ((p < m) && (*p == 0xff)) ++p; + // Need at least 160 bytes (32 offset + 128 scan range) + if (buf.size() >= 160) { + uint8_t* base = buf.data(); + uint8_t* p = base + 32; + uint8_t* m = base + 160; // Use fixed end based on buffer size check + uint8_t* end = base + buf.size(); - if ((p < m) && (*(p - 1) == 0xff) && (*(p - 2) == 0xff)) { - uint32_t height = *reinterpret_cast(p + 2); - switch (*(p + 1)) { - case 4: - height += *reinterpret_cast(p + 4) * 0x10000UL; - break; - case 3: - height += *(p + 4) * 0x10000UL; - break; + while ((p < m) && (*p != 0xff)) ++p; + while ((p < m) && (*p == 0xff)) ++p; + + // Ensure p-2 is valid (p >= base + 34) and p+5 is within buffer + if ((p < m) && (p >= base + 34) && (p + 5 <= end) && + (*(p - 1) == 0xff) && (*(p - 2) == 0xff)) { + uint32_t height = *reinterpret_cast(p + 2); + switch (*(p + 1)) { + case 4: + height += *reinterpret_cast(p + 4) * 0x10000UL; + break; + case 3: + height += *(p + 4) * 0x10000UL; + break; + } + job.setHeight(height); } - job.setHeight(height); - } - else { - job.setHeight(0); } } diff --git a/miner/proxy/src/base/net/stratum/SubmitResult.h b/miner/proxy/src/base/net/stratum/SubmitResult.h index a180f6f..350bb1d 100644 --- a/miner/proxy/src/base/net/stratum/SubmitResult.h +++ b/miner/proxy/src/base/net/stratum/SubmitResult.h @@ -47,6 +47,8 @@ public: {} inline void done() { elapsed = Chrono::steadyMSecs() - m_start; } + // SECURITY FIX (HIGH-017): Public getter for timeout cleanup + inline uint64_t start() const { return m_start; } int64_t reqId = 0; int64_t seq = 0; diff --git a/miner/proxy/src/core/Controller.cpp b/miner/proxy/src/core/Controller.cpp index 3a361b2..5be8edd 100644 --- a/miner/proxy/src/core/Controller.cpp +++ b/miner/proxy/src/core/Controller.cpp @@ -66,7 +66,10 @@ void xmrig::Controller::start() { Base::start(); - proxy()->connect(); + // SECURITY FIX (MED-006): Null check before accessing proxy + if (m_proxy) { + proxy()->connect(); + } } @@ -82,14 +85,18 @@ void xmrig::Controller::stop() const xmrig::StatsData &xmrig::Controller::statsData() const { assert(m_proxy != nullptr && "Controller not initialized"); - return proxy()->statsData(); + // SECURITY FIX (MED-006): Return empty static object if proxy is null + static const StatsData empty; + return m_proxy ? proxy()->statsData() : empty; } const std::vector &xmrig::Controller::workers() const { assert(m_proxy != nullptr && "Controller not initialized"); - return proxy()->workers(); + // SECURITY FIX (MED-006): Return empty static object if proxy is null + static const std::vector empty; + return m_proxy ? proxy()->workers() : empty; } @@ -102,12 +109,25 @@ xmrig::Proxy *xmrig::Controller::proxy() const std::vector xmrig::Controller::miners() const { assert(m_proxy != nullptr && "Controller not initialized"); - return proxy()->miners(); + // SECURITY FIX (MED-006): Return empty vector if proxy is null + return m_proxy ? proxy()->miners() : std::vector(); } void xmrig::Controller::execCommand(char command) { + // SECURITY FIX (MED-006): Handle config commands even if proxy is null + if (command == 'v' || command == 'V') { + config()->toggleVerbose(); + LOG_NOTICE("%s " WHITE_BOLD("verbose: ") CYAN_BOLD("%d"), Tags::config(), config()->isVerbose()); + return; + } + + // All other commands require proxy + if (!m_proxy) { + return; + } + switch (command) { # ifdef APP_DEVEL case 's': @@ -116,12 +136,6 @@ void xmrig::Controller::execCommand(char command) break; # endif - case 'v': - case 'V': - config()->toggleVerbose(); - LOG_NOTICE("%s " WHITE_BOLD("verbose: ") CYAN_BOLD("%d"), Tags::config(), config()->isVerbose()); - break; - case 'h': case 'H': proxy()->printHashrate(); diff --git a/miner/proxy/src/core/config/Config.cpp b/miner/proxy/src/core/config/Config.cpp index a6fb29a..c30723b 100644 --- a/miner/proxy/src/core/config/Config.cpp +++ b/miner/proxy/src/core/config/Config.cpp @@ -159,7 +159,10 @@ void xmrig::Config::toggleVerbose() void xmrig::Config::setCustomDiff(uint64_t diff) { - if (diff >= 100 && diff < INT_MAX) { + // SECURITY FIX (MED-001): Use explicit uint64_t max instead of platform-dependent INT_MAX + // Maximum reasonable difficulty is 2^63-1 (prevents overflow in calculations) + constexpr uint64_t kMaxDiff = static_cast(1) << 63; + if (diff >= 100 && diff < kMaxDiff) { m_diff = diff; } } diff --git a/miner/proxy/src/proxy/Server.cpp b/miner/proxy/src/proxy/Server.cpp index 4649683..b3ef289 100644 --- a/miner/proxy/src/proxy/Server.cpp +++ b/miner/proxy/src/proxy/Server.cpp @@ -23,6 +23,8 @@ */ +#include + #include "proxy/Server.h" #include "base/io/log/Log.h" #include "base/tools/Handle.h" @@ -86,8 +88,10 @@ void xmrig::Server::create(uv_stream_t *server, int status) return; } - auto miner = new Miner(m_ctx, m_port, m_strictTls); + // SECURITY FIX (HIGH-027): Use std::nothrow so null check is meaningful + auto miner = new (std::nothrow) Miner(m_ctx, m_port, m_strictTls); if (!miner) { + LOG_ERR("[%s:%u] failed to allocate Miner object", m_host.data(), m_port); return; } diff --git a/miner/proxy/src/proxy/splitters/nicehash/NonceMapper.cpp b/miner/proxy/src/proxy/splitters/nicehash/NonceMapper.cpp index afcfa5a..7a638eb 100644 --- a/miner/proxy/src/proxy/splitters/nicehash/NonceMapper.cpp +++ b/miner/proxy/src/proxy/splitters/nicehash/NonceMapper.cpp @@ -263,17 +263,15 @@ void xmrig::NonceMapper::onVerifyAlgorithm(IStrategy *strategy, const IClient *c xmrig::SubmitCtx xmrig::NonceMapper::submitCtx(int64_t seq) { - if (!m_results.count(seq)) { + // SECURITY FIX (MED-008): Use single find() instead of count() + at() + find() + auto it = m_results.find(seq); + if (it == m_results.end()) { return {}; } - SubmitCtx ctx = m_results.at(seq); + SubmitCtx ctx = it->second; ctx.miner = m_storage->miner(ctx.minerId); - - auto it = m_results.find(seq); - if (it != m_results.end()) { - m_results.erase(it); - } + m_results.erase(it); return ctx; } diff --git a/miner/proxy/src/proxy/splitters/simple/SimpleSplitter.cpp b/miner/proxy/src/proxy/splitters/simple/SimpleSplitter.cpp index 9041a64..ee93840 100644 --- a/miner/proxy/src/proxy/splitters/simple/SimpleSplitter.cpp +++ b/miner/proxy/src/proxy/splitters/simple/SimpleSplitter.cpp @@ -105,19 +105,25 @@ void xmrig::SimpleSplitter::tick(uint64_t ticks) } m_released.clear(); + // SECURITY FIX (HIGH-020): Collect pointers before iterating to prevent + // iterator invalidation if tick() or stop() modifies m_upstreams + std::vector toTick; + toTick.reserve(m_upstreams.size()); + for (auto const &kv : m_upstreams) { if (kv.second->idleTime() > m_reuseTimeout) { m_released.push_back(kv.second); - continue; + } else { + toTick.push_back(kv.second); } - - kv.second->tick(ticks, now); } - if (m_released.empty()) { - return; + // Process ticks on collected pointers (safe from iterator invalidation) + for (SimpleMapper *mapper : toTick) { + mapper->tick(ticks, now); } + // Stop released mappers for (SimpleMapper *mapper : m_released) { stop(mapper); }