p2p: Prevent block index fingerprinting by sending additional getheaders messages #24571

dergoegge · 2022-03-15T16:47:34Z

The block index might contain stale blocks that are not part of the main chain. If a malicious peer is able to probe a node's block index for certain stale blocks then it can use this information to fingerprint the node.

When receiving headers (either through a cmpctblock or headers messages) a node will send getheaders if the predecessor of the first header does not exist. This leaks information from the block index if the predecessor of the header is a stale block because no getheaders will be sent in that case revealing that the stale block exists in the index.

This PR prevents this fingerprinting by sending additional getheaders messages in cases where not doing so leaks the existence of stale blocks. To determine when additional messages should be send, we introduce the PeerManagerImpl::m_chain_tips_sets map which keeps track of seen chain tips per network, effectively creating a per network view of the node's global block index. We only try to accept new headers if they connect to anything in our global index and they connect to our active chain or to a chain that was previously sent to us by a peer on the same network. We send a getheaders message should these conditions not be met.

DrahtBot · 2022-03-15T18:50:41Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	naumenkogs, ariard, fjahr, sdaftuar, stratospher
Stale ACK	jnewbery

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
#26378 (refactor: Pass reference to last header, not pointer by MarcoFalke)
#25968 (Optimizations & simplifications following p2p: Implement anti-DoS headers sync #25717 by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

naumenkogs · 2022-03-19T13:29:04Z

Concept ACK
Neat and creative finding.

As I understood it,
An attacker is currently able to see whether a node received a stale block previously, by sending headers on top of that stale block (does it require proof-of-work for this attack?).
This PR prevents it by sending getheaders even if a block was received already (but was marked stale).

src/net_processing.cpp

dergoegge · 2022-03-21T13:59:01Z

Thank you @naumenkogs for the review!

does it require proof-of-work for this attack?

It does not, the header that the attacker uses can be completely invalid. The proof-of-work check happens later on in ChainstateManager::ProcessNewBlockHeaders. The test i included also uses headers that do not have any proof-of-work on them.

0xB10C

I found the wording of the first sentence of the second paragraph in the commit "[net processing] Prevent block index fingerprinting when receiving he…" to be confusing. It sounds like we now send (in)valid headers to prevent the attack.

This commit prevents malicious peers from performing such an attack by sending (in)valid headers that extend a stale chain on the victims node.

Maybe drop the "performing such an attack"

This commit prevents malicious peers from sending (in)valid headers that extend a stale chain on the victims node.

Maybe relevant for other reviewers: https://bitcoincore.reviews/24571

I'm planning to test this against a node on my reorg-Signet.

src/net_processing.cpp

ariard

Concept ACK.

Do you think such anti-fingerprinting protections should be extended to BIP157 messages request handling ? Even if there are few nodes offering such service now.

src/net_processing.cpp

dergoegge · 2022-04-14T16:13:00Z

Thanks everyone for the review so far! I am working on addressing all your feedback.
I will mark this as a draft for now and rework my approach because currently this messes with large reorgs in some instances. I am working on a functional test for these scenarios which i might PR separately as none of the current tests caught this.

@ariard Good point bringing up BIP157 requests but luckily we already handle them safely, see:

bitcoin/src/net_processing.cpp

Line 2437 in b69fd5e

if (!stop_index || !BlockRequestAllowed(stop_index)) {

fjahr · 2022-04-24T13:36:51Z

Concept ACK

dergoegge · 2022-04-27T11:41:03Z

I updated this PR with the new approach (see commits and new PR description) and it’s now ready for review again. Most of the review comments did not apply anymore so i marked them as resolved.

The previous approach can be found on this branch.
The test for the reorg issue i mentioned before can be found here.

jnewbery

Concept ACK. Nice find.

As an alternative approach, did you consider pruning stale tips from the block index once they're more than a certain age (eg a month)? Or perhaps just making them unavailable from outside validation if no peer has informed you about them for some period of time.

src/net_processing.cpp

This commit extracts the logic from the addr cache id generation into its own function (GetUniqueNetworkID).

dergoegge · 2022-09-12T19:21:00Z

Rebased.

@naumenkogs Thanks for the review! I have addressed some of your comments and will get to the others as soon as I can.

This commit introduces the `PeerManagerImpl::m_chain_tip_sets` map which keeps track of seen chain tips per network. No DoS risk is introduced by keeping these sets of chain tips since each set in the map will have at most as many entries as there are chain tips in our global index.

The block index might contain stale blocks that are not part of the main chain. If a malicious peer is able to probe a node's block index for certain stale blocks then it can use this information to fingerprint the node. Before this commit such an attack is possible due to the logic when receiving headers. A malicous peer can probe for a stale block in a node's index by sending a headers message in which the first header extends a stale chain and has invalid proof-of-work. Nodes behave differently when receiving such a headers message, depending on whether or not the stale block exists in its block index. If it exists, the node will disconnect as the proof-of-work on the new header was invalid. If it does not exist, the node will send a getheaders message in an attempt to connect the two chains. In this commit we prevent this attack by making use of the `PeerManagerImpl::m_chain_tip_sets` map which we introduced in the previous commit. We only try to accept new headers if they connect to anything in our global index and they connect to our active chain or to a chain that was previously sent to us by a peer on the same network. We send a getheaders message should these conditions not be met.

This commit prevents the same attack that was described in the previous commit when receiving compact blocks.

naumenkogs · 2022-09-16T10:47:30Z

src/net.h

@@ -1167,6 +1167,13 @@ class CConnman
    friend struct ConnmanTestMsg;
 };

+/**
+ * Get a unique identifier for the network (e.g. Tor, IPv4/6) of a connection.


nit: From my reading of the first line of the comment, it looks like this is just a network type identifier (without the local socket). e.g., tor=0, ipv4=1, etc.

DrahtBot · 2022-12-08T16:29:45Z

🐙 This pull request conflicts with the target branch and needs rebase.

stratospher

Concept ACK. I've tested this out on a couple of reorg scenarios in regtest. Liked how test_header_leak_via_headers() checks whether stale header leaks should happen or not.

Summarising this PR in case it's useful to other reviewers - if a node on 2 different networks have same stale block indices, it's highly possible that it's the same node (fingerprinting).
behaviour on master :

node1 sends sequence of headers built on some stale block to node2.
if node2 knows stale block, node2 won't send GETHEADERS
if node2 doesn't know stale block, node2 sends GETHEADERS (of it’s active chain tip)
this behaviour difference can be used to identify the stale blocks a node has.

behaviour on PR:

node1 sends sequence of headers built on some stale block to node2.
if node2 knows stale block, node2 sends GETHEADERS (of it’s active chain tip)
if node2 doesn't know stale block, node2 sends GETHEADERS (of it’s active chain tip)
presence/absence of stale blocks can't be identified now.

stratospher · 2023-03-03T19:21:59Z

src/net_processing.cpp

+     * replacing an existing tip if it is an ancestor of the new block or by
+     * adding a new tip if none of the existing chain tips were replaced.
+     */
+    void UpdateChainTipSet(const CNode& node, const CBlockIndex& potential_new_tip)


did i understand this correctly? - UpdateChainTipSet updates chain tip set based on:
1. the whole block/valid headers we received and were told of from other nodes on the same network
2. when we switch networks, chain tip set initially would only contain tip of active chain.
3. inbound and outbound connections on same network would have different chain tip set. is this intentional?

if we mine a block and tell other nodes on the network about it, should we update the chain tip set? (i haven't checked if UpdateChainTipSet is called in all the locations.) are there other situations in which a block isn’t inserted into the chain tip set?

stratospher · 2023-03-03T19:25:00Z

src/net_processing.cpp

+    const CBlockIndex* index{m_chainman.m_blockman.LookupBlockIndex(hash)};
+    if (!index) return nullptr;
+
+    if (IsAncestorOfBestHeaderOrTip(index) ||


IsAncestorOfBestHeaderOrTip would already by contained in IsBlockInChainTipSet right? do we include IsAncestorOfBestHeaderOrTip check for performance gain instead of iterating over all the chain tips? or is there some other reason?

dergoegge · 2023-03-19T15:06:43Z

@stratospher Thank you for having a look at this! There is an alternative approach to the one in this PR that I have been meaning to implement and I forgot to put this PR in draft mode.

We made some changes to the headers sync logic (#25454), which causes us to now only have one getheaders request in flight for the same peer. If we start storing the locator that is sent in the getheaders message and only allow headers in response that connect to the locator, then the issue described in this PR is avoided as well. There are some edge cases around headers announcements which are received without sending a getheaders prior but otherwise this approach would be much less complex.

dergoegge · 2023-05-30T15:21:38Z

Closing for now, can be marked up for grabs.

Someone should look into implementing the alternative approach I have described here: #24571 (comment).

fanquake added the P2P label Mar 15, 2022

dergoegge force-pushed the 2022-03-header-fingerprinting branch from a2b09e8 to 73b8e05 Compare March 15, 2022 18:11

dergoegge force-pushed the 2022-03-header-fingerprinting branch from 73b8e05 to b4d108e Compare March 15, 2022 19:41

This was referenced Mar 15, 2022

blockman: Properly guard blockfile members #24456

Closed

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

naumenkogs reviewed Mar 19, 2022

View reviewed changes