From 31e9c2ee2c452266ef48c2d98bf145bcb2118f28 Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 19 Oct 2026 10:34:47 -0500 Subject: [PATCH] security(exposure): owner-only ACLs on Windows, auto-clearing export copy, require SHA-256 for bootstrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - writeFileAtomically honored restrictPermissions only on POSIX (0600); on Windows it was a no-op, so exported keys / seed backups / the vault / chat exports inherited the parent dir's ACL. It now builds a PROTECTED owner-only DACL (current-user SID, SE_DACL_PROTECTED) on the temp file before the atomic rename; best-effort, falling back to default ACLs on failure. (Verified it compiles under mingw-w64; the POSIX path is unchanged.) - The key-export dialog's Copy wrote the private key to the plain, permanent clipboard; it now goes through copySecretToClipboard() (45s armed auto-wipe + its own "copied, auto-clears" notice), like the seed backup copy. - Bootstrap archive verification dropped the collision-prone MD5 fallback: it now requires a published SHA-256 and fails closed otherwise. (Authenticity vs a malicious server still needs a detached ed25519 signature like the updaters — a separate release-coordinated change.) Co-Authored-By: Claude Opus 4.8 --- src/app.cpp | 5 ++-- src/util/bootstrap.cpp | 67 ++++++++++++++---------------------------- src/util/platform.cpp | 49 ++++++++++++++++++++++++++++-- 3 files changed, 71 insertions(+), 50 deletions(-) diff --git a/src/app.cpp b/src/app.cpp index ab34a81..cbac4ea 100644 --- a/src/app.cpp +++ b/src/app.cpp @@ -4344,8 +4344,9 @@ void App::renderExportKeyDialog() ImGui::InputTextMultiline("##exportresult", export_result_, sizeof(export_result_), ImVec2(-1, 60.0f * dp), ImGuiInputTextFlags_ReadOnly); if (m::TactileButton(TR("copy"))) { - ImGui::SetClipboardText(export_result_); - ui::Notifications::instance().info(TR("copied_to_clipboard")); + // A private key is a secret — route through the auto-clearing clipboard path (45s armed wipe + // + its own "copied, auto-clears" toast) instead of a plain, permanent clipboard write. + copySecretToClipboard(export_result_); } } else if (export_error_) { ImGui::Spacing(); diff --git a/src/util/bootstrap.cpp b/src/util/bootstrap.cpp index d033d79..bc2b5ca 100644 --- a/src/util/bootstrap.cpp +++ b/src/util/bootstrap.cpp @@ -1125,56 +1125,33 @@ bool Bootstrap::verifyChecksums(const std::string& zipPath, const std::string& b std::string expectedSha = parseChecksumFile(sha256Content, 64); const bool haveSHA256 = !expectedSha.empty(); - std::string expectedMd5; - bool haveMD5 = false; - if (!haveSHA256) { // MD5 only as a fallback when no SHA-256 is published - std::string md5Content = httpGetString(baseUrl + "/" + kZipName + ".md5", "[bootstrap]", opt); - if (cancel_requested_) return false; - expectedMd5 = parseChecksumFile(md5Content, 32); - haveMD5 = !expectedMd5.empty(); - } - - if (!haveSHA256 && !haveMD5) { - DEBUG_LOGF("[Bootstrap] No checksum published - refusing to install an unverified archive\n"); - setProgress(State::Failed, "Verification failed: no checksum was published for the bootstrap. " + // SHA-256 is now required: the collision-prone MD5 fallback was removed. (A same-origin checksum + // only protects against transport corruption, not a malicious server — true authenticity needs a + // detached ed25519 signature like the daemon/xmrig updaters; that is a separate release-coordinated + // change, since it requires publishing per-archive .sig files and pinning a key.) + if (!haveSHA256) { + DEBUG_LOGF("[Bootstrap] No SHA-256 published - refusing to install an unverified archive\n"); + setProgress(State::Failed, "Verification failed: no SHA-256 checksum was published for the bootstrap. " "Refusing to install an unverified archive."); return false; // fail CLOSED; archive is KEPT } - if (haveSHA256) { - setProgress(State::Verifying, "Verifying SHA-256...", 0, 1); - std::string actual = computeSHA256(zipPath, 0.0f, 100.0f); - if (cancel_requested_) return false; - if (actual.empty()) { setProgress(State::Failed, "SHA-256 verification error"); return false; } - if (expectedSha != actual) { - last_verify_mismatch_ = true; - DEBUG_LOGF("[Bootstrap] SHA-256 MISMATCH!\n expected: %s\n actual: %s\n", - expectedSha.c_str(), actual.c_str()); - setProgress(State::Failed, - "SHA-256 mismatch \xe2\x80\x94 the server's checksum file may be out of date.\n" - "Expected: " + expectedSha.substr(0, 16) + "...\n" - "Got: " + actual.substr(0, 16) + "...\n" - "Try again or use a fresh download."); - return false; - } - DEBUG_LOGF("[Bootstrap] SHA-256 verified: %s\n", actual.c_str()); - } else { // haveMD5 - setProgress(State::Verifying, "Verifying MD5...", 0, 1); - std::string actual = computeMD5(zipPath, 0.0f, 100.0f); - if (cancel_requested_) return false; - if (actual.empty()) { setProgress(State::Failed, "MD5 verification error"); return false; } - if (expectedMd5 != actual) { - last_verify_mismatch_ = true; - DEBUG_LOGF("[Bootstrap] MD5 MISMATCH!\n expected: %s\n actual: %s\n", - expectedMd5.c_str(), actual.c_str()); - setProgress(State::Failed, - "MD5 mismatch \xe2\x80\x94 the server's checksum file may be out of date.\n" - "Expected: " + expectedMd5 + "\nGot: " + actual + "\n" - "Try again or use a fresh download."); - return false; - } - DEBUG_LOGF("[Bootstrap] MD5 verified: %s\n", actual.c_str()); + setProgress(State::Verifying, "Verifying SHA-256...", 0, 1); + std::string actual = computeSHA256(zipPath, 0.0f, 100.0f); + if (cancel_requested_) return false; + if (actual.empty()) { setProgress(State::Failed, "SHA-256 verification error"); return false; } + if (expectedSha != actual) { + last_verify_mismatch_ = true; + DEBUG_LOGF("[Bootstrap] SHA-256 MISMATCH!\n expected: %s\n actual: %s\n", + expectedSha.c_str(), actual.c_str()); + setProgress(State::Failed, + "SHA-256 mismatch \xe2\x80\x94 the server's checksum file may be out of date.\n" + "Expected: " + expectedSha.substr(0, 16) + "...\n" + "Got: " + actual.substr(0, 16) + "...\n" + "Try again or use a fresh download."); + return false; } + DEBUG_LOGF("[Bootstrap] SHA-256 verified: %s\n", actual.c_str()); setProgress(State::Verifying, "Checksums verified \xe2\x9c\x93", 100.0, 100.0); return true; diff --git a/src/util/platform.cpp b/src/util/platform.cpp index aa3531b..2c3c5f0 100644 --- a/src/util/platform.cpp +++ b/src/util/platform.cpp @@ -25,6 +25,7 @@ #include #include #include +#include // SetEntriesInAcl / EXPLICIT_ACCESS — owner-only DACL for restricted writes #else #include #include @@ -361,15 +362,54 @@ bool Platform::writeFileAtomically(const std::string& path, const std::string tmp = path + ".tmp"; #ifdef _WIN32 + // When restrictPermissions, build a PROTECTED owner-only DACL so the secret file (exported keys, + // seed backup, vault, chat export) is readable only by the current user — not whatever the parent + // dir's inherited ACL grants. Best-effort: on any failure fall back to default ACLs (the per-user + // profile dir still protects it). The ACL is set on the temp file; MoveFileEx is a same-volume + // rename, so the explicit (non-inherited) DACL carries to the final path. + PACL dacl = nullptr; + PTOKEN_USER tokenUser = nullptr; + SECURITY_DESCRIPTOR sd; + SECURITY_ATTRIBUTES saOwner{}; + SECURITY_ATTRIBUTES* psa = nullptr; + if (restrictPermissions) { + HANDLE tok = nullptr; + if (OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &tok)) { + DWORD need = 0; + GetTokenInformation(tok, TokenUser, nullptr, 0, &need); + if (need > 0) { + tokenUser = static_cast(LocalAlloc(LPTR, need)); + if (tokenUser && GetTokenInformation(tok, TokenUser, tokenUser, need, &need)) { + EXPLICIT_ACCESSA ea{}; + ea.grfAccessPermissions = GENERIC_ALL; + ea.grfAccessMode = SET_ACCESS; + ea.grfInheritance = NO_INHERITANCE; + ea.Trustee.TrusteeForm = TRUSTEE_IS_SID; + ea.Trustee.TrusteeType = TRUSTEE_IS_USER; + ea.Trustee.ptstrName = static_cast(tokenUser->User.Sid); + if (SetEntriesInAclA(1, &ea, nullptr, &dacl) == ERROR_SUCCESS && + InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION) && + SetSecurityDescriptorControl(&sd, SE_DACL_PROTECTED, SE_DACL_PROTECTED) && + SetSecurityDescriptorDacl(&sd, TRUE, dacl, FALSE)) { + saOwner.nLength = sizeof(saOwner); + saOwner.lpSecurityDescriptor = &sd; + saOwner.bInheritHandle = FALSE; + psa = &saOwner; + } + } + } + CloseHandle(tok); + } + } { // One handle for write + flush. CreateFileA with no SECURITY_ATTRIBUTES is NOT inheritable: the old // std::ofstream (CRT) handle was, so a child spawned with bInheritHandles=TRUE at that moment (xmrig, // runHiddenCapture) kept ".tmp" open without FILE_SHARE_DELETE and every later MoveFileEx // failed while it ran. Also no second share-mode-0 open, which silently skipped the flush whenever // AV / the indexer had the new file open. - HANDLE h = CreateFileA(tmp.c_str(), GENERIC_WRITE, 0, nullptr, + HANDLE h = CreateFileA(tmp.c_str(), GENERIC_WRITE, 0, psa, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr); - if (h == INVALID_HANDLE_VALUE) return false; + if (h == INVALID_HANDLE_VALUE) { if (dacl) LocalFree(dacl); if (tokenUser) LocalFree(tokenUser); return false; } bool ok = true; size_t written = 0; while (written < content.size()) { @@ -382,9 +422,12 @@ bool Platform::writeFileAtomically(const std::string& path, // Flush the temp file's contents to stable storage before the rename. if (ok && !FlushFileBuffers(h)) ok = false; CloseHandle(h); + // The DACL is baked into the temp file's security descriptor at CreateFileA time, so release the + // builders now; every path below (including the early returns) is past this point. + if (dacl) LocalFree(dacl); + if (tokenUser) LocalFree(tokenUser); if (!ok) { DeleteFileA(tmp.c_str()); return false; } } - (void)restrictPermissions; // owner-only ACLs rely on the per-user profile dir on Windows // MoveFileEx with REPLACE_EXISTING is atomic on NTFS; WRITE_THROUGH makes it durable. AV / the search // indexer can hold the destination briefly (ACCESS_DENIED / SHARING_VIOLATION / LOCK_VIOLATION): retry // a few times (20+40+80+160 ms worst case, only on a write that used to just fail) instead of dropping it.