From daca391375bd13723ad30a0944891b455cc5799f Mon Sep 17 00:00:00 2001 From: snider Date: Wed, 31 Dec 2025 18:41:36 +0000 Subject: [PATCH] fix: Address 5 additional security findings in proxy (batch 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical fixes: - CRIT-009: Document lifetime constraint on LoginEvent references to prevent use-after-free (architectural fix requires larger refactor) - CRIT-015: Add proper libuv handle cleanup with uv_walk() before uv_loop_close() to prevent resource leaks High priority fixes: - HIGH-012: Log warning when LineReader truncates oversized lines - HIGH-013: Add division-by-zero check in Job::setTarget() for malformed target values - HIGH-016: Add MAX_UPSTREAMS limit (1000) in NonceSplitter to prevent unbounded memory growth under DoS conditions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- miner/proxy/src/App.cpp | 15 +++++++++++++++ miner/proxy/src/base/net/stratum/Job.cpp | 9 ++++++++- miner/proxy/src/base/net/tools/LineReader.cpp | 4 +++- miner/proxy/src/proxy/events/LoginEvent.h | 12 ++++++++++-- .../proxy/splitters/nicehash/NonceSplitter.cpp | 11 +++++++++++ 5 files changed, 47 insertions(+), 4 deletions(-) diff --git a/miner/proxy/src/App.cpp b/miner/proxy/src/App.cpp index 7a9ac36..7d5123f 100644 --- a/miner/proxy/src/App.cpp +++ b/miner/proxy/src/App.cpp @@ -76,6 +76,21 @@ int xmrig::App::exec() m_controller->start(); rc = uv_run(uv_default_loop(), UV_RUN_DEFAULT); + + // SECURITY FIX (CRIT-015): Properly clean up libuv handles before closing loop + // Run the loop again to process any pending close callbacks + uv_run(uv_default_loop(), UV_RUN_NOWAIT); + + // Walk all handles and close any that are still open + uv_walk(uv_default_loop(), [](uv_handle_t *handle, void *) { + if (!uv_is_closing(handle)) { + uv_close(handle, nullptr); + } + }, nullptr); + + // Run loop once more to process the close callbacks + uv_run(uv_default_loop(), UV_RUN_NOWAIT); + uv_loop_close(uv_default_loop()); return rc; diff --git a/miner/proxy/src/base/net/stratum/Job.cpp b/miner/proxy/src/base/net/stratum/Job.cpp index 1a7cfbf..9d0f4f2 100644 --- a/miner/proxy/src/base/net/stratum/Job.cpp +++ b/miner/proxy/src/base/net/stratum/Job.cpp @@ -119,7 +119,14 @@ bool xmrig::Job::setTarget(const char *target) switch (raw.size()) { case 4: - return 0xFFFFFFFFFFFFFFFFULL / (0xFFFFFFFFULL / uint64_t(*reinterpret_cast(raw.data()))); + { + // SECURITY FIX (HIGH-013): Check for division by zero + const uint64_t val = uint64_t(*reinterpret_cast(raw.data())); + if (val == 0) { + return 0; + } + return 0xFFFFFFFFFFFFFFFFULL / (0xFFFFFFFFULL / val); + } case 8: return *reinterpret_cast(raw.data()); diff --git a/miner/proxy/src/base/net/tools/LineReader.cpp b/miner/proxy/src/base/net/tools/LineReader.cpp index 07d0f1d..060a8d4 100644 --- a/miner/proxy/src/base/net/tools/LineReader.cpp +++ b/miner/proxy/src/base/net/tools/LineReader.cpp @@ -21,6 +21,7 @@ #include "base/net/tools/LineReader.h" #include "base/kernel/constants.h" #include "base/kernel/interfaces/ILineListener.h" +#include "base/io/log/Log.h" #include "base/net/tools/NetBuffer.h" #include @@ -57,7 +58,8 @@ void xmrig::LineReader::reset() void xmrig::LineReader::add(const char *data, size_t size) { if (size + m_pos > XMRIG_NET_BUFFER_CHUNK_SIZE) { - // it breaks correctness silently for long lines + // SECURITY FIX (HIGH-012): Log warning instead of silent truncation + LOG_WARN("LineReader: line exceeds buffer size (%zu bytes), data truncated", size + m_pos); return; } diff --git a/miner/proxy/src/proxy/events/LoginEvent.h b/miner/proxy/src/proxy/events/LoginEvent.h index 9f37048..d5b62b6 100644 --- a/miner/proxy/src/proxy/events/LoginEvent.h +++ b/miner/proxy/src/proxy/events/LoginEvent.h @@ -38,6 +38,14 @@ namespace xmrig { +/** + * IMPORTANT LIFETIME CONSTRAINT (CRIT-009): + * The `params` and `algorithms` references are only valid during synchronous event processing. + * Do NOT store these references or access them after the event handler returns. + * The underlying JSON document may be destroyed after event dispatch. + * + * If you need to retain parameter data, copy it during event handling. + */ class LoginEvent : public MinerEvent { public: @@ -47,9 +55,9 @@ public: } - const Algorithms &algorithms; + const Algorithms &algorithms; // WARNING: Only valid during event dispatch const int64_t loginId; - const rapidjson::Value ¶ms; + const rapidjson::Value ¶ms; // WARNING: Only valid during event dispatch int error = -1; String flow; diff --git a/miner/proxy/src/proxy/splitters/nicehash/NonceSplitter.cpp b/miner/proxy/src/proxy/splitters/nicehash/NonceSplitter.cpp index 59bc747..545ed9a 100644 --- a/miner/proxy/src/proxy/splitters/nicehash/NonceSplitter.cpp +++ b/miner/proxy/src/proxy/splitters/nicehash/NonceSplitter.cpp @@ -40,6 +40,10 @@ #define LABEL(x) " \x1B[01;30m" x ":\x1B[0m " +// SECURITY FIX (HIGH-016): Maximum number of upstream connections to prevent DoS +// Each upstream can handle 256 miners, so 1000 upstreams = 256,000 miners max +static constexpr size_t MAX_UPSTREAMS = 1000; + xmrig::NonceSplitter::NonceSplitter(Controller *controller) : Splitter(controller) { @@ -185,6 +189,13 @@ void xmrig::NonceSplitter::login(LoginEvent *event) } } + // SECURITY FIX (HIGH-016): Limit maximum upstreams to prevent DoS + if (m_upstreams.size() >= MAX_UPSTREAMS) { + LOG_ERR("Maximum upstream limit reached (%zu), rejecting miner", MAX_UPSTREAMS); + event->reject(); + return; + } + connect(); login(event); }