Introduce whitelisted peers.

sipa commented at 12:18 pm on June 21, 2014: member

This adds a -whitelist option to specify subnet ranges from which peers that connect are whitelisted. In addition, there is a -whitebind option which works like -bind, except peers connecting to it are also whitelisted (allowing a separate listen port for trusted connections).

Being whitelisted has two effects (for now):

They are immune to DoS banning.
Transactions they broadcast (which are valid) are always relayed, even if they were already in the mempool. This means that a node can function as a gateway for a local network, and that rebroadcasts from the local network will work as expected.

Whitelisting replaces the magic exemption localhost had for DoS banning, which implies hidden service connects (from a localhost Tor node) were incorrectly immune to DoS banning as well. This old behaviour is removed for that reason, but can be restored using -whitelist=127.0.0.1 or -whitelist=::1. -whitebind is safer to use in case non-trusted localhost connections are expected (like hidden services).

This is a partial replacement for #3403 (but does not add RPC commands to make whitelisting dynamic). Also, hhitelisting becomes a boolean property of a peer here (set at connect time), rather than defined by a set of netmasks. This means we don’t need to match the address on every invocation of a relay.

dsnark commented at 2:41 pm on June 21, 2014: none

Appears functional with a quick test. Was able to bind two different ports and have clients connected to each, an intentionally misbehaving node on the whitelisted interface was not banned. Would it be sensible to show this as a line in getpeerinfo as well?

jgarzik commented at 2:43 pm on June 21, 2014: contributor

Comments:

Code review ACK
Agree w/ @dsnark that showing “I’m whitelisted” in getpeerinfo would be nice
-whitebind looks correct, but makes me nervous. Was this a requested feature? An IP address whitelist is consciously built, while a -whitebind port hopes the whitelist is consciously built elsewhere.

It seems easier for sysadmins, etc. to make a bad mistake with -whitebind, even though it might seem more convenient at first.

sipa commented at 2:57 pm on June 21, 2014: member

@jgarzik -whitelist just is not enough when you have both trusted and untrusted local services running. Right now, we’re unable to perform DoS banning on hidden service peers because they appear to be coming from localhost.

sipa commented at 3:08 pm on June 21, 2014: member

Added ‘whitelisted’ to getpeerinfo.

sipa commented at 3:09 pm on June 21, 2014: member

@dsnark Thanks for testing!

jgarzik commented at 3:10 pm on June 21, 2014: contributor

Agree that is an issue, but this is still adding a Big Ole Security Exemption. I would suggest inverting the problem, to achieve a more secure result:

Add -torbind (-shadybind?), where all incoming connections are untrusted, and point Tor hidden service at this port
Mark connections coming into this port with a special taint flag, so that they cannot be whitelisted

I think it would be useful to distinguish between localhost and Tor in any case. Another useful side effect of “inverting the problem in a more secure way.”

sipa commented at 3:24 pm on June 21, 2014: member

Interesting idea, but I’m not sure I like the double exception behavior of “normally peers are subjected to DoS banning, UNLESS they are whitelisted, which happens when they’re in a whitelisted range, UNLESS they are connecting to shadybind.”

jgarzik commented at 3:33 pm on June 21, 2014: contributor

Fundamentally, it is still adding a “default: insecure” feature where the only justification for such insecurity is not a use case, a user request, but an implementation artifact (“that’s how our code works”).

Based on the problem description, it sounds like the needs are

-whitelist
-torbind
Fix our stupid DoS code, so that bitcoind can recognize that -torbind nodes have a second level hierarchy, and we need to DoS-ban a single Tor peer, not all of Tor.

We should not add -whitebind insecurity just because our DoS banning code is too stupid to figure out how to ban a single Tor node.

dsnark commented at 3:48 pm on June 21, 2014: none

we need to DoS-ban a single Tor peer, not all of Tor.

If the peers are connected to a Hidden Service they can’t be identified due to the design of the network, they’ll always be localhost. I don’t see a way around that outside of just not allowing HS peers.

jgarzik commented at 3:55 pm on June 21, 2014: contributor

@dsnark You must consider the design of Tor + bitcoin, not just Tor alone.

sipa commented at 3:55 pm on June 21, 2014: member

@dsnark You’re absolutely right… I didn’t think about that.

Still, we want to at least be able to disconnect a misbehaving Tor peer, and not exclude them from DoS banning just because they seem to be connecting from localhost. However, it’s not as simple as this patch, as we need also a way to not ban 127.0.0.1.

sipa commented at 6:32 pm on June 21, 2014: member

@jgarzik Not disagreeing with you, but I’m not sure that potential security exception weighs up against the more complex semantics.

I’ll wait for some more opinions.

gmaxwell commented at 8:54 pm on June 21, 2014: contributor

@jgarzik In a lot of places the network security policy is implemented elsewhere, blocking that is annoying. The harm of being whitelisted inappropriately is also small— though I’m a little worried that the unconditional relaying is a little bit too powerful… since it does make you into a blind attack multiplier.

This is also a little coarse in that immunity from ban is not the same as immunity from disconnection. I think localhost should still remain immune from banning, without being immune from disconnection. (disconnection suffers no overkill)

sipa commented at 3:29 pm on June 22, 2014: member

So, two solutions:

Either adding special logic for localhost again to prevent it from being banned (even though connections from it may be DoS disconnected) [implemented now in this PR, as it was trivial]
Or add Jeff’s suggested -shadybind, and give it that the extra logic of never banning.

Also new suggested names: -trusted and -trustedbind (instead of -whitelist and -whitebind). -shadybind then becomes -untrustedbind.

sipa commented at 3:39 pm on June 22, 2014: member

@gmaxwell There is still the setInventoryKnown caching. One single inv will never be announced twice to the same peer (unless the setInventoryKnown cache overflows and is pruned). The largest advantage to rebroadcasts is that it does relay to new peers since the previous broadcast.

BitcoinPullTester commented at 4:10 pm on June 22, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4378_95b5f1961edb7a8b393d1cb298de7756621da20d for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

gmaxwell commented at 11:44 pm on June 22, 2014: contributor

A third thought: extend the whitelist interface so that an ACL entry can specify the port that it was connected to, then instead of trusted bind you’d be able to just bind multiple ports and write whitelist entries like 0/0:1234.

I still suspect that special casing localhost to prevent banning is something we want to do, even for onion peers. Banning all onion peers because one behaves is the wrong behavior (and creates a vulnerability where we currently have none: a single trouble maker could turn off bitcoin on tor cheaply, in order to be able to deanonymize users when they drop off tor to get things working).

laanwj commented at 8:14 am on June 25, 2014: member

Extending the ACLs to include what port/interface it is connected to sounds like feature creep to me. Too much work to maintain and test all that.

In cases such advanced configurability is needed I suggest using the host firewall combined with trustedbind to control access to a specific port.

mikehearn commented at 10:09 am on June 27, 2014: contributor

For as long as anti-DoS policy involves the notion of banning peers, it won’t be completely compatible with Tor either via hidden services or exit nodes. We can still use Tor when it works and hope that some attacker that is willing to very noisily interfere with the entire network to force people back onto the clearnet doesn’t come along.

Eventually perhaps we’ll have an alternative strategy that doesn’t rely on bans, which anyway aren’t really effective with large a enough botnet. Until then I don’t think we should hold up this kind of nice improvement on the behalf of Tor.

sipa commented at 4:12 pm on July 9, 2014: member

Rebased.

Introduce whitelisted peers.

This adds a -whitelist option to specify subnet ranges from which peers
that connect are whitelisted. In addition, there is a -whitebind option
which works like -bind, except peers connecting to it are also
whitelisted (allowing a separate listen port for trusted connections).

Being whitelisted has two effects (for now):
* They are immune to DoS disconnection/banning.
* Transactions they broadcast (which are valid) are always relayed,
  even if they were already in the mempool. This means that a node
  can function as a gateway for a local network, and that rebroadcasts
  from the local network will work as expected.

Whitelisting replaces the magic exemption localhost had for DoS
disconnection (local addresses are still never banned, though), which
implied hidden service connects (from a localhost Tor node) were
incorrectly immune to DoS disconnection as well. This old
behaviour is removed for that reason, but can be restored using
-whitelist=127.0.0.1 or -whitelist=::1 can be specified. -whitebind
is safer to use in case non-trusted localhost connections are expected
(like hidden services).

dc942e6f27

BitcoinPullTester commented at 7:01 pm on July 9, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4378_dc942e6f276b9fabc21f06d11cd16871d4054f82/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj commented at 6:11 am on July 10, 2014: member

ACK

laanwj merged this on Jul 14, 2014

laanwj closed this on Jul 14, 2014

laanwj referenced this in commit c9bc398ad9 on Jul 14, 2014

in src/main.cpp: in dc942e6f27

1424@@ -1425,7 +1425,8 @@ void Misbehaving(NodeId pnode, int howmuch)
1425         return;
1426 
1427     state->nMisbehavior += howmuch;
1428-    if (state->nMisbehavior >= GetArg("-banscore", 100))
1429+    int banscore = GetArg("-banscore", 100);
1430+    if (state->nMisbehavior >= banscore && state->nMisbehavior - howmuch < banscore)

rebroad commented at 4:35 am on July 16, 2014:

Why? This will cause any misbehaviour that is equal to or greater than the threshold set to cause the node to never be disconnected!

gmaxwell commented at 1:55 pm on July 16, 2014:

As I pointed out in #4378 this isn’t the case. (though I’m glad you’re reading the code!)

rebroad referenced this in commit 6583b60a5d on Jul 16, 2014

rebroad referenced this in commit d398f1e90e on Jul 16, 2014

rebroad referenced this in commit 5eec1fea68 on Jul 17, 2014

MarcoFalke locked this on Sep 8, 2021

Introduce whitelisted peers. #4378