From 5e3d9f21df21a822dc210d73a000faba084e6067 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sat, 27 Jul 2024 19:16:11 +0200 Subject: [PATCH 1/6] doc: validation: add a reference to historical header spam vulnerability --- src/validation.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 0384018bc36..3d2fe2364bf 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) { From c02d9f6dd53989f41375f13a2d39270fa5d58a04 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sat, 27 Jul 2024 19:28:21 +0200 Subject: [PATCH 2/6] doc: net_proc: reference past defect regarding invalid GETDATA types --- src/net_processing.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1da3ec9d211..2c2b23395dc 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); From 68ac9542c451c9088c59a3ec6124d87cfd3382a3 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sat, 27 Jul 2024 19:50:45 +0200 Subject: [PATCH 3/6] doc: net_proc: reference past DoS vulnerability in orphan processing --- src/net_processing.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2c2b23395dc..61049f524b7 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3072,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); From 4489117c3f6720ef92a328d3462cec8c0f466ae5 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sun, 28 Jul 2024 10:46:49 +0200 Subject: [PATCH 4/6] doc: txrequest: point to past censorship vulnerability in tx re-request handling --- src/txrequest.h | 4 ++++ 1 file changed, 4 insertions(+) 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. From ad616b6c013e69221f61b695c4ae09a3471c3f7c Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sun, 28 Jul 2024 10:54:38 +0200 Subject: [PATCH 5/6] doc: net: mention past vulnerability as rationale to limit incoming message size --- src/net.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/net.cpp b/src/net.cpp index c722ddfcb5f..e3938842e32 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; From eb0724f0dee307d6d14e47ebd3077b7ffd50f507 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sun, 28 Jul 2024 11:06:55 +0200 Subject: [PATCH 6/6] doc: banman: reference past vuln due to unbounded banlist --- src/banman.h | 5 +++++ 1 file changed, 5 insertions(+) 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 {