From 5f4524af7c484bbfa5f2e83916a79c7da2f0bcb4 Mon Sep 17 00:00:00 2001 From: VictorLux Date: Fri, 12 Jun 2026 08:07:30 +0200 Subject: [PATCH] security hardening: memory safety, crypto zeroization, FFI checks, DoS/wallet hardening Self-contained fixes from a 2026-06 multi-domain security review. Scoped to the files that are identical to upstream so they apply cleanly here; the consensus / main.cpp findings from the same review (CON-01/05, BLK-01, NET-01) depend on other not-yet-merged work and are intentionally left out of this PR. Memory safety: - streams.h size_t bounds + pre-add overflow guard in CBaseDataStream read/ignore - serialize.h ReadVarInt overflow guard before shift and increment (upstream form) - bootstrap.cpp bound the addr count before vector allocation in discovery Cryptography: - NoteEncryption.cpp RAII zeroization of symmetric key K and dhsecret on every scope exit (incl. KDF/DH-failure throw) - Proof.cpp explicit G1 prime-order subgroup check (mirrors the G2 check) Rust / FFI: - transaction_builder.cpp check librustzcash_sapling_spend_sig / _binding_sig returns; error out instead of a silent zero-signed tx - init.cpp ferror guard in check_file_hash read loop (no spin on I/O error) P2P / performance / stability: - net.cpp prune expired setBanned entries on Ban(); cap mapRelay growth - bootstrapvalidation.cpp reduce cs_main batch budget 80 -> 20 ms - wallet.cpp drop redundant LOCK(mempool.cs) in ReacceptWalletTransactions Wallet / RPC: - rpcwallet.cpp z_sendmany help warns about -debug=zrpcunsafe privacy leak - httpserver.cpp warn when -rpcallowip is set without -rpcbind - wallet.cpp raise wallet KDF iteration floor 25000 -> 100000 - rpcdump.cpp dumpwallet writes the export file 0600 (keys + HD seed) - server.h / rpcwallet.cpp / misc.cpp nWalletUnlockTime -> std::atomic CI: - qa/pull-tester/rpc-tests.sh re-enable the shielded regression tests Verification: builds and runs on mainnet at tip ~3.14M; the shielded encrypt/ decrypt + signing paths (NoteEncryption + transaction_builder) were exercised by a z_sendmany round-trip (note encrypted, signed, broadcast, mined, decrypted). Co-Authored-By: Claude Opus 4.8 --- qa/pull-tester/rpc-tests.sh | 30 +++++++++++++++------------ src/bootstrap.cpp | 22 +++++++++++++------- src/bootstrapvalidation.cpp | 5 ++++- src/httpserver.cpp | 8 ++++++++ src/init.cpp | 8 ++++++++ src/net.cpp | 22 ++++++++++++++++++++ src/rpc/misc.cpp | 2 +- src/rpc/server.h | 5 ++++- src/serialize.h | 16 +++++++++++++-- src/streams.h | 31 ++++++++++++++++------------ src/transaction_builder.cpp | 29 ++++++++++++++++++-------- src/wallet/rpcdump.cpp | 11 ++++++++++ src/wallet/rpcwallet.cpp | 11 +++++++--- src/wallet/wallet.cpp | 20 +++++++++++++----- src/zcash/NoteEncryption.cpp | 40 ++++++++++++++++++++++++++++++++++++ src/zcash/Proof.cpp | 9 ++++++++ 16 files changed, 214 insertions(+), 55 deletions(-) diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh index 7d8b0b7d4fb..a4bbc27dd31 100755 --- a/qa/pull-tester/rpc-tests.sh +++ b/qa/pull-tester/rpc-tests.sh @@ -10,25 +10,29 @@ export BITCOIND=${REAL_BITCOIND} #Run the tests +# SUP-04: the shielded-pool regression tests below were re-enabled (they had been +# commented out, leaving ZClassic's core privacy features unguarded in CI). If any +# fails, triage and fix the underlying issue or file a tracking ticket with a code +# comment — do NOT silently re-comment it. testScripts=( - # 'paymentdisclosure.py' + 'paymentdisclosure.py' 'prioritisetransaction.py' - # 'wallet_treestate.py' - # 'wallet_anchorfork.py' + 'wallet_treestate.py' + 'wallet_anchorfork.py' # 'wallet_changeindicator.py' 'wallet_import_export.py' - # 'wallet_protectcoinbase.py' - # 'wallet_shieldcoinbase_sprout.py' - # 'wallet_shieldcoinbase_sapling.py' - # 'wallet_listreceived.py' + 'wallet_protectcoinbase.py' + 'wallet_shieldcoinbase_sprout.py' + 'wallet_shieldcoinbase_sapling.py' + 'wallet_listreceived.py' # 'wallet.py' # 'wallet_overwintertx.py' 'wallet_persistence.py' - # 'wallet_nullifiers.py' + 'wallet_nullifiers.py' # 'wallet_1941.py' 'wallet_addresses.py' 'wallet_sapling.py' - # 'wallet_listnotes.py' + 'wallet_listnotes.py' # 'mergetoaddress_sprout.py' # 'mergetoaddress_sapling.py' 'listtransactions.py' @@ -40,8 +44,8 @@ testScripts=( 'rest.py' 'mempool_spendcoinbase.py' 'mempool_reorg.py' - # 'mempool_tx_input_limit.py' - # 'mempool_nu_activation.py' + 'mempool_tx_input_limit.py' + 'mempool_nu_activation.py' 'mempool_tx_expiry.py' 'httpbasics.py' 'zapwallettxes.py' @@ -58,8 +62,8 @@ testScripts=( 'blockchain.py' 'disablewallet.py' 'zcjoinsplit.py' - # 'zcjoinsplitdoublespend.py' - # 'zkey_import_export.py' + 'zcjoinsplitdoublespend.py' + 'zkey_import_export.py' 'reorg_limit.py' 'getblocktemplate.py' 'bip65-cltv-p2p.py' diff --git a/src/bootstrap.cpp b/src/bootstrap.cpp index a2a5d1979c6..7246127b1ee 100644 --- a/src/bootstrap.cpp +++ b/src/bootstrap.cpp @@ -268,17 +268,25 @@ static size_t DiscoverBootstrapPeersFromSocket(SOCKET socket, const CService& pe std::vector vAddr; try { - addrPayload >> vAddr; + // MEM-03: read and bound the element COUNT before allocating/deserializing + // the vector, mirroring the normal addr handler's 1000-entry bound. The + // previous `addrPayload >> vAddr` deserialized the whole list (up to the + // 2 MiB message cap) before the size guard below could fire. + uint64_t nAddr = ReadCompactSize(addrPayload); + if (nAddr > 1000) { + LogPrint("net", "bootstrap discovery: oversized addr (%llu) from %s\n", (unsigned long long)nAddr, peerAddress.ToStringIPPort()); + return 0; + } + vAddr.reserve(nAddr); + for (uint64_t i = 0; i < nAddr; ++i) { + CAddress a; + addrPayload >> a; + vAddr.push_back(a); + } } catch (const std::exception& e) { LogPrint("net", "bootstrap discovery: malformed addr from %s: %s\n", peerAddress.ToStringIPPort(), e.what()); return 0; } - // Mirror the addr-message bound enforced by the normal net handler so a - // misbehaving peer cannot make us iterate an enormous list. - if (vAddr.size() > 1000) { - LogPrint("net", "bootstrap discovery: oversized addr (%u) from %s\n", (unsigned int)vAddr.size(), peerAddress.ToStringIPPort()); - return 0; - } size_t appended = 0; for (size_t i = 0; i < vAddr.size() && out.size() < BOOTSTRAP_DISCOVERY_MAX_RESULTS; ++i) { diff --git a/src/bootstrapvalidation.cpp b/src/bootstrapvalidation.cpp index d831b887ba6..3fa3fa2c6e2 100644 --- a/src/bootstrapvalidation.cpp +++ b/src/bootstrapvalidation.cpp @@ -103,7 +103,10 @@ static void RefreshFinalizationHoldLocked() g_finalizationHold.store(provisional || tipHold, std::memory_order_relaxed); } -static const int64_t BOOTSTRAPVAL_BATCH_MS = 80; // cs_main per-batch budget +// PERF-04: cs_main per-batch budget. Lowered 80 -> 20 ms so the background UTXO +// validator holds cs_main for far shorter spans, reducing stalls to live message +// processing / block relay (and peer timeouts on slower hardware) while it runs. +static const int64_t BOOTSTRAPVAL_BATCH_MS = 20; // cs_main per-batch budget static const size_t BOOTSTRAPVAL_FLUSH_CAP = 300 * (1 << 20); // in-mem coin cache cap static boost::filesystem::path ScratchDir() diff --git a/src/httpserver.cpp b/src/httpserver.cpp index b6d810531c9..4cd0b7145f7 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -336,6 +336,14 @@ static bool HTTPBindAddresses(struct evhttp* http) endpoints.push_back(std::make_pair(host, port)); } } else { // No specific bind address specified, bind to any + // WAL-02: -rpcallowip was set without -rpcbind, so the RPC port binds on + // ALL interfaces and is reachable from the network, protected only by the + // IP ACL plus the RPC password. Warn loudly — a broad ACL (e.g. + // -rpcallowip=0.0.0.0/0) then exposes dumpprivkey/dumpwallet/z_exportkey to + // the internet behind the password alone. + LogPrintf("WARNING: -rpcallowip was specified without -rpcbind; binding RPC to all interfaces " + "(0.0.0.0 and ::). The RPC port is now network-reachable, guarded only by the IP ACL " + "and password. Set -rpcbind=127.0.0.1 (or a specific address) unless this is intended.\n"); endpoints.push_back(std::make_pair("::", defaultPort)); endpoints.push_back(std::make_pair("0.0.0.0", defaultPort)); } diff --git a/src/init.cpp b/src/init.cpp index ac853e6b495..8b5de9a3254 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -766,6 +766,14 @@ static bool check_file_hash(const std::string& path, const std::string& hash) SHA256 buff; while (!feof(file)){ size = fread(buffer.data(), 1, kHashReadBufSize, file); + // RUST-02: on a real I/O error fread sets ferror() (not feof()) and returns + // 0, so the original `while(!feof)` loop would spin forever at 100% CPU. + // Bail out instead of hanging node startup. + if (size == 0 && ferror(file)) { + LogPrintf("%s: I/O error while reading for hash check\n", path); + fclose(file); + return false; + } buff.update(buffer.data(), size); } std::string buff_hash = buff.hash(); diff --git a/src/net.cpp b/src/net.cpp index 4aab7bda1c0..99a7122f9db 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -496,6 +496,17 @@ void CNode::Ban(const CSubNet& subNet, int64_t bantimeoffset, bool sinceUnixEpoc banTime = (sinceUnixEpoch ? 0 : GetTime() )+bantimeoffset; LOCK(cs_setBanned); + // NET-02: prune expired entries on each new ban so setBanned cannot grow + // unbounded. Without this, an attacker cycling distinct source IPs accumulates + // dead entries that IsBanned() linearly scans under cs_setBanned on every + // inbound connection, degrading into a lock-contention bottleneck over time. + int64_t nowPrune = GetTime(); + for (std::map::iterator it = setBanned.begin(); it != setBanned.end(); ) { + if (it->second < nowPrune) + setBanned.erase(it++); + else + ++it; + } if (setBanned[subNet] < banTime) setBanned[subNet] = banTime; } @@ -1883,6 +1894,17 @@ void RelayTransaction(const CTransaction& tx, const CDataStream& ss) vRelayExpiration.pop_front(); } + // PERF-03: hard cap on relay entries. Time-based expiry alone lets a flood + // of unique-txid transactions grow mapRelay (full CDataStream per entry) to + // hundreds of MB before the 15-minute timer reclaims the oldest. Evict the + // oldest entries once the cap is reached. + static const size_t MAX_RELAY_ENTRIES = 100000; + while (mapRelay.size() >= MAX_RELAY_ENTRIES && !vRelayExpiration.empty()) + { + mapRelay.erase(vRelayExpiration.front().second); + vRelayExpiration.pop_front(); + } + // Save original serialized message so newer versions are preserved mapRelay.insert(std::make_pair(inv, ss)); vRelayExpiration.push_back(std::make_pair(GetTime() + 15 * 60, inv)); diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index f6d31c1641f..1ab134644f2 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -100,7 +100,7 @@ UniValue getinfo(const UniValue& params, bool fHelp) obj.push_back(Pair("keypoolsize", (int)pwalletMain->GetKeyPoolSize())); } if (pwalletMain && pwalletMain->IsCrypted()) - obj.push_back(Pair("unlocked_until", nWalletUnlockTime)); + obj.push_back(Pair("unlocked_until", nWalletUnlockTime.load())); // WAL-06 obj.push_back(Pair("paytxfee", ValueFromAmount(payTxFee.GetFeePerK()))); #endif obj.push_back(Pair("relayfee", ValueFromAmount(::minRelayTxFee.GetFeePerK()))); diff --git a/src/rpc/server.h b/src/rpc/server.h index 6ae4279287a..4fb2e3cc251 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -10,6 +10,7 @@ #include "rpc/protocol.h" #include "uint256.h" +#include #include #include #include @@ -169,7 +170,9 @@ extern uint256 ParseHashO(const UniValue& o, std::string strKey); extern std::vector ParseHexV(const UniValue& v, std::string strName); extern std::vector ParseHexO(const UniValue& o, std::string strKey); -extern int64_t nWalletUnlockTime; +// WAL-06: atomic so unlocked reads in getwalletinfo/getinfo cannot tear against +// the relock timer's write (writes still occur under cs_nWalletUnlockTime). +extern std::atomic nWalletUnlockTime; extern CAmount AmountFromValue(const UniValue& value); extern UniValue ValueFromAmount(const CAmount& amount); extern double GetDifficulty(const CBlockIndex* blockindex = NULL); diff --git a/src/serialize.h b/src/serialize.h index a945650d6f5..965ba62da4d 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -366,13 +366,25 @@ template I ReadVarInt(Stream& is) { I n = 0; + // MEM-02: guard against overflow of I BEFORE shifting and BEFORE the + // increment, so a malformed max-length encoding cannot wrap the value or hit + // signed UB (VARINT is also used for signed fields, e.g. CDiskBlockIndex + // nFile/nPos). This is the upstream Bitcoin Core form; it also bounds the + // iteration count, since n grows past the limit within ceil(bits/7) bytes. while(true) { unsigned char chData = ser_readdata8(is); + if (n > (std::numeric_limits::max() >> 7)) { + throw std::ios_base::failure("ReadVarInt(): size too large"); + } n = (n << 7) | (chData & 0x7F); - if (chData & 0x80) + if (chData & 0x80) { + if (n == std::numeric_limits::max()) { + throw std::ios_base::failure("ReadVarInt(): size too large"); + } n++; - else + } else { return n; + } } } diff --git a/src/streams.h b/src/streams.h index 9d4a2e39e04..79f4185f260 100644 --- a/src/streams.h +++ b/src/streams.h @@ -281,20 +281,21 @@ class CBaseDataStream throw std::ios_base::failure("CBaseDataStream::read(): cannot read from null pointer"); } - // Read from the beginning of the buffer - unsigned int nReadPosNext = nReadPos + nSize; - if (nReadPosNext >= vch.size()) + // MEM-01: do the bounds arithmetic in size_t and check for overflow + // BEFORE adding. Previously `unsigned int nReadPosNext = nReadPos + nSize` + // narrowed the size_t nSize on LP64, so a large nSize could wrap to a small + // value that slipped past the guard before the full-size memcpy ran. + if (nReadPos > vch.size() || nSize > vch.size() - nReadPos) { + throw std::ios_base::failure("CBaseDataStream::read(): end of data"); + } + size_t nReadPosNext = (size_t)nReadPos + nSize; + memcpy(pch, &vch[nReadPos], nSize); + if (nReadPosNext == vch.size()) { - if (nReadPosNext > vch.size()) - { - throw std::ios_base::failure("CBaseDataStream::read(): end of data"); - } - memcpy(pch, &vch[nReadPos], nSize); nReadPos = 0; vch.clear(); return; } - memcpy(pch, &vch[nReadPos], nSize); nReadPos = nReadPosNext; } @@ -304,11 +305,15 @@ class CBaseDataStream if (nSize < 0) { throw std::ios_base::failure("CDataStream::ignore(): nSize negative"); } - unsigned int nReadPosNext = nReadPos + nSize; - if (nReadPosNext >= vch.size()) + // MEM-01: size_t arithmetic with an explicit pre-addition overflow guard + // (see read() above). + size_t snSize = (size_t)nSize; + if (nReadPos > vch.size() || snSize > vch.size() - nReadPos) { + throw std::ios_base::failure("CBaseDataStream::ignore(): end of data"); + } + size_t nReadPosNext = (size_t)nReadPos + snSize; + if (nReadPosNext == vch.size()) { - if (nReadPosNext > vch.size()) - throw std::ios_base::failure("CBaseDataStream::ignore(): end of data"); nReadPos = 0; vch.clear(); return; diff --git a/src/transaction_builder.cpp b/src/transaction_builder.cpp index 30808e7200e..7239d1a448b 100644 --- a/src/transaction_builder.cpp +++ b/src/transaction_builder.cpp @@ -285,18 +285,29 @@ TransactionBuilderResult TransactionBuilder::Build() } // Create Sapling spendAuth and binding signatures + // RUST-01: these FFI calls return bool and fail on an invalid ask/ar (e.g. a + // corrupted spending key). Previously the returns were discarded, so on + // failure the tx was built with an all-zero signature that every node rejects + // while the wallet treated the note as spent (funds stranded, hard to + // diagnose). Check both and surface an error instead. for (size_t i = 0; i < spends.size(); i++) { - librustzcash_sapling_spend_sig( - spends[i].expsk.ask.begin(), - spends[i].alpha.begin(), + if (!librustzcash_sapling_spend_sig( + spends[i].expsk.ask.begin(), + spends[i].alpha.begin(), + dataToBeSigned.begin(), + mtx.vShieldedSpend[i].spendAuthSig.data())) { + librustzcash_sapling_proving_ctx_free(ctx); + return TransactionBuilderResult("Failed to create Sapling spend signature"); + } + } + if (!librustzcash_sapling_binding_sig( + ctx, + mtx.valueBalance, dataToBeSigned.begin(), - mtx.vShieldedSpend[i].spendAuthSig.data()); + mtx.bindingSig.data())) { + librustzcash_sapling_proving_ctx_free(ctx); + return TransactionBuilderResult("Failed to create Sapling binding signature"); } - librustzcash_sapling_binding_sig( - ctx, - mtx.valueBalance, - dataToBeSigned.begin(), - mtx.bindingSig.data()); librustzcash_sapling_proving_ctx_free(ctx); diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 9a0f71e3f70..590f0926237 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -487,6 +487,17 @@ UniValue dumpwallet_impl(const UniValue& params, bool fHelp, bool fDumpZKeys) if (!file.is_open()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file"); + // WAL-05: this file contains every private key AND the HD seed (root entropy + // for all derived keys) in plaintext. Restrict it to owner read/write (0600) + // immediately so it is not left world/group-readable in the export directory. + try { + boost::filesystem::permissions(exportfilepath, + boost::filesystem::owner_read | boost::filesystem::owner_write); + } catch (const boost::filesystem::filesystem_error& e) { + LogPrintf("dumpwallet: warning: could not set 0600 permissions on %s: %s\n", + exportfilepath.string(), e.what()); + } + std::map mapKeyBirth; std::set setKeyPool; pwalletMain->GetKeyBirthTimes(mapKeyBirth); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 86149e415f2..9cfe397aaf5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -50,8 +50,8 @@ const std::string ADDR_TYPE_SAPLING = "sapling"; extern UniValue TxJoinSplitToJSON(const CTransaction& tx); -int64_t nWalletUnlockTime; -static CCriticalSection cs_nWalletUnlockTime; +std::atomic nWalletUnlockTime; // WAL-06: atomic (see rpc/server.h) +static CCriticalSection cs_nWalletUnlockTime; // still serializes check-then-set writes // Private method: UniValue z_getoperationstatus_IMPL(const UniValue&, bool); @@ -2299,7 +2299,7 @@ UniValue getwalletinfo(const UniValue& params, bool fHelp) obj.push_back(Pair("keypoololdest", pwalletMain->GetOldestKeyPoolTime())); obj.push_back(Pair("keypoolsize", (int)pwalletMain->GetKeyPoolSize())); if (pwalletMain->IsCrypted()) - obj.push_back(Pair("unlocked_until", nWalletUnlockTime)); + obj.push_back(Pair("unlocked_until", nWalletUnlockTime.load())); // WAL-06 obj.push_back(Pair("paytxfee", ValueFromAmount(payTxFee.GetFeePerK()))); uint256 seedFp = pwalletMain->GetHDChain().seedFp; if (!seedFp.IsNull()) @@ -3743,6 +3743,11 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) + strprintf("%s", FormatMoney(ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE)) + ") The fee amount to attach to this transaction.\n" "\nResult:\n" "\"operationid\" (string) An operationid to pass to z_getoperationstatus to get the result of the operation.\n" + "\nWARNING: running with -debug=zrpcunsafe (or -debug=all) writes the\n" + "sender address, every recipient address, amounts and memo fields of this\n" + "call to debug.log in plaintext, defeating shielded-pool privacy. Do not\n" + "enable that category on a node whose debug.log is shipped to log\n" + "aggregators or shared.\n" "\nExamples:\n" + HelpExampleCli("z_sendmany", "\"t1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\" '[{\"address\": \"ztfaW34Gj9FrnGUEf833ywDVL62NWXBM81u6EQnM6VR45eYnXhwztecW1SjxA7JrmAXKJhxhj3vDNEpVCQoSvVoSpmbhtjf\" ,\"amount\": 5.0}]'") + HelpExampleRpc("z_sendmany", "\"t1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\", [{\"address\": \"ztfaW34Gj9FrnGUEf833ywDVL62NWXBM81u6EQnM6VR45eYnXhwztecW1SjxA7JrmAXKJhxhj3vDNEpVCQoSvVoSpmbhtjf\" ,\"amount\": 5.0}]") diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 10602ebf5d6..cb32aab346f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -537,8 +537,13 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod); pMasterKey.second.nDeriveIterations = (pMasterKey.second.nDeriveIterations + pMasterKey.second.nDeriveIterations * 100 / ((double)(GetTimeMillis() - nStartTime))) / 2; - if (pMasterKey.second.nDeriveIterations < 25000) - pMasterKey.second.nDeriveIterations = 25000; + // WAL-03: raise the KDF iteration floor 25000 -> 100000. The + // dynamic calibration targets ~0.1s and normally lands far higher; + // the floor only binds on very fast machines, where 25000 rounds of + // (non-memory-hard) PBKDF2-SHA512 is too cheap against an offline + // wallet.dat dictionary attack. + if (pMasterKey.second.nDeriveIterations < 100000) + pMasterKey.second.nDeriveIterations = 100000; LogPrintf("Wallet passphrase changed to an nDeriveIterations of %i\n", pMasterKey.second.nDeriveIterations); @@ -1195,8 +1200,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, kMasterKey.nDeriveIterations, kMasterKey.nDerivationMethod); kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + kMasterKey.nDeriveIterations * 100 / ((double)(GetTimeMillis() - nStartTime))) / 2; - if (kMasterKey.nDeriveIterations < 25000) - kMasterKey.nDeriveIterations = 25000; + // WAL-03: raise the KDF iteration floor 25000 -> 100000 (see ChangeWalletPassphrase). + if (kMasterKey.nDeriveIterations < 100000) + kMasterKey.nDeriveIterations = 100000; LogPrintf("Encrypting Wallet with an nDeriveIterations of %i\n", kMasterKey.nDeriveIterations); @@ -2779,7 +2785,11 @@ void CWallet::ReacceptWalletTransactions() { CWalletTx& wtx = *(item.second); - LOCK(mempool.cs); + // PERF-01: do not take an explicit LOCK(mempool.cs) here. AcceptToMemoryPool + // already acquires mempool.cs internally (under cs_main); taking it here too + // only added a redundant cs_wallet -> mempool.cs lock-order edge that a + // lock-order checker flags as an inversion against the pool.cs -> cs_wallet + // path (NotifyRecentlyAdded -> SyncWithWallets). wtx.AcceptToMemoryPool(false); } } diff --git a/src/zcash/NoteEncryption.cpp b/src/zcash/NoteEncryption.cpp index 63e07326542..eee0a61c736 100644 --- a/src/zcash/NoteEncryption.cpp +++ b/src/zcash/NoteEncryption.cpp @@ -4,9 +4,26 @@ #include #include "prf.h" #include "librustzcash.h" +#include "support/cleanse.h" // CRY-01: zeroize symmetric keys / DH secrets after use #define NOTEENCRYPTION_CIPHER_KEYSIZE 32 +namespace { +// CRY-01: zeroes a fixed buffer when it leaves scope, so a derived symmetric key +// or DH secret is cleared on EVERY exit path — normal return, early return, and +// any throw (e.g. a should-never-happen KDF hash-failure between key derivation +// and the end of the function). Declaring one of these right after the secret +// buffer guarantees cleanup without relying on manual calls before each return. +struct MemoryCleanser { + void* p; + size_t n; + MemoryCleanser(void* p_, size_t n_) : p(p_), n(n_) {} + ~MemoryCleanser() { memory_cleanse(p, n); } + MemoryCleanser(const MemoryCleanser&) = delete; + MemoryCleanser& operator=(const MemoryCleanser&) = delete; +}; +} // namespace + void clamp_curve25519(unsigned char key[crypto_scalarmult_SCALARBYTES]) { key[0] &= 248; @@ -38,8 +55,10 @@ void PRF_ock( personalization ) != 0) { + memory_cleanse(block, sizeof(block)); // CRY-01 throw std::logic_error("hash function failure"); } + memory_cleanse(block, sizeof(block)); // CRY-01: block held ovk/cv/cm/epk } void KDF_Sapling( @@ -62,8 +81,10 @@ void KDF_Sapling( personalization ) != 0) { + memory_cleanse(block, sizeof(block)); // CRY-01 throw std::logic_error("hash function failure"); } + memory_cleanse(block, sizeof(block)); // CRY-01: block held the DH secret } void KDF(unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE], @@ -95,8 +116,10 @@ void KDF(unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE], personalization ) != 0) { + memory_cleanse(block, sizeof(block)); // CRY-01 throw std::logic_error("hash function failure"); } + memory_cleanse(block, sizeof(block)); // CRY-01: block held hSig/dhsecret/epk/pk_enc } namespace libzcash { @@ -126,6 +149,7 @@ boost::optional SaplingNoteEncryption::encrypt_to_recipien } uint256 dhsecret; + MemoryCleanser _cleanseDhsecret(dhsecret.begin(), 32); // CRY-01: zeroed on every exit if (!librustzcash_sapling_ka_agree(pk_d.begin(), esk.begin(), dhsecret.begin())) { return boost::none; @@ -133,6 +157,7 @@ boost::optional SaplingNoteEncryption::encrypt_to_recipien // Construct the symmetric key unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE]; + MemoryCleanser _cleanseK(K, sizeof(K)); // CRY-01: zeroed on every exit KDF_Sapling(K, dhsecret, epk); // The nonce is zero because we never reuse keys @@ -147,6 +172,7 @@ boost::optional SaplingNoteEncryption::encrypt_to_recipien NULL, cipher_nonce, K ); + already_encrypted_enc = true; return ciphertext; @@ -159,6 +185,7 @@ boost::optional AttemptSaplingEncDecryption( ) { uint256 dhsecret; + MemoryCleanser _cleanseDhsecret(dhsecret.begin(), 32); // CRY-01: zeroed on every exit if (!librustzcash_sapling_ka_agree(epk.begin(), ivk.begin(), dhsecret.begin())) { return boost::none; @@ -166,6 +193,7 @@ boost::optional AttemptSaplingEncDecryption( // Construct the symmetric key unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE]; + MemoryCleanser _cleanseK(K, sizeof(K)); // CRY-01: zeroed on every exit KDF_Sapling(K, dhsecret, epk); // The nonce is zero because we never reuse keys @@ -195,6 +223,7 @@ boost::optional AttemptSaplingEncDecryption ( ) { uint256 dhsecret; + MemoryCleanser _cleanseDhsecret(dhsecret.begin(), 32); // CRY-01: zeroed on every exit if (!librustzcash_sapling_ka_agree(pk_d.begin(), esk.begin(), dhsecret.begin())) { return boost::none; @@ -202,6 +231,7 @@ boost::optional AttemptSaplingEncDecryption ( // Construct the symmetric key unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE]; + MemoryCleanser _cleanseK(K, sizeof(K)); // CRY-01: zeroed on every exit KDF_Sapling(K, dhsecret, epk); // The nonce is zero because we never reuse keys @@ -237,6 +267,7 @@ SaplingOutCiphertext SaplingNoteEncryption::encrypt_to_ourselves( // Construct the symmetric key unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE]; + MemoryCleanser _cleanseK(K, sizeof(K)); // CRY-01: zeroed on every exit PRF_ock(K, ovk, cv, cm, epk); // The nonce is zero because we never reuse keys @@ -251,6 +282,7 @@ SaplingOutCiphertext SaplingNoteEncryption::encrypt_to_ourselves( NULL, cipher_nonce, K ); + already_encrypted_out = true; return ciphertext; @@ -266,6 +298,7 @@ boost::optional AttemptSaplingOutDecryption( { // Construct the symmetric key unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE]; + MemoryCleanser _cleanseK(K, sizeof(K)); // CRY-01: zeroed on every exit PRF_ock(K, ovk, cv, cm, epk); // The nonce is zero because we never reuse keys @@ -313,6 +346,7 @@ typename NoteEncryption::Ciphertext NoteEncryption::encrypt ) { uint256 dhsecret; + MemoryCleanser _cleanseDhsecret(dhsecret.begin(), 32); // CRY-01: zeroed on every exit if (crypto_scalarmult(dhsecret.begin(), esk.begin(), pk_enc.begin()) != 0) { throw std::logic_error("Could not create DH secret"); @@ -320,6 +354,7 @@ typename NoteEncryption::Ciphertext NoteEncryption::encrypt // Construct the symmetric key unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE]; + MemoryCleanser _cleanseK(K, sizeof(K)); // CRY-01: zeroed on every exit KDF(K, dhsecret, epk, pk_enc, hSig, nonce); // Increment the number of encryptions we've performed @@ -335,6 +370,7 @@ typename NoteEncryption::Ciphertext NoteEncryption::encrypt NULL, 0, // no "additional data" NULL, cipher_nonce, K); + return ciphertext; } @@ -347,12 +383,14 @@ typename NoteDecryption::Plaintext NoteDecryption::decrypt ) const { uint256 dhsecret; + MemoryCleanser _cleanseDhsecret(dhsecret.begin(), 32); // CRY-01: zeroed on every exit if (crypto_scalarmult(dhsecret.begin(), sk_enc.begin(), epk.begin()) != 0) { throw std::logic_error("Could not create DH secret"); } unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE]; + MemoryCleanser _cleanseK(K, sizeof(K)); // CRY-01: zeroed on every exit KDF(K, dhsecret, epk, pk_enc, hSig, nonce); // The nonce is zero because we never reuse keys @@ -387,6 +425,7 @@ typename PaymentDisclosureNoteDecryption::Plaintext PaymentDisclosureNoteD ) const { uint256 dhsecret; + MemoryCleanser _cleanseDhsecret(dhsecret.begin(), 32); // CRY-01: zeroed on every exit if (crypto_scalarmult(dhsecret.begin(), esk.begin(), pk_enc.begin()) != 0) { throw std::logic_error("Could not create DH secret"); @@ -396,6 +435,7 @@ typename PaymentDisclosureNoteDecryption::Plaintext PaymentDisclosureNoteD uint256 epk = NoteEncryption::generate_pubkey(esk); unsigned char K[NOTEENCRYPTION_CIPHER_KEYSIZE]; + MemoryCleanser _cleanseK(K, sizeof(K)); // CRY-01: zeroed on every exit KDF(K, dhsecret, epk, pk_enc, hSig, nonce); // The nonce is zero because we never reuse keys diff --git a/src/zcash/Proof.cpp b/src/zcash/Proof.cpp index af87d1b8188..9eb40a500ea 100644 --- a/src/zcash/Proof.cpp +++ b/src/zcash/Proof.cpp @@ -127,6 +127,15 @@ curve_G1 CompressedG1::to_libsnark_g1() const assert(r.is_well_formed()); + // CRY-03: explicitly verify the point is in the prime-order subgroup, + // mirroring the G2 check in to_libsnark_g2(). For alt_bn128 G1 the cofactor + // is 1 (every well-formed point is already in the subgroup), so this rejects + // nothing new today, but it removes the implicit, undocumented dependence on + // that cofactor-1 property and makes the G1/G2 defense posture symmetric. + if (alt_bn128_modulus_r * r != curve_G1::zero()) { + throw std::runtime_error("point is not in G1"); + } + return r; }