Prevent DOS attacks on in-flight data structures #5507

1. 
   ajweiss commented at 11:49 pm on December 18, 2014: contributor

   This is a regression in 0.10 that does not appear in 0.9.x.

   vBlocksInFlight, mapBlocksInFlight and friends can be remotely inflated by supplying an excessive number of block inv messages with garbage hashes. In my local testing, it took about 10 minutes to pump in 1GB of bad inv messages, resulting in 6GB of memory usage and 30GB of outbound getheaders data from bitcoind. After a malicious client disconnects, the memory may be reused, but does not appear to be released back to the operating system.

   The 0.9.x code started applying a 10 point ban penalty at 5000 blocks in flight leading to a ban at 5010 , while under headers first we shouldn’t see more than 16. It’s probably good to have some headroom to prevent buggy or older software from getting banned, but the numbers here are just reasonable guesstimates.

   I don’t like fixed constants, and while MAX_DOWNLOADS_PER_PEER seems to make sense here, I like the idea of leaving some headroom. This bumps it down to 2000 blocks and uses an existing constant that won’t be changing soon.

   This will probably be superceded by #4831, or something built upon it, but a patch for 0.10 seems prudent.

   I’ve tested by sync’ing from genesis, and multiple runs catching up tip-48h to tip. No problems were observed.

2. laanwj added this to the milestone 0.10.0 on Dec 19, 2014
3. 
   laanwj commented at 7:11 am on December 19, 2014: member

   Thanks, good catch, assigned 0.10 milestone.

   utACK

4. 
   sipa commented at 2:29 pm on December 19, 2014: member

   Nice catch, this is indeed a problem.

   I’m less sure about the solution. MarkBlockAsInFlight is only called as a result of a local decision to start fetching a block, so if it overflows, it just means the logic for decision to download blocks in the first place is wrong.

   So, there are two possible code paths that lead to a decision to download a block. One is the ’normal’ headers-based synchronization: see the FindNextBlocksToDownload() call in main.cpp:SendMessages. This one should properly take the number of blocks in flight into account, and can always function as a fallback.

   The other is the ‘direct download’ optimized path, in the “inv” handling in main.cppProcessMessage, which only works in the case we assume we’re already very close to syncing, and is there to reduce latency in waiting for SendMessages, plus avoids a roundtrip between asking the headers and asking the block.

   I think the solution is just disabling the second mechanism in case we’re already downloading too many blocks from the peer under consideration. We shouldn’t look at the total number in flight for this, though, as that would lead to a potential DoS where a few peers spam block announcements, and avoid using the optimized fetching from honest peers.

5. 
   sipa commented at 2:34 pm on December 19, 2014: member

   Also, #4831 is only affects transaction fetching, not block fetching, so a solution is very welcome here, including master.
6. 
   ajweiss commented at 5:32 pm on December 19, 2014: contributor

   Cool! I’ve reverted the original strategy… I assume you mean something like this. I’ve verified that it does indeed stop the attack and I’m playing with testing synchronization under a few conditions now.
7. 
   sipa commented at 3:08 pm on December 22, 2014: member

   Untested ACK (after squash). Testing on testnet now.
8. 
   sipa commented at 8:51 pm on December 22, 2014: member

   Tested ACK.
9. 
   ajweiss commented at 9:38 pm on December 22, 2014: contributor

   afk today, will squash tonight… also tested against bogus inv floods and early/late stage sync…
10. DOS: Respect max per-peer blocks in flight limit

    Don't allow immediate inv driven block downloads if
    a peer already has MAX_BLOCKS_IN_TRANSIT_PER_PEER
    active downloads.  Prevents bogus inv spam from
    blowing up block transfer tracking data structures.

    c90770430d
11. ajweiss force-pushed on Dec 23, 2014
12. 
    ajweiss commented at 7:32 am on December 23, 2014: contributor

    Rebase-squashed. No changes from what sipa tested.
13. 
    btcdrak commented at 8:33 am on December 23, 2014: contributor

    utACK. code is the same after squash.

    nicely found @ajweiss

14. 
    gmaxwell commented at 10:28 am on December 23, 2014: contributor

    ACK.
15. laanwj merged this on Dec 23, 2014
16. laanwj closed this on Dec 23, 2014

    ---

17. laanwj referenced this in commit 844ace95de on Dec 23, 2014
18. laanwj referenced this in commit d10a9015ad on Dec 23, 2014
19. reddink referenced this in commit 8baab45ac9 on Jul 11, 2020
20. reddink referenced this in commit d24bfc3d9f on Jul 14, 2020
21. MarcoFalke locked this on Sep 8, 2021

Contributors
ajweiss laanwj sipa btcdrak gmaxwell

Milestone
0.10.0