fix: Address 5 additional security findings in proxy (batch 3)

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 <noreply@anthropic.com>
This commit is contained in:
snider 2025-12-31 18:41:36 +00:00
parent 3d8423f6e1
commit daca391375
5 changed files with 47 additions and 4 deletions

View file

@ -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;

View file

@ -119,7 +119,14 @@ bool xmrig::Job::setTarget(const char *target)
switch (raw.size()) {
case 4:
return 0xFFFFFFFFFFFFFFFFULL / (0xFFFFFFFFULL / uint64_t(*reinterpret_cast<const uint32_t *>(raw.data())));
{
// SECURITY FIX (HIGH-013): Check for division by zero
const uint64_t val = uint64_t(*reinterpret_cast<const uint32_t *>(raw.data()));
if (val == 0) {
return 0;
}
return 0xFFFFFFFFFFFFFFFFULL / (0xFFFFFFFFULL / val);
}
case 8:
return *reinterpret_cast<const uint64_t *>(raw.data());

View file

@ -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 <cassert>
@ -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;
}

View file

@ -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 &params;
const rapidjson::Value &params; // WARNING: Only valid during event dispatch
int error = -1;
String flow;

View file

@ -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);
}