Prevent DOS attacks on in-flight data structures #5507
Conversation
|
Thanks, good catch, assigned 0.10 milestone. utACK |
|
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. |
|
Also, #4831 is only affects transaction fetching, not block fetching, so a solution is very welcome here, including master. |
|
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. |
|
Untested ACK (after squash). Testing on testnet now. |
|
Tested ACK. |
|
afk today, will squash tonight... also tested against bogus inv floods and early/late stage sync... |
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.
ajweiss
force-pushed
the
ajw_1412_bound_inflight_data
branch
from
a81e06e to
c907704
Compare
|
Rebase-squashed. No changes from what sipa tested. |
|
utACK. code is the same after squash. nicely found @ajweiss |
|
ACK. |
c907704 DOS: Respect max per-peer blocks in flight limit (Adam Weiss)
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. Rebased-From: c907704 Github-Pull: bitcoin#5507 (cherry picked from commit d10a901)
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. Rebased-From: c907704 Github-Pull: bitcoin#5507 (cherry picked from commit d10a901)
5 participants
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.