p2p: Do not relay banned IP addresses #15617

pull sipa wants to merge 1 commits into bitcoin:master from sipa:201903_nobannedrelay changing 1 files +6 −2

sipa commented at 5:07 AM on March 18, 2019: member

No description provided.
Do not relay banned IP addresses 054d01d0a8
fanquake added the label P2P on Mar 18, 2019
gmaxwell commented at 5:17 AM on March 18, 2019: contributor

Concept ACK. utACK, will test.
naumenkogs commented at 7:21 AM on March 18, 2019: member

utACK
jonasschnelli commented at 7:55 AM on March 18, 2019: contributor

utACK 054d01d0a87a5adc43428588ecc29f1339a69dd2
practicalswift commented at 8:29 AM on March 18, 2019: contributor

utACK 054d01d0a87a5adc43428588ecc29f1339a69dd2
MarcoFalke renamed this:
~~Do not relay banned IP addresses~~
p2p: Do not relay banned IP addresses
on Mar 18, 2019
promag commented at 6:44 PM on March 18, 2019: member

How can this be tested?
gmaxwell commented at 6:55 PM on March 18, 2019: contributor

ACK (now tested it, appears to work)
gmaxwell commented at 2:24 AM on March 19, 2019: contributor

@promag Instrument your node to log the addr message it sends, ban stuff, check that it's not relaying the banned stuff... which is what I did :) (or otherwise, run the patch, and observe that nothing catches fire)
promag commented at 5:01 AM on March 19, 2019: member

@gmaxwell I mean it could have a functional test or something.
gmaxwell commented at 8:29 AM on March 19, 2019: contributor

I sure hope that when people say they've tested a PR it doesn't mean they just ran its unit test...
promag commented at 9:19 AM on March 19, 2019: member

I hope too, but a test would be a nice addition no?
practicalswift commented at 2:02 PM on March 19, 2019: contributor

Nit: Do we care about the object slicing going on here (slicing from CAddress to CNetAddr)?

(Note: Slicing was present in this code also before this patch.)
laanwj added this to the milestone 0.18.0 on Mar 19, 2019
laanwj added the label Needs backport on Mar 19, 2019
pstratem commented at 10:07 PM on March 19, 2019: contributor

utACK 054d01d0a87a5adc43428588ecc29f1339a69dd2
jonasschnelli commented at 8:17 AM on March 20, 2019: contributor

I sure hope that when people say they've tested a PR it doesn't mean they just ran its unit test...

Heh. Indeed. Though, the functional tests are usually a good starting point in how to understand how manual testing can be done...
laanwj commented at 10:30 AM on March 20, 2019: member

I sure hope that when people say they've tested a PR it doesn't mean they just ran its unit test...

I don't hope so either. Would make sense to extend the definition of ACKs in CONTRIBUTING.md to describe that a tested ACK involves change-specific manual testing, and in case it's not obvious how that's done, it should be described in the post.

(not trying to say that running the unit+functional tests locally isn't useful! travis cannot possible cover all possible combinations of hardware and software)

utACK 054d01d0a87a5adc43428588ecc29f1339a69dd2
laanwj merged this on Mar 20, 2019
laanwj closed this on Mar 20, 2019
laanwj referenced this in commit 81f732bcaa on Mar 20, 2019
laanwj removed the label Needs backport on Mar 20, 2019
laanwj referenced this in commit 238ef33692 on Mar 20, 2019
jonatack referenced this in commit 731414d4d4 on Mar 20, 2019
jonatack referenced this in commit 0d9d2b385b on Mar 20, 2019
promag commented at 11:30 AM on March 20, 2019: member

My comment was about having this new behavior checked, not about how reviews should be done.
laanwj referenced this in commit bbc436e09e on Mar 20, 2019
marcinja referenced this in commit 8cfdf0c306 on Mar 20, 2019
marcinja referenced this in commit 2f605968e9 on Mar 20, 2019
luke-jr commented at 8:51 PM on April 17, 2019: member

utACK
HashUnlimited referenced this in commit 1726ca6960 on Apr 19, 2019
HashUnlimited referenced this in commit 6554ceaaad on Aug 30, 2019
barrystyle referenced this in commit 57d5dd36ad on Nov 11, 2019
deadalnix referenced this in commit 4676f0d4cf on May 21, 2020
cculianu commented at 9:31 PM on June 8, 2020: none

You guys realize -- this is a DoS vector. This commit introduces effectively a quadratic loop for GETADDR. IsBanned() is a relatively expensive call which linearly scans the entire ban table after taking a lock. This loop does IsBanned for each item in vAddr up to 2500 items max.

What's worse, the ban table can be manipulated and is unbounded in size -- anybody with enough IP addresses can grow it on any target node they can connect to -- to tens of thousands of entries. This is especially true if using IPv6 where thousands of IP addresses are easily obtained by people with even modest resources. See RFC3177 - Recommendations on IPv6 Address Allocations to Sites

I was able to manipulate the ban table on my testnet node. I added some debug/printing to my local build. This is to service just 1 GETADDR message:

2020-06-08T21:09:40Z GETADDR vAddr: 2500 banSz: 47506 ~iters: 118765000(118764972) took: 2010.761 ms
ftrader referenced this in commit a9e47e7970 on Aug 17, 2020
PastaPastaPasta referenced this in commit 76c9cbf8f4 on Jun 25, 2021
PastaPastaPasta referenced this in commit 328d43c080 on Jun 27, 2021
PastaPastaPasta referenced this in commit 58519790a0 on Jun 27, 2021
PastaPastaPasta referenced this in commit 8923a42950 on Jun 28, 2021
PastaPastaPasta referenced this in commit 02f8e759c1 on Jun 28, 2021
PastaPastaPasta referenced this in commit ab79520dad on Jun 29, 2021
PastaPastaPasta referenced this in commit 6dacf59aed on Jun 29, 2021
PastaPastaPasta referenced this in commit e386e99f01 on Jul 1, 2021
PastaPastaPasta referenced this in commit fb8d075464 on Jul 1, 2021
PastaPastaPasta referenced this in commit 47ddb2619c on Jul 8, 2021
PastaPastaPasta referenced this in commit 368f5f0c9f on Jul 10, 2021
linuxsh2 referenced this in commit 1818cb0256 on Aug 11, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Milestone
0.18.0

Linked (view graph)

#15626 Docs: Update ACK description in CONTRIBUTING.md