From 150ab1d34c98096d28d4f7951a02516232f40ae2 Mon Sep 17 00:00:00 2001 From: Daira Emma Hopwood Date: Tue, 31 Oct 2023 18:16:44 +0000 Subject: [PATCH 1/5] Remove code that is definitely dead, because pfrom->nVersion must be >= MIN_PEER_PROTO_VERSION (170100) at this point. Signed-off-by: Daira Emma Hopwood --- src/main.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 8e93c5d7759..c7a1ba427d0 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6849,8 +6849,6 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string return false; } - if (pfrom->nVersion == 10300) - pfrom->nVersion = 300; if (!vRecv.empty()) vRecv >> addrFrom >> nNonce; if (!vRecv.empty()) { From 493acc5deeebecd8599875dfd9a77df021ef3e84 Mon Sep 17 00:00:00 2001 From: Daira Emma Hopwood Date: Tue, 31 Oct 2023 18:59:14 +0000 Subject: [PATCH 2/5] Fix some issues with handling of version messages: * fields were deserialized into variables of platform-dependent length; * pfrom->nVersion was set even if it does not meet the minimum version requirements; * make sure that pfrom->nTimeOffset is set before pfrom->fSuccessfullyConnected. At this point it is still possible for parsing to fail due to a `std::ios_base::failure`, which will not cause a disconnect. Co-authored-by: Andrea Lanfranchi Signed-off-by: Daira Emma Hopwood --- src/main.cpp | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index c7a1ba427d0..7b28b627ccd 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6802,19 +6802,20 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string return false; } - int64_t nTime; + int32_t nVersion = 0; + int64_t nTime = 0; CAddress addrMe; CAddress addrFrom; uint64_t nNonce = 1; std::string strSubVer; std::string cleanSubVer; uint64_t nServices; - vRecv >> pfrom->nVersion >> nServices >> nTime >> addrMe; - pfrom->nServices = nServices; - if (pfrom->nVersion < MIN_PEER_PROTO_VERSION) + vRecv >> nVersion >> nServices >> nTime >> addrMe; + + if (nVersion < MIN_PEER_PROTO_VERSION) { // disconnect from peers older than this proto version - LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, pfrom->nVersion); + LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, nVersion); pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE, strprintf("Version must be %d or greater", MIN_PEER_PROTO_VERSION)); pfrom->fDisconnect = true; @@ -6822,10 +6823,10 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string } if (chainparams.NetworkIDString() == "test" && - pfrom->nVersion < MIN_TESTNET_PEER_PROTO_VERSION) + nVersion < MIN_TESTNET_PEER_PROTO_VERSION) { // disconnect from testnet peers older than this proto version - LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, pfrom->nVersion); + LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, nVersion); pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE, strprintf("Version must be %d or greater", MIN_TESTNET_PEER_PROTO_VERSION)); pfrom->fDisconnect = true; @@ -6835,13 +6836,13 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string // Reject incoming connections from nodes that don't know about the current epoch const Consensus::Params& consensusParams = chainparams.GetConsensus(); auto currentEpoch = CurrentEpoch(GetHeight(), consensusParams); - if (pfrom->nVersion < consensusParams.vUpgrades[currentEpoch].nProtocolVersion && + if (nVersion < consensusParams.vUpgrades[currentEpoch].nProtocolVersion && !( chainparams.NetworkIDString() == "regtest" && !GetBoolArg("-nurejectoldversions", DEFAULT_NU_REJECT_OLD_VERSIONS) ) ) { - LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, pfrom->nVersion); + LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, nVersion); pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE, strprintf("Version must be %d or greater", consensusParams.vUpgrades[currentEpoch].nProtocolVersion)); @@ -6849,23 +6850,30 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string return false; } - if (!vRecv.empty()) + // We've successfully parsed the mandatory fields and checked the version. + // It's safe to leave these set even if subsequent parsing fails. + pfrom->nVersion = nVersion; + pfrom->nServices = nServices; + + if (!vRecv.empty()) { vRecv >> addrFrom >> nNonce; + } if (!vRecv.empty()) { vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH); cleanSubVer = SanitizeString(strSubVer, SAFE_CHARS_SUBVERSION); } if (!vRecv.empty()) { - int nStartingHeight; + int32_t nStartingHeight; vRecv >> nStartingHeight; pfrom->nStartingHeight = nStartingHeight; } { LOCK(pfrom->cs_filter); - if (!vRecv.empty()) + if (!vRecv.empty()) { vRecv >> pfrom->fRelayTxes; // set to true after we get the first filter* message - else + } else { pfrom->fRelayTxes = true; + } } // Disconnect if we connected to ourself @@ -6947,6 +6955,7 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string item.second.RelayTo(pfrom); } + pfrom->nTimeOffset = timeWarning.AddTimeData(pfrom->addr, nTime, GetTime()); pfrom->fSuccessfullyConnected = true; string remoteAddr; @@ -6957,8 +6966,6 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string cleanSubVer, pfrom->nVersion, pfrom->nStartingHeight, addrMe.ToString(), pfrom->id, remoteAddr); - - pfrom->nTimeOffset = timeWarning.AddTimeData(pfrom->addr, nTime, GetTime()); } From 285290fc30ec6e0985978f5656b542a91504a2dc Mon Sep 17 00:00:00 2001 From: Daira Emma Hopwood Date: Tue, 31 Oct 2023 19:04:28 +0000 Subject: [PATCH 3/5] Treat a malformed version message as cause for disconnection. Co-authored-by: Andrea Lanfranchi Signed-off-by: Daira Emma Hopwood --- src/main.cpp | 127 +++++++++++++++++++++++++++------------------------ 1 file changed, 68 insertions(+), 59 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 7b28b627ccd..47661b1cff6 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6796,12 +6796,14 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string // Each connection can only send one version message if (pfrom->nVersion != 0) { - pfrom->PushMessage("reject", strCommand, REJECT_DUPLICATE, string("Duplicate version message")); + pfrom->PushMessage("reject", strCommand, REJECT_DUPLICATE, std::string("Duplicate version message")); LOCK(cs_main); Misbehaving(pfrom->GetId(), 1); return false; } + // It's necessary to initialize nVersion because if the message is less than 4 bytes, + // it will not be set (but is read in the catch block for std::ios_base::failure). int32_t nVersion = 0; int64_t nTime = 0; CAddress addrMe; @@ -6809,71 +6811,78 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string uint64_t nNonce = 1; std::string strSubVer; std::string cleanSubVer; - uint64_t nServices; - vRecv >> nVersion >> nServices >> nTime >> addrMe; + try { + uint64_t nServices; + vRecv >> nVersion >> nServices >> nTime >> addrMe; - if (nVersion < MIN_PEER_PROTO_VERSION) - { - // disconnect from peers older than this proto version - LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, nVersion); - pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE, - strprintf("Version must be %d or greater", MIN_PEER_PROTO_VERSION)); - pfrom->fDisconnect = true; - return false; - } + if (nVersion < MIN_PEER_PROTO_VERSION) + { + // disconnect from peers older than this proto version + LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, nVersion); + pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE, + strprintf("Version must be %d or greater", MIN_PEER_PROTO_VERSION)); + pfrom->fDisconnect = true; + return false; + } - if (chainparams.NetworkIDString() == "test" && - nVersion < MIN_TESTNET_PEER_PROTO_VERSION) - { - // disconnect from testnet peers older than this proto version - LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, nVersion); - pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE, - strprintf("Version must be %d or greater", MIN_TESTNET_PEER_PROTO_VERSION)); - pfrom->fDisconnect = true; - return false; - } + if (chainparams.NetworkIDString() == "test" && + nVersion < MIN_TESTNET_PEER_PROTO_VERSION) + { + // disconnect from testnet peers older than this proto version + LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, nVersion); + pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE, + strprintf("Version must be %d or greater", MIN_TESTNET_PEER_PROTO_VERSION)); + pfrom->fDisconnect = true; + return false; + } - // Reject incoming connections from nodes that don't know about the current epoch - const Consensus::Params& consensusParams = chainparams.GetConsensus(); - auto currentEpoch = CurrentEpoch(GetHeight(), consensusParams); - if (nVersion < consensusParams.vUpgrades[currentEpoch].nProtocolVersion && - !( - chainparams.NetworkIDString() == "regtest" && - !GetBoolArg("-nurejectoldversions", DEFAULT_NU_REJECT_OLD_VERSIONS) - ) - ) { - LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, nVersion); - pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE, - strprintf("Version must be %d or greater", - consensusParams.vUpgrades[currentEpoch].nProtocolVersion)); - pfrom->fDisconnect = true; - return false; - } + // Reject incoming connections from nodes that don't know about the current epoch + const Consensus::Params& consensusParams = chainparams.GetConsensus(); + auto currentEpoch = CurrentEpoch(GetHeight(), consensusParams); + if (nVersion < consensusParams.vUpgrades[currentEpoch].nProtocolVersion && + !( + chainparams.NetworkIDString() == "regtest" && + !GetBoolArg("-nurejectoldversions", DEFAULT_NU_REJECT_OLD_VERSIONS) + ) + ) { + LogPrintf("peer=%d using obsolete version %i; disconnecting\n", pfrom->id, nVersion); + pfrom->PushMessage("reject", strCommand, REJECT_OBSOLETE, + strprintf("Version must be %d or greater", + consensusParams.vUpgrades[currentEpoch].nProtocolVersion)); + pfrom->fDisconnect = true; + return false; + } - // We've successfully parsed the mandatory fields and checked the version. - // It's safe to leave these set even if subsequent parsing fails. - pfrom->nVersion = nVersion; - pfrom->nServices = nServices; + // We've successfully parsed the mandatory fields and checked the version. + // It's safe to leave these set even if subsequent parsing fails. + pfrom->nVersion = nVersion; + pfrom->nServices = nServices; - if (!vRecv.empty()) { - vRecv >> addrFrom >> nNonce; - } - if (!vRecv.empty()) { - vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH); - cleanSubVer = SanitizeString(strSubVer, SAFE_CHARS_SUBVERSION); - } - if (!vRecv.empty()) { - int32_t nStartingHeight; - vRecv >> nStartingHeight; - pfrom->nStartingHeight = nStartingHeight; - } - { - LOCK(pfrom->cs_filter); if (!vRecv.empty()) { - vRecv >> pfrom->fRelayTxes; // set to true after we get the first filter* message - } else { - pfrom->fRelayTxes = true; + vRecv >> addrFrom >> nNonce; + } + if (!vRecv.empty()) { + vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH); + cleanSubVer = SanitizeString(strSubVer, SAFE_CHARS_SUBVERSION); + } + if (!vRecv.empty()) { + int32_t nStartingHeight; + vRecv >> nStartingHeight; + pfrom->nStartingHeight = nStartingHeight; } + { + LOCK(pfrom->cs_filter); + if (!vRecv.empty()) { + vRecv >> pfrom->fRelayTxes; // set to true after we get the first filter* message + } else { + pfrom->fRelayTxes = true; + } + } + } catch (const std::ios_base::failure&) { + LogPrintf("peer=%d using version %i sent malformed version message; disconnecting\n", pfrom->id, nVersion); + pfrom->PushMessage("reject", strCommand, REJECT_MALFORMED, std::string("Malformed version message")); + pfrom->fDisconnect = true; + return false; } // Disconnect if we connected to ourself From e6ac385de4b5a891c1af29e3384ed2ccd22125dc Mon Sep 17 00:00:00 2001 From: Daira Emma Hopwood Date: Tue, 31 Oct 2023 20:11:59 +0000 Subject: [PATCH 4/5] Reject duplicate verack. Co-authored-by: Andrea Lanfranchi Signed-off-by: Daira Emma Hopwood --- src/main.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 47661b1cff6..14acd9d6e58 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6989,12 +6989,19 @@ bool static ProcessMessage(const CChainParams& chainparams, CNode* pfrom, string else if (strCommand == "verack") { + LOCK(cs_main); + CNodeState* state = State(pfrom->GetId()); + assert(state != nullptr); + if (state->fCurrentlyConnected) { + pfrom->PushMessage("reject", strCommand, REJECT_DUPLICATE, std::string("Duplicate verack message")); + Misbehaving(pfrom->GetId(), 1); + return false; + } pfrom->SetRecvVersion(min(pfrom->nVersion, PROTOCOL_VERSION)); // Mark this node as currently connected, so we update its timestamp later. if (pfrom->fNetworkNode) { - LOCK(cs_main); - State(pfrom->GetId())->fCurrentlyConnected = true; + state->fCurrentlyConnected = true; } } From abe934ce79af8c0a6ca3fb4b09b8baa958159a3d Mon Sep 17 00:00:00 2001 From: Daira-Emma Hopwood Date: Fri, 17 May 2024 13:30:13 +0100 Subject: [PATCH 5/5] net: define NodeId as an int64_t This should make occurrences of NodeId wrapping essentially impossible for real-world usage. Backport of https://github.com/bitcoin/bitcoin/pull/10176 [zcashd] I have checked that zcashd has no current uses of NodeId that depend on it being an `int`. All accesses to the global `nLastNodeId` are under lock. `NodeId` *is* required to be a signed integral type, because `-1` is used as a sentinel value. It is also formatted using the `%d` tinyformat specifier, but unlike the C format specifier it is inspired by, this correctly handles integral types of arbitrary width. There are `NodeId` fields in `CNodeStats`, `NodeEvictionCandidate`, and (test-only) `COrphanTx`, but those types are not serializable, and there is no other ad-hoc serialization of `NodeId` values apart from its use in the "id" field of the output from the `getpeerinfo` RPC. `UniValue` has an override of `pushKV` for `int64_t`, and so that use will correctly handle values up to the [maximum safe JSON/JavaScript integer](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER), i.e. $2^{53} - 1$. As upstream did, we argue that it is not feasible to cause that value to be exceeded. Co-authored-by: Cory Fields Signed-off-by: Daira-Emma Hopwood --- src/net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.h b/src/net.h index 51b902202a3..23c70c3e6ac 100644 --- a/src/net.h +++ b/src/net.h @@ -103,7 +103,7 @@ void StartNode(boost::thread_group& threadGroup, CScheduler& scheduler); bool StopNode(); void SocketSendData(CNode *pnode); -typedef int NodeId; +typedef int64_t NodeId; struct CombinerAll {