From d16bbf3695553df283bec2a46559a39f3f940f21 Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 19 Oct 2026 11:39:38 -0500 Subject: [PATCH] security: payment-URI amount validation, export filename sanitize, chat crash-guard, HTTPS-only, conf 0600, pool auto-lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six bounded defensive fixes: - Payment-URI amount is now strictly validated (reject NaN/inf/sign/garbage, <=8 decimals, 0..21,000,000) at parse time, so a crafted QR/URI can't inject a bogus amount into the Send form. The z_sendmany call the user confirms is unchanged; this is input validation only. - export-all-keys filename is sanitized (basename-only, no traversal) via a shared util::sanitizeExportFilename (factored out of the CSV export dialog). - Chat append()/upsert() wrap the strict serialize in try/catch so a malformed (invalid-UTF-8, attacker-influenceable) body is skipped, not crashed on — matching appendBatch's existing drop behavior. - http_download (used by the updaters + pool stats) restricts transfers AND redirects to HTTPS and bounds the download path's redirects, so a redirect can't downgrade to http:// or another scheme. - The default DRGX.conf (rpcuser/rpcpassword) is written 0600 from the start via writeFileAtomically, closing the brief world-readable ofstream+chmod window. - Auto-lock is suppressed only for SOLO mining (needs the wallet), not pool mining (pays an external address), which was holding the wallet open forever. Co-Authored-By: Claude Opus 4.8 --- src/app_security.cpp | 11 ++-- src/chat/chat_database.cpp | 28 +++++++-- src/rpc/connection.cpp | 27 +++----- src/ui/windows/export_all_keys_dialog.cpp | 8 ++- src/ui/windows/export_transactions_dialog.cpp | 28 +-------- src/util/export_filename.h | 62 +++++++++++++++++++ src/util/http_download.cpp | 17 +++++ src/util/payment_uri.cpp | 35 +++++++++-- tests/test_phase4.cpp | 12 +++- 9 files changed, 165 insertions(+), 63 deletions(-) create mode 100644 src/util/export_filename.h diff --git a/src/app_security.cpp b/src/app_security.cpp index b880e69..ed74904 100644 --- a/src/app_security.cpp +++ b/src/app_security.cpp @@ -791,12 +791,11 @@ void App::checkAutoLock() { if (capture_mode_) return; // don't auto-lock while a UI sweep forces the encrypted/locked state if (!state_.isEncrypted() || state_.isLocked()) return; - // Don't auto-lock while mining — mining is a long-running intentional - // operation and locking the wallet on the daemon side stops solo mining. - bool miningActive = state_.mining.generate - || (xmrig_manager_ && xmrig_manager_->isRunning()); - if (miningActive) { - // Keep resetting the idle timer so we don't lock the instant mining stops + // Don't auto-lock while SOLO mining — it runs in the daemon and locking the wallet there would stop + // it (and it shields matured rewards, which needs the wallet). Pool mining pays an external address + // and needs no unlocked wallet, so it must NOT hold the wallet open indefinitely. + if (state_.mining.generate) { + // Keep resetting the idle timer so we don't lock the instant solo mining stops. idle_.noteActivity(); return; } diff --git a/src/chat/chat_database.cpp b/src/chat/chat_database.cpp index 12a9873..08b91b8 100644 --- a/src/chat/chat_database.cpp +++ b/src/chat/chat_database.cpp @@ -108,8 +108,18 @@ bool ChatDatabase::append(const ChatMessage& message) std::vector nonce; std::vector cipher; - std::string plain = serialize(message); // full plaintext (decrypted body + metadata) - const bool encrypted = encrypt(plain, nonce, cipher); + std::string plain; + bool encrypted = false; + try { + // serialize() dumps strictly, so an invalid-UTF-8 (attacker-influenceable) body THROWS here; + // skip the row instead of letting it escape the write path and crash (appendBatch already does). + plain = serialize(message); + encrypted = encrypt(plain, nonce, cipher); + } catch (const std::exception& e) { + if (!plain.empty()) sodium_memzero(&plain[0], plain.size()); + DEBUG_LOGF("[chat] append skipped a malformed message: %s\n", e.what()); + return false; + } if (!plain.empty()) sodium_memzero(&plain[0], plain.size()); // don't leave it on the heap if (!encrypted) return false; @@ -187,8 +197,16 @@ bool ChatDatabase::upsert(const ChatMessage& message) std::vector nonce; std::vector cipher; - std::string plain = serialize(message); - const bool encrypted = encrypt(plain, nonce, cipher); + std::string plain; + bool encrypted = false; + try { + plain = serialize(message); // strict dump → a malformed body throws; skip rather than crash + encrypted = encrypt(plain, nonce, cipher); + } catch (const std::exception& e) { + if (!plain.empty()) sodium_memzero(&plain[0], plain.size()); + DEBUG_LOGF("[chat] upsert skipped a malformed message: %s\n", e.what()); + return false; + } if (!plain.empty()) sodium_memzero(&plain[0], plain.size()); if (!encrypted) return false; @@ -416,7 +434,7 @@ std::string ChatDatabase::serialize(const ChatMessage& message) const json["ts"] = message.timestamp; json["pos"] = static_cast(message.payload_position); json["dl"] = static_cast(message.delivery); - return json.dump(); + return json.dump(); // strict: an invalid-UTF-8 body throws and is DROPPED by the callers (contract) } bool ChatDatabase::deserialize(const std::string& json, ChatMessage& out) const diff --git a/src/rpc/connection.cpp b/src/rpc/connection.cpp index 5b0ecc4..4b26356 100644 --- a/src/rpc/connection.cpp +++ b/src/rpc/connection.cpp @@ -460,15 +460,14 @@ bool Connection::createDefaultConfig(const std::string& path) return false; } - std::ofstream file(path); - if (!file.is_open()) { - DEBUG_LOGF("Failed to create config file: %s\n", path.c_str()); - return false; - } - + // Build the content in memory, then write it 0600 from the start (writeFileAtomically with + // restrictPermissions) — the old ofstream+chmod left a brief world-readable window during which + // another local user could read the freshly-generated rpcuser/rpcpassword. + std::ostringstream file; + // Get the data directory for exportdir std::string dataDir = getDefaultDataDir(); - + file << "# DragonX configuration file\n"; file << "# Auto-generated by DragonX Wallet\n"; file << "\n"; @@ -485,18 +484,10 @@ bool Connection::createDefaultConfig(const std::string& path) file << "addnode=seed.dragonx.is\n"; file << "addnode=node1.dragonx.is\n"; file << "addnode=node5.dragonx.is\n"; - - file.close(); - // The file holds the freshly-generated rpcuser/rpcpassword in plaintext. ofstream creates it - // with the process umask (typically world-readable 0644), so restrict it to owner read/write - // before another local user can read the credentials. - { - namespace fs = std::filesystem; - std::error_code ec; - fs::permissions(path, fs::perms::owner_read | fs::perms::owner_write, - fs::perm_options::replace, ec); - if (ec) DEBUG_LOGF("Could not restrict config permissions on %s: %s\n", path.c_str(), ec.message().c_str()); + if (!util::Platform::writeFileAtomically(path, file.str(), /*restrictPermissions=*/true)) { + DEBUG_LOGF("Failed to create config file: %s\n", path.c_str()); + return false; } DEBUG_LOGF("Created default config file: %s\n", path.c_str()); diff --git a/src/ui/windows/export_all_keys_dialog.cpp b/src/ui/windows/export_all_keys_dialog.cpp index d6d4ac3..c72e878 100644 --- a/src/ui/windows/export_all_keys_dialog.cpp +++ b/src/ui/windows/export_all_keys_dialog.cpp @@ -9,6 +9,7 @@ #include "../../rpc/rpc_worker.h" #include "../../util/i18n.h" #include "../../util/platform.h" +#include "../../util/export_filename.h" #include "../notifications.h" #include "../schema/ui_schema.h" #include "../material/draw_helpers.h" @@ -160,8 +161,11 @@ void ExportAllKeysDialog::render(App* app) if (s_include_t) { for (const auto& a : state.t_addresses) t_addrs.push_back(a.address); } - std::string filename(s_filename); - + // Sanitize the typed filename (basename only, no traversal) and reflect it back in the + // buffer so the user sees exactly what gets written. Mirrors the CSV export dialog. + std::string filename = util::sanitizeExportFilename(s_filename, ".txt", "dragonx_keys.txt"); + snprintf(s_filename, sizeof(s_filename), "%s", filename.c_str()); + // Run all key exports on worker thread if (app->worker()) { app->worker()->post([rpc = app->rpc(), z_addrs, t_addrs, filename]() -> rpc::RPCWorker::MainCb { diff --git a/src/ui/windows/export_transactions_dialog.cpp b/src/ui/windows/export_transactions_dialog.cpp index 5a70d61..761ce28 100644 --- a/src/ui/windows/export_transactions_dialog.cpp +++ b/src/ui/windows/export_transactions_dialog.cpp @@ -6,6 +6,7 @@ #include "../../app.h" #include "../../util/i18n.h" #include "../../util/platform.h" +#include "../../util/export_filename.h" #include "../../data/transaction_csv.h" #include "transactions_tab.h" #include "../notifications.h" @@ -49,31 +50,6 @@ static bool parseYmd(const char* s, std::int64_t& outStartOfDay) { return true; } -// Make a user-typed filename safe to write into the config dir: basename only, illegal chars → '_', -// no leading dots / trailing dots-or-spaces, length-capped, always ending in .csv. -static std::string sanitizeExportFilename(std::string name) { - const size_t slash = name.find_last_of("/\\"); - if (slash != std::string::npos) name = name.substr(slash + 1); - auto notspace = [](unsigned char c) { return !std::isspace(c); }; - name.erase(name.begin(), std::find_if(name.begin(), name.end(), notspace)); - name.erase(std::find_if(name.rbegin(), name.rend(), notspace).base(), name.end()); - for (char& c : name) { - const unsigned char u = static_cast(c); - if (u < 0x20 || c == '<' || c == '>' || c == ':' || c == '"' || c == '|' || c == '?' || c == '*') - c = '_'; - } - const size_t firstGood = name.find_first_not_of('.'); - if (firstGood == std::string::npos) name.clear(); else name.erase(0, firstGood); - while (!name.empty() && (name.back() == '.' || name.back() == ' ')) name.pop_back(); - if (name.size() > 120) name.resize(120); - std::string lower = name; - std::transform(lower.begin(), lower.end(), lower.begin(), - [](unsigned char c) { return static_cast(std::tolower(c)); }); - if (lower.size() < 4 || lower.compare(lower.size() - 4, 4, ".csv") != 0) name += ".csv"; - if (name.empty() || name == ".csv") name = "dragonx_transactions.csv"; - return name; -} - void ExportTransactionsDialog::show() { s_open = true; @@ -226,7 +202,7 @@ void ExportTransactionsDialog::render(App* app) std::string configDir = util::Platform::getConfigDir(); // Sanitize in-buffer so the user sees exactly what was written (and we can't be pointed outside // the config dir by a pasted path). - std::string safeName = sanitizeExportFilename(s_filename); + std::string safeName = util::sanitizeExportFilename(s_filename, ".csv", "dragonx_transactions.csv"); snprintf(s_filename, sizeof(s_filename), "%s", safeName.c_str()); std::string filepath = configDir + "/" + safeName; // Build the chosen subset (scope 0 exports the whole list with no copy), then write atomically diff --git a/src/util/export_filename.h b/src/util/export_filename.h new file mode 100644 index 0000000..f68ba57 --- /dev/null +++ b/src/util/export_filename.h @@ -0,0 +1,62 @@ +// DragonX Wallet - ImGui Edition +// Copyright 2024-2026 The Hush Developers +// Released under the GPLv3 +#pragma once + +#include +#include +#include + +namespace dragonx { +namespace util { + +// Make a user-typed filename safe to write into a trusted config/export directory: basename only +// (no path traversal), illegal / control chars -> '_', no leading dots or trailing dots-or-spaces, +// Windows reserved device stems prefixed with '_', length-capped, and always ending in requiredExt +// (e.g. ".csv"). Returns `fallback` if nothing usable remains. Header-only so every export dialog +// shares one implementation. +inline std::string sanitizeExportFilename(std::string name, const char* requiredExt, const char* fallback) { + const std::size_t slash = name.find_last_of("/\\"); + if (slash != std::string::npos) name = name.substr(slash + 1); + auto notSpace = [](unsigned char c) { return !std::isspace(c); }; + name.erase(name.begin(), std::find_if(name.begin(), name.end(), notSpace)); + name.erase(std::find_if(name.rbegin(), name.rend(), notSpace).base(), name.end()); + for (char& c : name) { + const unsigned char u = static_cast(c); + if (u < 0x20 || c == '<' || c == '>' || c == ':' || c == '"' || c == '|' || + c == '?' || c == '*' || c == '/' || c == '\\') + c = '_'; + } + const std::size_t firstGood = name.find_first_not_of('.'); + if (firstGood == std::string::npos) name.clear(); else name.erase(0, firstGood); + while (!name.empty() && (name.back() == '.' || name.back() == ' ')) name.pop_back(); + if (name.size() > 120) name.resize(120); + // Windows reserved device names (case-insensitive, checked on the stem before the first dot). + { + std::string stem = name; + const std::size_t dot = stem.find('.'); + if (dot != std::string::npos) stem = stem.substr(0, dot); + std::transform(stem.begin(), stem.end(), stem.begin(), + [](unsigned char c) { return static_cast(std::toupper(c)); }); + static const char* const kReserved[] = { + "CON", "PRN", "AUX", "NUL", + "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", + "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", + }; + for (const char* r : kReserved) if (stem == r) { name = "_" + name; break; } + } + std::string lower = name; + std::transform(lower.begin(), lower.end(), lower.begin(), + [](unsigned char c) { return static_cast(std::tolower(c)); }); + std::string lext = requiredExt ? requiredExt : ""; + std::transform(lext.begin(), lext.end(), lext.begin(), + [](unsigned char c) { return static_cast(std::tolower(c)); }); + if (!lext.empty() && + (lower.size() < lext.size() || lower.compare(lower.size() - lext.size(), lext.size(), lext) != 0)) + name += requiredExt; + if (name.empty() || (requiredExt && name == requiredExt)) name = fallback ? fallback : "export"; + return name; +} + +} // namespace util +} // namespace dragonx diff --git a/src/util/http_download.cpp b/src/util/http_download.cpp index a96d4a2..74b3bb2 100644 --- a/src/util/http_download.cpp +++ b/src/util/http_download.cpp @@ -60,6 +60,20 @@ int cancelXferCb(void* clientp, curl_off_t, curl_off_t, curl_off_t, curl_off_t) return static_cast*>(clientp)->load(std::memory_order_relaxed) ? 1 : 0; } +// Restrict the transfer AND any redirect to HTTPS so a redirect can't silently downgrade to http://, +// file://, ftp:// or an unexpected scheme. Every caller of these shared helpers (daemon/xmrig updaters, +// pool stats) uses https URLs. (Verify-peer/host stay on at the call sites.) +void restrictToHttps(CURL* curl) +{ +#if LIBCURL_VERSION_NUM >= 0x075500 // 7.85.0: the string-form protocol setopts + curl_easy_setopt(curl, CURLOPT_PROTOCOLS_STR, "https"); + curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS_STR, "https"); +#else + curl_easy_setopt(curl, CURLOPT_PROTOCOLS, CURLPROTO_HTTPS); + curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTPS); +#endif +} + } // namespace HttpGetResult httpGet(const std::string& url, const char* logTag, const HttpGetOptions& opt) @@ -78,6 +92,7 @@ HttpGetResult httpGet(const std::string& url, const char* logTag, const HttpGetO curl_easy_setopt(curl, CURLOPT_WRITEDATA, &sink); curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L); curl_easy_setopt(curl, CURLOPT_MAXREDIRS, opt.maxRedirects); + restrictToHttps(curl); curl_easy_setopt(curl, CURLOPT_USERAGENT, "ObsidianDragon/1.0"); curl_easy_setopt(curl, CURLOPT_TIMEOUT, opt.timeoutSec); curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, opt.connectTimeoutSec); @@ -131,6 +146,8 @@ bool httpDownloadToFile(const std::string& url, const std::string& destPath, curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writeFileCb); curl_easy_setopt(curl, CURLOPT_WRITEDATA, fp); curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L); + curl_easy_setopt(curl, CURLOPT_MAXREDIRS, 5L); // bound redirects (this path set none before) + restrictToHttps(curl); curl_easy_setopt(curl, CURLOPT_USERAGENT, "ObsidianDragon/1.0"); curl_easy_setopt(curl, CURLOPT_TIMEOUT, 0L); curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, 30L); diff --git a/src/util/payment_uri.cpp b/src/util/payment_uri.cpp index bb835d3..782f8d7 100644 --- a/src/util/payment_uri.cpp +++ b/src/util/payment_uri.cpp @@ -10,6 +10,29 @@ #include #include #include +#include + +namespace { +// Strictly parse a decimal DRGX amount from an (attacker-influenceable) payment URI / QR: digits with at +// most one '.', at most 8 fractional digits, in [0, 21000000]. Rejects empty, NaN/inf, signs, whitespace +// and trailing garbage that std::stod would otherwise accept — so a crafted URI can't inject a bogus +// amount into the Send form. +bool parseStrictAmount(const std::string& s, double& out) { + if (s.empty() || s.size() > 32) return false; + int dots = 0, digits = 0, frac = 0; bool afterDot = false; + for (char c : s) { + if (c == '.') { if (++dots > 1) return false; afterDot = true; } + else if (c >= '0' && c <= '9') { ++digits; if (afterDot && ++frac > 8) return false; } + else return false; + } + if (digits == 0) return false; + double v = 0.0; + try { v = std::stod(s); } catch (...) { return false; } + if (!std::isfinite(v) || v < 0.0 || v > 21000000.0) return false; + out = v; + return true; +} +} // namespace namespace dragonx { namespace util { @@ -141,11 +164,13 @@ PaymentURI parsePaymentURI(const std::string& uri) std::transform(key.begin(), key.end(), key.begin(), ::tolower); if (key == "amount" || key == "amt") { - try { - result.amount = std::stod(value); - } catch (...) { - // Invalid amount, keep as 0 + double a = 0.0; + if (!parseStrictAmount(value, a)) { + result.error = "Invalid amount"; + result.valid = false; + return result; // reject the whole URI rather than silently treating it as 0 } + result.amount = a; } else if (key == "label") { result.label = value; } else if (key == "memo") { @@ -174,7 +199,7 @@ PaymentURI parsePaymentURI(const std::string& uri) } // Validate amount if present - if (result.amount < 0) { + if (!std::isfinite(result.amount) || result.amount < 0 || result.amount > 21000000.0) { result.error = "Invalid negative amount"; return result; } diff --git a/tests/test_phase4.cpp b/tests/test_phase4.cpp index 36568fe..b2623c9 100644 --- a/tests/test_phase4.cpp +++ b/tests/test_phase4.cpp @@ -1033,7 +1033,17 @@ void testPaymentUri() auto invalid = dragonx::util::parsePaymentURI("drgx:" + taddr + "?amount=-1"); EXPECT_FALSE(invalid.valid); - EXPECT_EQ(invalid.error, std::string("Invalid negative amount")); + EXPECT_EQ(invalid.error, std::string("Invalid amount")); // strict parse rejects '-' up front now + + // Crafted amounts that std::stod would have accepted are now rejected before the Send form. + for (const char* bad : {"nan", "inf", "1e3", "0x10", "1.5abc", "0.000000001", "21000001", ""}) { + auto r = dragonx::util::parsePaymentURI("drgx:" + taddr + "?amount=" + bad); + EXPECT_FALSE(r.valid); + } + for (const char* ok : {"1", "0.00000001", "21000000", "1.5"}) { + auto r = dragonx::util::parsePaymentURI("drgx:" + taddr + "?amount=" + ok); + EXPECT_TRUE(r.valid); + } } void testWalletFileProbe()