net: fix banning and disallow sending messages before receiving verack #9720

theuni commented at 6:45 AM on February 8, 2017: member

This is the last of my net issues for 0.14. As discussed with @TheBlueMatt and @gmaxwell.

Fixes for a few problems discovered while running a network stress/fuzzer:

Remote nodes weren't always banned when they hadn't yet sent a verack. Regression from 7a8c2519015650acd51eaf42719f04e53f839bbe.
Require a verack before sending any non-handshake messages. This is much more straightforward behavior, and allows for tests to be easily written
Now that there's a sane model for testing, add checks for leaky messages sent out before the handshake is complete, as well as for banning in those cases.

fanquake added the label P2P on Feb 8, 2017

fanquake added this to the milestone 0.14.0 on Feb 8, 2017

theuni commented at 7:03 AM on February 8, 2017: member

I see that @TheBlueMatt and I managed to make almost the exact same change: 2a278cdaf6ea46dc85a61a37a12a8acd7acd5670 vs a5032b5b0c55c90a8e4df658d85d99824cf4699d. I'm happy to rebase and drop mine if his goes in first.

Edit: I should also mention that this and #9715 are complementary.

rebroad commented at 10:39 AM on February 8, 2017: contributor

What is the risk by not merging this?

TheBlueMatt commented at 2:02 PM on February 8, 2017: member

Concept ACK, will give this a more full review after breakfast.

On February 8, 2017 1:46:00 AM EST, Cory Fields notifications@github.com wrote:

This is the last of my net issues for 0.14. As discussed with @TheBlueMatt and @gmaxwell.

Fixes for a few problems discovered while running a network stress/fuzzer:

Remote nodes weren't always banned when they hadn't yet sent a verack. Regression from 7a8c2519015650acd51eaf42719f04e53f839bbe.

Require a verack before sending any non-handshake messages. This is much more straightforward behavior, and allows for tests to be easily written

Now that there's a sane model for testing, add checks for leaky messages sent out before the handshake is complete, as well as for banning in those cases. You can view, comment on, or merge this pull request online at:

#9720

-- Commit Summary --

net: correctly ban before the handshake is complete

net: parse reject earlier

net: require a verack before responding to anything else

qa: allow for a node that does not send an initial version message

qa: add a test to detect leaky p2p messages

-- File Changes --

M qa/pull-tester/rpc-tests.py (1) A qa/rpc-tests/p2p-leaktests.py (135) M qa/rpc-tests/test_framework/mininode.py (21) M src/net_processing.cpp (115)

-- Patch Links --

https://github.com/bitcoin/bitcoin/pull/9720.patch https://github.com/bitcoin/bitcoin/pull/9720.diff

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9720

in src/net_processing.cpp:None in 0abd2ccfaa outdated

2593 | @@ -2594,6 +2594,35 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2594 |      return true;
2595 |  }
2596 |  
2597 | +static bool SendRejectsAndCheckBan(CNode* pnode, CConnman& connman)
2598 | +{
2599 | +    AssertLockHeld(cs_main);
2600 | +    CNodeState &state = *State(pnode->GetId());
2601 | +
2602 | +    BOOST_FOREACH(const CBlockReject& reject, state.rejects)

instagibbs commented at 3:40 PM on February 8, 2017:

mind putting braces around this?

theuni commented at 4:38 PM on February 8, 2017:

Will do.

kallewoof commented at 8:51 AM on February 9, 2017:

Nit: avoid BOOST_FOREACH if possible (though this was moved code, so perhaps not applicable, but..).

for (const CBlockReject& reject : state.rejects) [...]

laanwj commented at 10:17 AM on February 9, 2017:

No, for moved code this is not applicable. To avoid confusion, let's keep code style changes, moves and bugfixes separate where possible.

in src/net_processing.cpp:None in 0abd2ccfaa outdated

2593 | @@ -2594,6 +2594,35 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2594 |      return true;
2595 |  }
2596 |  
2597 | +static bool SendRejectsAndCheckBan(CNode* pnode, CConnman& connman)

instagibbs commented at 3:52 PM on February 8, 2017:

nit: The name makes me think true means fShouldBan was set to true. Invert the boolean?

theuni commented at 4:37 PM on February 8, 2017:

Heh, i waffled back and forth on this. Sure.

TheBlueMatt commented at 4:42 PM on February 8, 2017:

The usual fix for waffling is comment and clarify name :p

instagibbs approved

instagibbs commented at 4:12 PM on February 8, 2017: member

utACK 50ce6755e024bc849ef8681f9aaa81ff3f3bca57

I'd also like the commit referred to in 0abd2ccfaa24fdd1e5d24271e2360c4d9d64b662 's commit message to include its commit hash for easier understanding.

theuni force-pushed on Feb 8, 2017

theuni commented at 4:32 PM on February 8, 2017: member

I got a few questions about these changes, so I've updated the commit messages to provide (I hope) better back-story/context.

kallewoof commented at 8:53 AM on February 9, 2017: member

utACK, did not review tests

in src/net_processing.cpp:None in f9829821a2 outdated

2734 | @@ -2706,8 +2735,11 @@ bool ProcessMessages(CNode* pfrom, CConnman& connman, const std::atomic<bool>& i
2735 |              PrintExceptionContinue(NULL, "ProcessMessages()");
2736 |          }
2737 |  
2738 | -        if (!fRet)
2739 | +        if (!fRet) {
2740 |              LogPrintf("%s(%s, %u bytes) FAILED peer=%d\n", __func__, SanitizeString(strCommand), nMessageSize, pfrom->id);
2741 | +            LOCK(cs_main);
2742 | +            SendRejectsAndCheckBan(pfrom, connman);

TheBlueMatt commented at 4:49 PM on February 9, 2017:

There are a few cases it looks like we return true after setting misbehaving, so this probably needs to be checked either way (or those cases need updating - but that might result in double-logging).

theuni commented at 5:30 PM on February 9, 2017:

I was trying to avoid the cs_main lock for each message received, but you're right that it means that we rely on a return value that's not very well-defined.

I'll just make it unconditional. Until we have parallel processing, there's not much harm in that.

TheBlueMatt commented at 5:34 PM on February 9, 2017:

Sounds good.

TheBlueMatt commented at 4:29 PM on February 10, 2017: member

utACK df1a32392933a4f716c53d62703a56e8d8bda9da, did not review tests.

gmaxwell approved

gmaxwell commented at 9:32 PM on February 10, 2017: contributor

ACK (only tested lightly)

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

   0 | @@ -0,0 +1,135 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2017 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +
   6 | +from test_framework.mininode import *
   7 | +from test_framework.test_framework import BitcoinTestFramework
   8 | +from test_framework.util import *
   9 | +import logging

jnewbery commented at 12:27 AM on February 11, 2017:

You can remove this. logging isn't being used.

theuni commented at 2:39 AM on February 11, 2017:

ok

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

   0 | @@ -0,0 +1,135 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2017 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.

jnewbery commented at 12:28 AM on February 11, 2017:

Please add a docstring describing what this test case is doing.

theuni commented at 2:39 AM on February 11, 2017:

ok

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

  19 | +        self.connection = conn
  20 | +
  21 | +    def check(self):
  22 | +        def done():
  23 | +            return self.done
  24 | +        return wait_until(done, timeout=10) and not self.unrequested_msg

jnewbery commented at 12:28 AM on February 11, 2017:

I'm not entirely sure what this is achieving. I think you can remove check() and self.done entirely.

You can check directly that no unexpected messages have been received at the end of the test (see my comment below).

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

  25 | +
  26 | +    def send_message(self, message):
  27 | +        self.connection.send_message(message)
  28 | +
  29 | +    def bad_message(self, message):
  30 | +        self.unrequested_msg = True

jnewbery commented at 12:29 AM on February 11, 2017:

Is unexpected_message a better name than unrequested_message?

theuni commented at 2:40 AM on February 11, 2017:

seems about the same to me, but sure

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

  26 | +    def send_message(self, message):
  27 | +        self.connection.send_message(message)
  28 | +
  29 | +    def bad_message(self, message):
  30 | +        self.unrequested_msg = True
  31 | +        raise Exception("should not have received message: %s" % message.command)

jnewbery commented at 12:30 AM on February 11, 2017:

I don't think you should be raising an exception in the network thread. Just set unexpected_message to True (and optionally print some debug info)

theuni commented at 2:40 AM on February 11, 2017:

ok

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

  49 | +    def on_feefilter(self, conn, message): self.bad_message(message)
  50 | +    def on_sendheaders(self, conn, message): self.bad_message(message)
  51 | +    def on_sendcmpct(self, conn, message): self.bad_message(message)
  52 | +    def on_cmpctblock(self, conn, message): self.bad_message(message)
  53 | +    def on_getblocktxn(self, conn, message): self.bad_message(message)
  54 | +    def on_blocktxn(self, conn, message): self.bad_message(message)

jnewbery commented at 12:34 AM on February 11, 2017:

Sigh. This is verbose and will be incomplete every time new p2p message types are added.

I was tempted to say just override the deliver() message from NodeConnCB but that just feels ugly (deliver() should really be a private method and the testcases just override use the on_ callbacks).

I can't think of anything better here.

theuni commented at 2:49 AM on February 11, 2017:

yes, i hate this too.

Too much to do here but for the future: how about a base class for messages. Then there's an overridable dispatcher that forwards to the individual callbacks. So here, I would just override the dispatcher and check the message type.

Basically just another thin layer under deliver()

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

  78 | +        CLazyNode.__init__(self)
  79 | +
  80 | +    def on_reject(self, conn, message): pass
  81 | +    def on_verack(self, conn, message): pass
  82 | +
  83 | +    # When version is received, don't replay with a verack. Instead, see if the

jnewbery commented at 12:34 AM on February 11, 2017:

s/replay/reply

theuni commented at 2:49 AM on February 11, 2017:

ok

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

  90 | +
  91 | +class P2PLeakTest(BitcoinTestFramework):
  92 | +    def add_options(self, parser):
  93 | +        parser.add_option("--testbinary", dest="testbinary",
  94 | +                          default=os.getenv("BITCOIND", "bitcoind"),
  95 | +                          help="Binary to test max block requests behavior")

jnewbery commented at 12:35 AM on February 11, 2017:

I don't think this parser.add_option line is required.

theuni commented at 2:50 AM on February 11, 2017:

ok. Definitely the c/p from the wrong test isn't needed :)

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

  95 | +                          help="Binary to test max block requests behavior")
  96 | +
  97 | +    def __init__(self):
  98 | +        super().__init__()
  99 | +        self.num_nodes = 1
 100 | +        self.banscore = 10

jnewbery commented at 12:35 AM on February 11, 2017:

Since banscore isn't changing I'd prefer to make it a global constant.

theuni commented at 2:50 AM on February 11, 2017:

I think i'd rather keep this local so that we can use different scores for different tests.

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

  99 | +        self.num_nodes = 1
 100 | +        self.banscore = 10
 101 | +    def setup_network(self):
 102 | +        extra_args = [['-debug', '-banscore='+str(self.banscore)]
 103 | +                      for i in range(self.num_nodes)]
 104 | +        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, extra_args)

jnewbery commented at 12:38 AM on February 11, 2017:

There's only one node here. You can replace the above three lines with:

self.nodes = [start_node(0, self.options.tmpdir, ['-debug', '-banscore='+str(self.banscore)])]

theuni commented at 2:50 AM on February 11, 2017:

ok

MarcoFalke commented at 2:31 PM on February 13, 2017:

Please leave it as is for now. My goal was to generalize this logic and get rid of most setup_network methods in test as the only thing they commonly do is fire up the requested number of nodes and nothing else. So there would be no need to overwrite this function if you want a simple network set up.

MarcoFalke commented at 2:34 PM on February 13, 2017:

Also, it would be easier to adapt if the number of nodes changes.

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

 115 | +        no_version_bannode.add_connection(connections[0])
 116 | +        no_version_idlenode.add_connection(connections[1])
 117 | +        no_verack_idlenode.add_connection(connections[2])
 118 | +
 119 | +        NetworkThread().start()  # Start up network handling in another thread
 120 | +        self.nodes[0].generate(1)

jnewbery commented at 12:38 AM on February 11, 2017:

not needed?

theuni commented at 2:51 AM on February 11, 2017:

Very needed, but needs a comment. We generate a block, and wait a few secs to make sure that it's not relayed to the nodes who haven't versioned/veracked yet.

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

 127 | +        #Give the node enough time to possibly leak out a message
 128 | +        time.sleep(5)
 129 | +
 130 | +        assert(no_version_bannode.check())
 131 | +        assert(no_version_idlenode.check())
 132 | +        assert(no_verack_idlenode.check())

jnewbery commented at 12:39 AM on February 11, 2017:

Might as well just check the variable directly:

        assert(not no_version_bannode.unexpected_message)
        assert(not no_version_idlenode.unexpected_message)
        assert(not no_verack_idlenode.unexpected_message)

theuni commented at 2:52 AM on February 11, 2017:

ok

in qa/rpc-tests/p2p-leaktests.py:None in df1a323929 outdated

 128 | +        time.sleep(5)
 129 | +
 130 | +        assert(no_version_bannode.check())
 131 | +        assert(no_version_idlenode.check())
 132 | +        assert(no_verack_idlenode.check())
 133 | +

jnewbery commented at 12:40 AM on February 11, 2017:

I recommend you disconnect all the connections to the node at the end of the test:

        # Disconnect all peers
        [conn.disconnect_node() for conn in connections]

theuni commented at 2:52 AM on February 11, 2017:

ok

jnewbery commented at 12:44 AM on February 11, 2017: member

Test looks good @theuni ! I've added a bunch of style comments.

theuni commented at 2:54 AM on February 11, 2017: member

@jnewbery Thanks for the great test review. I'm not sure I'll have time to get to this before Sunday, so let's not let it hold back merge if a few more ACKs come in. I'll for sure fix up the tests post-merge if that's the case.

net: correctly ban before the handshake is complete

7a8c251901 made a change to avoid getting into SendMessages() until the
version handshake (VERSION + VERACK) is complete. That was done to avoid
leaking out messages to nodes who could connect, but never bothered sending
us their version/verack.

Unfortunately, the ban tally and possible disconnect are done as part of
SendMessages(). So after 7a8c251901, if a peer managed to do something
bannable before completing the handshake (say send 100 non-version messages
before their version), they wouldn't actually end up getting
disconnected/banned. That's fixed here by checking the banscore as part of
ProcessMessages() in addition to SendMessages().

c45b9fb54c

net: parse reject earlier

Prior to this change, all messages were ignored until a VERSION message was
received, as well as possibly incurring a ban score.

Since REJECT messages can be sent at any time (including as a response to a bad
VERSION message), make sure to always parse them.

Moving this parsing up keeps it from being caught in the
if (pfrom->nVersion == 0) check below.

8502e7acbe

net: require a verack before responding to anything else

7a8c251901 made this logic hard to follow. After that change, messages would
not be sent to a peer via SendMessages() before the handshake was complete, but
messages could still be sent as a response to an incoming message.

For example, if a peer had not yet sent a verack, we wouldn't notify it about
new blocks, but we would respond to a PING with a PONG.

This change makes the behavior straightforward: until we've received a verack,
never send any message other than version/verack/reject.

The behavior until a VERACK is received has always been undefined, this change
just tightens our policy.

This also makes testing much easier, because we can now connect but not send
version/verack, and anything sent to us is an error.

cbfc5a6728

qa: mininode learns when a socket connects, not its first action 5b5e4f8330

qa: Expose on-connection to mininode listeners 8650bbb660

qa: add a test to detect leaky p2p messages

This is certainly not exhaustive, but it's better than nothing. Adds checks
for:

- Any message received before sending a version
- Any message received other than version/reject before sending a verack

It also tries to goad the remote into sending a pong, address, or block
announcement.

d9434918d2

theuni force-pushed on Feb 14, 2017

theuni commented at 12:40 AM on February 14, 2017: member

Fixed up the tests and squashed. Only the tests changed, the bitcoin code is exactly the same as before squash. I've archived the old branch here: https://github.com/theuni/bitcoin/commits/fix-ban2 in case anyone wants to compare.

I replaced my mininode changes with @TheBlueMatt's commits from #9715, so that these won't conflict with eachother.

laanwj merged this on Feb 14, 2017

laanwj closed this on Feb 14, 2017

laanwj referenced this in commit e87ce95fbd on Feb 14, 2017

codablock referenced this in commit cccd334532 on Jan 19, 2018

codablock referenced this in commit fe0ef87cd3 on Jan 23, 2018

HashUnlimited referenced this in commit 5183ade2d4 on Feb 27, 2018

andvgal referenced this in commit 13840cb474 on Jan 6, 2019

CryptoCentric referenced this in commit 6151cd6e1f on Feb 27, 2019

DrahtBot locked this on Sep 8, 2021