fix(mining): remediate mining-tab audit (22 findings) — crash-safety, async control, validation, math
Fixes all 22 confirmed findings from the mining-tab audit (10 Medium, 12 Low; 0 Critical/High), adversarially reviewed (6 follow-ups found + fixed, incl. the review-caught idle-auto-start bypass and a wrong benchmark-restore condition). Crash-safety & lifecycle: - M-04: join a stale/finished monitor thread in XmrigManager::start() and ~XmrigManager so an xmrig crash-then-restart (or quit) no longer std::terminate()s the wallet. - L-03/L-10: surface an unexpected miner exit once and clear the stale running flag. UI never blocks (M-03/L-06/L-08/L-09/L-13): pool start/stop now run on a dedicated serialized FIFO mining-control thread (joined before teardown), so the ~13 call sites don't block the render thread on stop()'s SIGTERM->SIGKILL->join; the spawn result marshals back to the UI. Miner-process / pool trust boundary: - M-01: validate the payout address (util::isValidRecipientAddress) at EVERY start path — the UI gate AND App::startPoolMining() (idle auto-start / thread scaling) — so a stale/wrong-chain address can't silently lose rewards. - M-09: SSRF guard skips the background pool-stats GET for loopback/private/link-local/single-label hosts. - M-02/L-02: cap the pool-stats + xmrig-API HTTP response bodies. - L-01: write the xmrig config 0600 at creation (POSIX open with mode) — no world/group-readable window. - M-10: reject shell-metacharacter binary paths before the version popen (excluding '()' so Program Files (x86) still works). Solo mining: M-06/M-08 clamp thread count to [1, cores] at the setgenerate/xmrig boundary; M-07 notify + don't lie on stop failure. Correctness: L-05 block-time constant 75->150s (chainparams); M-05 discloses pool-mode "Est. Daily" as a rough solo-equivalent; L-04/L-11/L-12 benchmark lifecycle (cancel on nav-away / mode-switch with restore, skip rebalance mid-benchmark); L-07 honor cancel mid-extract in both the xmrig and daemon updaters. Two new i18n keys back-filled across all 8 languages; CJK subset font rebuilt for the new glyphs. Verified across full-node, lite, and Windows builds; tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.8
@@ -89,8 +89,32 @@ static std::string getConfigDir() {
|
||||
// libcurl write callback
|
||||
static size_t curlWriteCb(void* ptr, size_t sz, size_t n, void* userdata) {
|
||||
auto* s = static_cast<std::string*>(userdata);
|
||||
s->append(static_cast<char*>(ptr), sz * n);
|
||||
return sz * n;
|
||||
const size_t add = sz * n;
|
||||
// Stats JSON (local xmrig HTTP API + pool API) is tiny; refuse an unbounded body from a hostile or
|
||||
// MITM'd endpoint so it can't grow this string until OOM. Returning < add aborts the transfer. (L-02)
|
||||
constexpr size_t kMaxStatsBytes = 1u << 20; // 1 MiB
|
||||
if (s->size() + add > kMaxStatsBytes) return 0;
|
||||
s->append(static_cast<char*>(ptr), add);
|
||||
return add;
|
||||
}
|
||||
|
||||
// True if `host` (already stripped of scheme+port) is a loopback/private/link-local/single-label target
|
||||
// that a public mining pool would never be — used to refuse a background stats GET to it (M-09).
|
||||
static bool hostLooksInternal(const std::string& host) {
|
||||
if (host.empty() || host == "localhost") return true;
|
||||
if (host.rfind("127.", 0) == 0 || host.rfind("10.", 0) == 0 ||
|
||||
host.rfind("192.168.", 0) == 0 || host.rfind("169.254.", 0) == 0) return true;
|
||||
if (host.rfind("172.", 0) == 0) { // 172.16.0.0 - 172.31.255.255
|
||||
const int second = std::atoi(host.c_str() + 4);
|
||||
if (second >= 16 && second <= 31) return true;
|
||||
}
|
||||
if (host.find(':') != std::string::npos) { // IPv6 literal: loopback / ULA / link-local
|
||||
if (host == "::1" || host.rfind("fc", 0) == 0 || host.rfind("fd", 0) == 0 ||
|
||||
host.rfind("fe80", 0) == 0) return true;
|
||||
}
|
||||
if (host.size() >= 6 && host.compare(host.size() - 6, 6, ".local") == 0) return true;
|
||||
if (host.find('.') == std::string::npos) return true; // bare single-label name = LAN/hosts, not a pool
|
||||
return false;
|
||||
}
|
||||
|
||||
// ============================================================================
|
||||
@@ -100,9 +124,14 @@ static size_t curlWriteCb(void* ptr, size_t sz, size_t n, void* userdata) {
|
||||
XmrigManager::XmrigManager() = default;
|
||||
|
||||
XmrigManager::~XmrigManager() {
|
||||
should_stop_ = true;
|
||||
if (isRunning()) {
|
||||
stop(3000);
|
||||
}
|
||||
// Join a monitor thread left joinable by an unexpected xmrig exit (State::Error, so isRunning() is
|
||||
// false and stop() above was skipped) — std::thread's destructor would otherwise std::terminate(). (M-04)
|
||||
if (monitor_thread_.joinable())
|
||||
monitor_thread_.join();
|
||||
}
|
||||
|
||||
// ============================================================================
|
||||
@@ -208,21 +237,41 @@ bool XmrigManager::generateConfig(const Config& cfg, const std::string& outPath)
|
||||
|
||||
try {
|
||||
fs::create_directories(fs::path(outPath).parent_path());
|
||||
const std::string dumped = j.dump(4);
|
||||
#ifndef _WIN32
|
||||
// Create the config 0600 AT CREATION (open with mode) so the API token + wallet address are never
|
||||
// in a world/group-readable file — even for a local attacker who opened it in the old
|
||||
// create-then-chmod window and held the fd open across the chmod. (L-01)
|
||||
int fd = ::open(outPath.c_str(), O_WRONLY | O_CREAT | O_TRUNC, 0600);
|
||||
if (fd < 0) {
|
||||
setLastError("Cannot write xmrig config: " + outPath);
|
||||
DEBUG_LOGF("[ERROR] XmrigManager: %s\n", last_error_.c_str());
|
||||
return false;
|
||||
}
|
||||
size_t off = 0;
|
||||
bool wrote = true;
|
||||
while (off < dumped.size()) {
|
||||
ssize_t nw = ::write(fd, dumped.data() + off, dumped.size() - off);
|
||||
if (nw <= 0) { wrote = false; break; }
|
||||
off += static_cast<size_t>(nw);
|
||||
}
|
||||
::close(fd);
|
||||
if (!wrote) {
|
||||
setLastError("Cannot write xmrig config: " + outPath);
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
#else
|
||||
std::ofstream ofs(outPath, std::ios::trunc);
|
||||
if (!ofs.is_open()) {
|
||||
setLastError("Cannot write xmrig config: " + outPath);
|
||||
DEBUG_LOGF("[ERROR] XmrigManager: %s\n", last_error_.c_str());
|
||||
return false;
|
||||
}
|
||||
#ifndef _WIN32
|
||||
// Restrict to owner (0600) BEFORE writing any secret material (API token, wallet
|
||||
// address, worker name). The file is still empty here, so the config is never
|
||||
// world-readable — closing the window between creation and the previous post-write chmod.
|
||||
chmod(outPath.c_str(), 0600);
|
||||
#endif
|
||||
ofs << j.dump(4);
|
||||
ofs << dumped;
|
||||
ofs.close();
|
||||
return true;
|
||||
#endif
|
||||
} catch (const std::exception& e) {
|
||||
setLastError(std::string("Config write error: ") + e.what());
|
||||
DEBUG_LOGF("[ERROR] XmrigManager: %s\n", last_error_.c_str());
|
||||
@@ -296,7 +345,11 @@ bool XmrigManager::start(const Config& cfg) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// Start monitor thread
|
||||
// Join a prior monitor thread before move-assigning: if xmrig exited unexpectedly, monitorProcess set
|
||||
// State::Error and returned, leaving monitor_thread_ joinable — move-assigning over a joinable
|
||||
// std::thread calls std::terminate() and aborts the whole wallet. (M-04)
|
||||
if (monitor_thread_.joinable())
|
||||
monitor_thread_.join();
|
||||
monitor_thread_ = std::thread(&XmrigManager::monitorProcess, this);
|
||||
state_ = State::Running;
|
||||
DEBUG_LOGF("[INFO] XmrigManager: started\n");
|
||||
@@ -783,6 +836,11 @@ void XmrigManager::fetchPoolApiStats() {
|
||||
// own API shape (pool.dragonx.is = custom /api/stats; pool.dragonx.cc = Miningcore
|
||||
// /api/pools); unknown/custom hosts fall back to the .is convention.
|
||||
const util::KnownPool* known = util::findKnownPoolByUrl(pool_host_);
|
||||
// SSRF guard: for an UNKNOWN (user-typed) pool host, don't let the wallet issue a background GET to a
|
||||
// loopback/private/link-local/single-label target — those aren't public mining pools, and a
|
||||
// paste-a-pool-config lure could otherwise point us at an internal host. Known pools use their trusted
|
||||
// registry statsUrl and are exempt. (M-09)
|
||||
if (!known && hostLooksInternal(pool_host_)) return;
|
||||
const std::string url = known ? known->statsUrl
|
||||
: ("https://" + pool_host_ + "/api/stats");
|
||||
|
||||
@@ -861,7 +919,14 @@ void XmrigManager::startVersionDetection()
|
||||
std::thread([]() {
|
||||
const std::string bin = findXmrigBinary();
|
||||
std::string ver;
|
||||
if (!bin.empty()) {
|
||||
// Don't hand a path containing shell/cmd metacharacters to popen()'s shell — bin is normally an
|
||||
// app-controlled path, but this closes command injection if it ever isn't. (M-10)
|
||||
// Reject only chars that stay shell-special INSIDE the double-quotes we wrap bin in ("\"" + bin + "\"")
|
||||
// on cmd.exe or /bin/sh. Parens are inert when quoted, so they're excluded — otherwise common Windows
|
||||
// paths like "C:\Program Files (x86)\..." would be rejected and version detection would silently fail. (M-10)
|
||||
const bool binShellSafe =
|
||||
!bin.empty() && bin.find_first_of("\"'`$;&|<>^%\n\r") == std::string::npos;
|
||||
if (binShellSafe) {
|
||||
const std::string cmd = "\"" + bin + "\" --version 2>&1";
|
||||
#ifdef _WIN32
|
||||
FILE* fp = _popen(cmd.c_str(), "r");
|
||||
|
||||
Reference in New Issue
Block a user