diff --git a/miner/proxy/src/base/net/stratum/Client.cpp b/miner/proxy/src/base/net/stratum/Client.cpp index b412a54..b86819e 100644 --- a/miner/proxy/src/base/net/stratum/Client.cpp +++ b/miner/proxy/src/base/net/stratum/Client.cpp @@ -812,7 +812,11 @@ void xmrig::Client::parseResponse(int64_t id, const rapidjson::Value &result, co } if (error.IsObject()) { - const char *message = error["message"].GetString(); + // SECURITY FIX (CRIT-011): Validate "message" field exists and is a string before accessing + const char *message = Json::getString(error, "message"); + if (!message) { + message = "unknown error"; + } if (!handleSubmitResponse(id, message) && !isQuiet()) { LOG_ERR("%s " RED("error: ") RED_BOLD("\"%s\"") RED(", code: ") RED_BOLD("%d"), tag(), message, Json::getInt(error, "code")); diff --git a/miner/proxy/src/base/net/stratum/Tls.cpp b/miner/proxy/src/base/net/stratum/Tls.cpp index 2a1ad1e..60905a4 100644 --- a/miner/proxy/src/base/net/stratum/Tls.cpp +++ b/miner/proxy/src/base/net/stratum/Tls.cpp @@ -45,6 +45,10 @@ xmrig::Client::Tls::Tls(Client *client) : m_write = BIO_new(BIO_s_mem()); m_read = BIO_new(BIO_s_mem()); SSL_CTX_set_options(m_ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3); + + // SECURITY FIX (CRIT-008): Enable peer certificate verification + // When no fingerprint is configured, this ensures we at least validate the certificate chain + SSL_CTX_set_verify(m_ctx, SSL_VERIFY_PEER, nullptr); } diff --git a/miner/proxy/src/base/net/tls/TlsGen.cpp b/miner/proxy/src/base/net/tls/TlsGen.cpp index ad2e648..5bafaab 100644 --- a/miner/proxy/src/base/net/tls/TlsGen.cpp +++ b/miner/proxy/src/base/net/tls/TlsGen.cpp @@ -24,6 +24,11 @@ #include #include +// SECURITY FIX (HIGH-002): Include for setting file permissions +#ifndef _WIN32 +# include +#endif + namespace xmrig { @@ -137,6 +142,11 @@ bool xmrig::TlsGen::write() return false; } + // SECURITY FIX (HIGH-002): Set restrictive permissions on private key file (owner read/write only) +# ifndef _WIN32 + chmod(m_certKey, S_IRUSR | S_IWUSR); // 0600 +# endif + auto x509_file = fopen(m_cert, "wb"); if (!x509_file) { return false; diff --git a/miner/proxy/src/core/config/ConfigTransform.cpp b/miner/proxy/src/core/config/ConfigTransform.cpp index 0d99b82..2f7d755 100644 --- a/miner/proxy/src/core/config/ConfigTransform.cpp +++ b/miner/proxy/src/core/config/ConfigTransform.cpp @@ -27,6 +27,9 @@ #include "base/kernel/interfaces/IConfig.h" #include "proxy/BindHost.h" +#include +#include + namespace xmrig { @@ -82,7 +85,16 @@ void xmrig::ConfigTransform::transform(rapidjson::Document &doc, int key, const case IConfig::CustomDiffKey: /* --custom-diff */ case IConfig::ReuseTimeoutKey: /* --reuse-timeout */ - return transformUint64(doc, key, static_cast(strtol(arg, nullptr, 10))); + { + // SECURITY FIX (HIGH-004): Validate strtol result for errors and overflow + char *endptr = nullptr; + errno = 0; + const long val = strtol(arg, &endptr, 10); + if (endptr == arg || errno == ERANGE || val < 0) { + return; // Invalid input, skip silently + } + return transformUint64(doc, key, static_cast(val)); + } case IConfig::LoginFileKey: /* --login-file */ return set(doc, "login-file", arg); diff --git a/miner/proxy/src/proxy/Events.cpp b/miner/proxy/src/proxy/Events.cpp index b72245a..43defef 100644 --- a/miner/proxy/src/proxy/Events.cpp +++ b/miner/proxy/src/proxy/Events.cpp @@ -22,13 +22,16 @@ */ +#include + #include "base/io/log/Log.h" #include "proxy/Events.h" namespace xmrig { -bool Events::m_ready = true; +// THREAD SAFETY FIX (CRIT-013): Use atomic for thread-safe reentrancy guard +std::atomic Events::m_ready{true}; std::map > Events::m_listeners; } @@ -36,13 +39,13 @@ std::map > Events::m_listeners; bool xmrig::Events::exec(IEvent *event) { - if (!m_ready) { + // THREAD SAFETY FIX (CRIT-013): Use atomic compare-and-swap for thread-safe guard + bool expected = true; + if (!m_ready.compare_exchange_strong(expected, false)) { LOG_ERR("failed start event %d", (int) event->type()); return false; } - m_ready = false; - std::vector &listeners = m_listeners[event->type()]; for (IEventListener *listener : listeners) { event->isRejected() ? listener->onRejectedEvent(event) : listener->onEvent(event); @@ -51,7 +54,7 @@ bool xmrig::Events::exec(IEvent *event) const bool rejected = event->isRejected(); event->~IEvent(); - m_ready = true; + m_ready.store(true); return !rejected; } diff --git a/miner/proxy/src/proxy/Events.h b/miner/proxy/src/proxy/Events.h index 4e8ef9e..ae879df 100644 --- a/miner/proxy/src/proxy/Events.h +++ b/miner/proxy/src/proxy/Events.h @@ -26,6 +26,7 @@ #define XMRIG_EVENTS_H +#include #include #include @@ -45,7 +46,8 @@ public: static void subscribe(IEvent::Type type, IEventListener *listener); private: - static bool m_ready; + // THREAD SAFETY FIX (CRIT-013): Use atomic for thread-safe reentrancy guard + static std::atomic m_ready; static std::map > m_listeners; }; diff --git a/miner/proxy/src/proxy/Miner.cpp b/miner/proxy/src/proxy/Miner.cpp index fa161f6..7d0da0d 100644 --- a/miner/proxy/src/proxy/Miner.cpp +++ b/miner/proxy/src/proxy/Miner.cpp @@ -351,8 +351,19 @@ void xmrig::Miner::parse(char *line, size_t len) return shutdown(true); } + // SECURITY FIX (HIGH-018): Validate all JSON fields before accessing + if (!doc.HasMember("id") || !doc.HasMember("method")) { + return shutdown(true); + } + const rapidjson::Value &id = doc["id"]; - if (id.IsInt64() && parseRequest(id.GetInt64(), doc["method"].GetString(), doc["params"])) { + const rapidjson::Value &method = doc["method"]; + + if (!id.IsInt64() || !method.IsString()) { + return shutdown(true); + } + + if (parseRequest(id.GetInt64(), method.GetString(), doc.HasMember("params") ? doc["params"] : rapidjson::Value())) { return; } diff --git a/miner/proxy/src/proxy/splitters/extra_nonce/ExtraNonceSplitter.cpp b/miner/proxy/src/proxy/splitters/extra_nonce/ExtraNonceSplitter.cpp index 8928c1c..dec40eb 100644 --- a/miner/proxy/src/proxy/splitters/extra_nonce/ExtraNonceSplitter.cpp +++ b/miner/proxy/src/proxy/splitters/extra_nonce/ExtraNonceSplitter.cpp @@ -63,6 +63,10 @@ xmrig::ExtraNonceSplitter::~ExtraNonceSplitter() xmrig::Upstreams xmrig::ExtraNonceSplitter::upstreams() const { + // SECURITY FIX (CRIT-010): Add null check to prevent segfault before connect() + if (!m_upstream) { + return { 0U, 0U, 0U }; + } return { m_upstream->isActive() ? 1U : 0U, m_upstream->isSuspended() ? 1U : 0U, 1U }; } @@ -76,7 +80,10 @@ void xmrig::ExtraNonceSplitter::connect() void xmrig::ExtraNonceSplitter::gc() { - m_upstream->gc(); + // SECURITY FIX (CRIT-010): Add null check + if (m_upstream) { + m_upstream->gc(); + } } @@ -95,6 +102,10 @@ void xmrig::ExtraNonceSplitter::printConnections() void xmrig::ExtraNonceSplitter::tick(uint64_t ticks) { + // SECURITY FIX (CRIT-010): Add null check + if (!m_upstream) { + return; + } const uint64_t now = uv_now(uv_default_loop()); m_upstream->tick(ticks, now); } @@ -103,14 +114,18 @@ void xmrig::ExtraNonceSplitter::tick(uint64_t ticks) #ifdef APP_DEVEL void xmrig::ExtraNonceSplitter::printState() { - m_upstream->printState(); + // SECURITY FIX (CRIT-010): Add null check + if (m_upstream) { + m_upstream->printState(); + } } #endif void xmrig::ExtraNonceSplitter::onConfigChanged(Config *config, Config *previousConfig) { - if (config->pools() != previousConfig->pools()) { + // SECURITY FIX (CRIT-010): Add null check + if (m_upstream && config->pools() != previousConfig->pools()) { config->pools().print(); m_upstream->reload(config->pools()); } diff --git a/miner/proxy/src/proxy/splitters/extra_nonce/ExtraNonceStorage.cpp b/miner/proxy/src/proxy/splitters/extra_nonce/ExtraNonceStorage.cpp index 3a0a06a..a32430a 100644 --- a/miner/proxy/src/proxy/splitters/extra_nonce/ExtraNonceStorage.cpp +++ b/miner/proxy/src/proxy/splitters/extra_nonce/ExtraNonceStorage.cpp @@ -22,6 +22,8 @@ * along with this program. If not, see . */ +#include + #include "base/io/log/Log.h" #include "proxy/Counters.h" #include "proxy/Miner.h" @@ -34,7 +36,13 @@ bool xmrig::ExtraNonceStorage::add(Miner *miner) if (isActive()) { miner->setJob(m_job, m_extraNonce); - ++m_extraNonce; + // SECURITY FIX (HIGH-007): Handle overflow with wrap-around + if (m_extraNonce == INT64_MAX) { + m_extraNonce = 0; + LOG_WARN("ExtraNonce overflow, wrapping to 0"); + } else { + ++m_extraNonce; + } } return true; @@ -96,7 +104,13 @@ void xmrig::ExtraNonceStorage::setJob(const Job &job) for (const auto& m : m_miners) { m.second->setJob(m_job, m_extraNonce); - ++m_extraNonce; + // SECURITY FIX (HIGH-007): Handle overflow with wrap-around + if (m_extraNonce == INT64_MAX) { + m_extraNonce = 0; + LOG_WARN("ExtraNonce overflow during setJob, wrapping to 0"); + } else { + ++m_extraNonce; + } } } diff --git a/miner/proxy/src/proxy/splitters/nicehash/NonceSplitter.cpp b/miner/proxy/src/proxy/splitters/nicehash/NonceSplitter.cpp index 407426d..59bc747 100644 --- a/miner/proxy/src/proxy/splitters/nicehash/NonceSplitter.cpp +++ b/miner/proxy/src/proxy/splitters/nicehash/NonceSplitter.cpp @@ -196,7 +196,13 @@ void xmrig::NonceSplitter::remove(Miner *miner) return; } - m_upstreams[miner->mapperId()]->remove(miner); + // SECURITY FIX (CRIT-018): Add bounds check before vector access + const auto mapperId = static_cast(miner->mapperId()); + if (mapperId >= m_upstreams.size()) { + return; + } + + m_upstreams[mapperId]->remove(miner); } @@ -206,5 +212,11 @@ void xmrig::NonceSplitter::submit(SubmitEvent *event) return; } - m_upstreams[event->miner()->mapperId()]->submit(event); + // SECURITY FIX (CRIT-018): Add bounds check before vector access + const auto mapperId = static_cast(event->miner()->mapperId()); + if (mapperId >= m_upstreams.size()) { + return; + } + + m_upstreams[mapperId]->submit(event); }