security hardening: memory safety, crypto zeroization, FFI checks, DoS/wallet hardening

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'

src/bootstrap.cpp

@@ -268,17 +268,25 @@ static size_t DiscoverBootstrapPeersFromSocket(SOCKET socket, const CService& pe
 
     std::vector<CAddress> 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) {

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()

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

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();

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<CSubNet, int64_t>::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));

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())));

src/rpc/server.h

@@ -10,6 +10,7 @@
 #include "rpc/protocol.h"
 #include "uint256.h"
 
+#include <atomic>
 #include <list>
 #include <map>
 #include <stdint.h>
@@ -169,7 +170,9 @@ extern uint256 ParseHashO(const UniValue& o, std::string strKey);
 extern std::vector<unsigned char> ParseHexV(const UniValue& v, std::string strName);
 extern std::vector<unsigned char> 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<int64_t> nWalletUnlockTime;
 extern CAmount AmountFromValue(const UniValue& value);
 extern UniValue ValueFromAmount(const CAmount& amount);
 extern double GetDifficulty(const CBlockIndex* blockindex = NULL);

src/serialize.h

@@ -366,13 +366,25 @@ template<typename Stream, typename I>
 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<I>::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<I>::max()) {
+                throw std::ios_base::failure("ReadVarInt(): size too large");
+            }
             n++;
-        else
+        } else {
             return n;
+        }
     }
 }
 

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;

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

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<CKeyID, int64_t> mapKeyBirth;
     std::set<CKeyID> setKeyPool;
     pwalletMain->GetKeyBirthTimes(mapKeyBirth);

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<int64_t> 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}]")