New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce whitelisted peers. #4378

Merged
merged 1 commit into from Jul 14, 2014

Conversation

Projects
None yet
9 participants
@sipa
Member

sipa commented Jun 21, 2014

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

This comment has been minimized.

Show comment
Hide comment
@dsnark

dsnark Jun 21, 2014

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?

dsnark commented Jun 21, 2014

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

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 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.

Contributor

jgarzik commented Jun 21, 2014

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 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.

Member

sipa commented Jun 21, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 21, 2014

Member

Added 'whitelisted' to getpeerinfo.

Member

sipa commented Jun 21, 2014

Added 'whitelisted' to getpeerinfo.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 21, 2014

Member

@dsnark Thanks for testing!

Member

sipa commented Jun 21, 2014

@dsnark Thanks for testing!

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 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."

Contributor

jgarzik commented Jun 21, 2014

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 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."

Member

sipa commented Jun 21, 2014

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

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 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.

Contributor

jgarzik commented Jun 21, 2014

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

This comment has been minimized.

Show comment
Hide comment
@dsnark

dsnark Jun 21, 2014

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.

dsnark commented Jun 21, 2014

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

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 21, 2014

Contributor

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

Contributor

jgarzik commented Jun 21, 2014

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 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.

Member

sipa commented Jun 21, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 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.

Member

sipa commented Jun 21, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 21, 2014

Member

@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)

Member

gmaxwell commented Jun 21, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 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.

Member

sipa commented Jun 22, 2014

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 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.

Member

sipa commented Jun 22, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@BitcoinPullTester

BitcoinPullTester Jun 22, 2014

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.

BitcoinPullTester commented Jun 22, 2014

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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 22, 2014

Member

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).

Member

gmaxwell commented Jun 22, 2014

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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.

Member

laanwj commented Jun 25, 2014

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

This comment has been minimized.

Show comment
Hide comment
@mikehearn

mikehearn Jun 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.

Contributor

mikehearn commented Jun 27, 2014

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 9, 2014

Member

Rebased.

Member

sipa commented Jul 9, 2014

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).
@BitcoinPullTester

This comment has been minimized.

Show comment
Hide comment
@BitcoinPullTester

BitcoinPullTester Jul 9, 2014

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.

BitcoinPullTester commented Jul 9, 2014

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 10, 2014

Member

ACK

Member

laanwj commented Jul 10, 2014

ACK

@laanwj laanwj merged commit dc942e6 into bitcoin:master Jul 14, 2014

laanwj added a commit that referenced this pull request Jul 14, 2014

Merge pull request #4378
dc942e6 Introduce whitelisted peers. (Pieter Wuille)
@@ -1425,7 +1425,8 @@ void Misbehaving(NodeId pnode, int howmuch)
return;
state->nMisbehavior += howmuch;
if (state->nMisbehavior >= GetArg("-banscore", 100))
int banscore = GetArg("-banscore", 100);
if (state->nMisbehavior >= banscore && state->nMisbehavior - howmuch < banscore)

This comment has been minimized.

@rebroad

rebroad Jul 16, 2014

Contributor

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

@rebroad

rebroad Jul 16, 2014

Contributor

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

This comment has been minimized.

@gmaxwell

gmaxwell Jul 16, 2014

Member

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

@gmaxwell

gmaxwell Jul 16, 2014

Member

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

rebroad added a commit to rebroad/bitcoin that referenced this pull request Jul 16, 2014

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jul 16, 2014

@sipa Would be good to update our tor doc to include the behaviour change IMHO. So this removes the DoS-protection which HS-connects had, right?

Diapolo commented on dc942e6 Jul 16, 2014

@sipa Would be good to update our tor doc to include the behaviour change IMHO. So this removes the DoS-protection which HS-connects had, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment