diff --git a/src/banman.h b/src/banman.h index 57ba2ac23ce..23e19506df6 100644 --- a/src/banman.h +++ b/src/banman.h @@ -54,6 +54,11 @@ class CSubNet; // transaction that fails a policy check and a future version changes the // policy check so the transaction is accepted, then that transaction could // cause the network to split between old nodes and new nodes. +// +// NOTE: previously a misbehaving peer would get banned instead of discouraged. +// This meant a peer could unboundedly grow our in-memory map of banned ips. When +// receiving an ADDR message we would also compare every address received to every +// item in the map. See https://bitcoincore.org/en/2024/07/03/disclose-unbounded-banlist. class BanMan { diff --git a/src/net.cpp b/src/net.cpp index 735985a8414..69551cc52e9 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -761,6 +761,8 @@ int V1Transport::readHeader(Span msg_bytes) } // reject messages larger than MAX_SIZE or MAX_PROTOCOL_MESSAGE_LENGTH + // NOTE: failing to perform this check previously allowed a malicious peer to make us allocate 32MiB of memory per + // connection. See https://bitcoincore.org/en/2024/07/03/disclose_receive_buffer_oom. if (hdr.nMessageSize > MAX_SIZE || hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) { LogDebug(BCLog::NET, "Header error: Size too large (%s, %u bytes), peer=%d\n", SanitizeString(hdr.GetMessageType()), hdr.nMessageSize, m_node_id); return -1; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1da3ec9d211..61049f524b7 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2426,6 +2426,9 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic } // else: If the first item on the queue is an unknown type, we erase it // and continue processing the queue on the next call. + // NOTE: previously we wouldn't do so and the peer sending us a malformed GETDATA could + // result in never making progress and this thread using 100% allocated CPU. See + // https://bitcoincore.org/en/2024/07/03/disclose-getdata-cpu. } peer.m_getdata_requests.erase(peer.m_getdata_requests.begin(), it); @@ -3069,6 +3072,8 @@ void PeerManagerImpl::ProcessPackageResult(const node::PackageToValidate& packag } } +// NOTE: the orphan processing used to be uninterruptible and quadratic, which could allow a peer to stall the node for +// hours with specially crafted transactions. See https://bitcoincore.org/en/2024/07/03/disclose-orphan-dos. bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) { AssertLockHeld(g_msgproc_mutex); diff --git a/src/txrequest.h b/src/txrequest.h index fe793210935..10667554355 100644 --- a/src/txrequest.h +++ b/src/txrequest.h @@ -92,6 +92,10 @@ * peers with a nonzero number of tracked announcements. * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of * announcements affected by an operation (amortized O(1) per announcement). + * + * Context: + * - In an earlier version of the transaction request logic it was possible for a peer to prevent us from seeing a + * specific transaction. See https://bitcoincore.org/en/2024/07/03/disclose_already_asked_for. */ class TxRequestTracker { // Avoid littering this header file with implementation details. diff --git a/src/validation.cpp b/src/validation.cpp index a1ac4e1e145..fed260aa932 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4202,6 +4202,10 @@ arith_uint256 CalculateClaimedHeadersWork(std::span headers) * enforced in this function (eg by adding a new consensus rule). See comment * in ConnectBlock(). * Note that -reindex-chainstate skips the validation that happens here! + * + * NOTE: failing to check the header's height against the last checkpoint's opened a DoS vector between + * v0.12 and v0.15 (when no additional protection was in place) whereby an attacker could unboundedly + * grow our in-memory block index. See https://bitcoincore.org/en/2024/07/03/disclose-header-spam. */ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidationState& state, BlockManager& blockman, const ChainstateManager& chainman, const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {