From c45b9fb54c5ca068a5e276c3bd6ebf4ae720f6f7 Mon Sep 17 00:00:00 2001
From: Cory Fields <cory-nospam-@coryfields.com>
Date: Tue, 7 Feb 2017 12:02:02 -0500
Subject: [PATCH 1/6] 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().
---
 src/net_processing.cpp | 60 ++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index bb14e69d83c23..587e857970c48 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2596,6 +2596,36 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
     return true;
 }
 
+static bool SendRejectsAndCheckIfBanned(CNode* pnode, CConnman& connman)
+{
+    AssertLockHeld(cs_main);
+    CNodeState &state = *State(pnode->GetId());
+
+    BOOST_FOREACH(const CBlockReject& reject, state.rejects) {
+        connman.PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, (std::string)NetMsgType::BLOCK, reject.chRejectCode, reject.strRejectReason, reject.hashBlock));
+    }
+    state.rejects.clear();
+
+    if (state.fShouldBan) {
+        state.fShouldBan = false;
+        if (pnode->fWhitelisted)
+            LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode->addr.ToString());
+        else if (pnode->fAddnode)
+            LogPrintf("Warning: not punishing addnoded peer %s!\n", pnode->addr.ToString());
+        else {
+            pnode->fDisconnect = true;
+            if (pnode->addr.IsLocal())
+                LogPrintf("Warning: not banning local peer %s!\n", pnode->addr.ToString());
+            else
+            {
+                connman.Ban(pnode->addr, BanReasonNodeMisbehaving);
+            }
+        }
+        return true;
+    }
+    return false;
+}
+
 bool ProcessMessages(CNode* pfrom, CConnman& connman, const std::atomic<bool>& interruptMsgProc)
 {
     const CChainParams& chainparams = Params();
@@ -2706,8 +2736,12 @@ bool ProcessMessages(CNode* pfrom, CConnman& connman, const std::atomic<bool>& i
             PrintExceptionContinue(NULL, "ProcessMessages()");
         }
 
-        if (!fRet)
+        if (!fRet) {
             LogPrintf("%s(%s, %u bytes) FAILED peer=%d\n", __func__, SanitizeString(strCommand), nMessageSize, pfrom->id);
+        }
+
+        LOCK(cs_main);
+        SendRejectsAndCheckIfBanned(pfrom, connman);
 
     return fMoreWork;
 }
@@ -2773,30 +2807,10 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& interr
         if (!lockMain)
             return true;
 
+        if (SendRejectsAndCheckIfBanned(pto, connman))
+            return true;
         CNodeState &state = *State(pto->GetId());
 
-        BOOST_FOREACH(const CBlockReject& reject, state.rejects)
-            connman.PushMessage(pto, msgMaker.Make(NetMsgType::REJECT, (std::string)NetMsgType::BLOCK, reject.chRejectCode, reject.strRejectReason, reject.hashBlock));
-        state.rejects.clear();
-
-        if (state.fShouldBan) {
-            state.fShouldBan = false;
-            if (pto->fWhitelisted)
-                LogPrintf("Warning: not punishing whitelisted peer %s!\n", pto->addr.ToString());
-            else if (pto->fAddnode)
-                LogPrintf("Warning: not punishing addnoded peer %s!\n", pto->addr.ToString());
-            else {
-                pto->fDisconnect = true;
-                if (pto->addr.IsLocal())
-                    LogPrintf("Warning: not banning local peer %s!\n", pto->addr.ToString());
-                else
-                {
-                    connman.Ban(pto->addr, BanReasonNodeMisbehaving);
-                }
-                return true;
-            }
-        }
-
         // Address refresh broadcast
         int64_t nNow = GetTimeMicros();
         if (!IsInitialBlockDownload() && pto->nNextLocalAddrSend < nNow) {

From 8502e7acbe0f42fd6e6979681bc9c4610c4fb8cb Mon Sep 17 00:00:00 2001
From: Cory Fields <cory-nospam-@coryfields.com>
Date: Wed, 8 Feb 2017 01:02:49 -0500
Subject: [PATCH 2/6] 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.
---
 src/net_processing.cpp | 50 ++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 587e857970c48..b304da76c254a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1190,8 +1190,31 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
         }
     }
 
+    if (strCommand == NetMsgType::REJECT)
+    {
+        if (fDebug) {
+            try {
+                std::string strMsg; unsigned char ccode; std::string strReason;
+                vRecv >> LIMITED_STRING(strMsg, CMessageHeader::COMMAND_SIZE) >> ccode >> LIMITED_STRING(strReason, MAX_REJECT_MESSAGE_LENGTH);
+
+                std::ostringstream ss;
+                ss << strMsg << " code " << itostr(ccode) << ": " << strReason;
 
-    if (strCommand == NetMsgType::VERSION)
+                if (strMsg == NetMsgType::BLOCK || strMsg == NetMsgType::TX)
+                {
+                    uint256 hash;
+                    vRecv >> hash;
+                    ss << ": hash " << hash.ToString();
+                }
+                LogPrint("net", "Reject %s\n", SanitizeString(ss.str()));
+            } catch (const std::ios_base::failure&) {
+                // Avoid feedback loops by preventing reject messages from triggering a new reject message.
+                LogPrint("net", "Unparseable reject message received\n");
+            }
+        }
+    }
+
+    else if (strCommand == NetMsgType::VERSION)
     {
         // Each connection can only send one version message
         if (pfrom->nVersion != 0)
@@ -2544,31 +2567,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
         pfrom->fRelayTxes = true;
     }
 
-
-    else if (strCommand == NetMsgType::REJECT)
-    {
-        if (fDebug) {
-            try {
-                std::string strMsg; unsigned char ccode; std::string strReason;
-                vRecv >> LIMITED_STRING(strMsg, CMessageHeader::COMMAND_SIZE) >> ccode >> LIMITED_STRING(strReason, MAX_REJECT_MESSAGE_LENGTH);
-
-                std::ostringstream ss;
-                ss << strMsg << " code " << itostr(ccode) << ": " << strReason;
-
-                if (strMsg == NetMsgType::BLOCK || strMsg == NetMsgType::TX)
-                {
-                    uint256 hash;
-                    vRecv >> hash;
-                    ss << ": hash " << hash.ToString();
-                }
-                LogPrint("net", "Reject %s\n", SanitizeString(ss.str()));
-            } catch (const std::ios_base::failure&) {
-                // Avoid feedback loops by preventing reject messages from triggering a new reject message.
-                LogPrint("net", "Unparseable reject message received\n");
-            }
-        }
-    }
-
     else if (strCommand == NetMsgType::FEEFILTER) {
         CAmount newFeeFilter = 0;
         vRecv >> newFeeFilter;

From cbfc5a6728d389fbb15e0555cdf50f1b04595106 Mon Sep 17 00:00:00 2001
From: Cory Fields <cory-nospam-@coryfields.com>
Date: Wed, 8 Feb 2017 01:04:53 -0500
Subject: [PATCH 3/6] 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.
---
 src/net_processing.cpp | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index b304da76c254a..f458a352560cc 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1420,6 +1420,13 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
         pfrom->fSuccessfullyConnected = true;
     }
 
+    else if (!pfrom->fSuccessfullyConnected)
+    {
+        // Must have a verack message before anything else
+        LOCK(cs_main);
+        Misbehaving(pfrom->GetId(), 1);
+        return false;
+    }
 
     else if (strCommand == NetMsgType::ADDR)
     {

From 5b5e4f8330634dc33446854677badc52aef43b82 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Tue, 7 Feb 2017 17:35:57 -0500
Subject: [PATCH 4/6] qa: mininode learns when a socket connects, not its first
 action

---
 qa/rpc-tests/test_framework/mininode.py | 36 ++++++++++++++++---------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/qa/rpc-tests/test_framework/mininode.py b/qa/rpc-tests/test_framework/mininode.py
index 91daa4ab1f585..696a065282c62 100755
--- a/qa/rpc-tests/test_framework/mininode.py
+++ b/qa/rpc-tests/test_framework/mininode.py
@@ -1614,7 +1614,7 @@ class NodeConn(asyncore.dispatcher):
         "regtest": b"\xfa\xbf\xb5\xda",   # regtest
     }
 
-    def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK):
+    def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE_NETWORK, send_version=True):
         asyncore.dispatcher.__init__(self, map=mininode_socket_map)
         self.log = logging.getLogger("NodeConn(%s:%d)" % (dstaddr, dstport))
         self.dstaddr = dstaddr
@@ -1631,14 +1631,16 @@ def __init__(self, dstaddr, dstport, rpc, callback, net="regtest", services=NODE
         self.disconnect = False
         self.nServices = 0
 
-        # stuff version msg into sendbuf
-        vt = msg_version()
-        vt.nServices = services
-        vt.addrTo.ip = self.dstaddr
-        vt.addrTo.port = self.dstport
-        vt.addrFrom.ip = "0.0.0.0"
-        vt.addrFrom.port = 0
-        self.send_message(vt, True)
+        if send_version:
+            # stuff version msg into sendbuf
+            vt = msg_version()
+            vt.nServices = services
+            vt.addrTo.ip = self.dstaddr
+            vt.addrTo.port = self.dstport
+            vt.addrFrom.ip = "0.0.0.0"
+            vt.addrFrom.port = 0
+            self.send_message(vt, True)
+
         print('MiniNode: Connecting to Bitcoin Node IP # ' + dstaddr + ':' \
             + str(dstport))
 
@@ -1652,8 +1654,9 @@ def show_debug_msg(self, msg):
         self.log.debug(msg)
 
     def handle_connect(self):
-        self.show_debug_msg("MiniNode: Connected & Listening: \n")
-        self.state = "connected"
+        if self.state != "connected":
+            self.show_debug_msg("MiniNode: Connected & Listening: \n")
+            self.state = "connected"
 
     def handle_close(self):
         self.show_debug_msg("MiniNode: Closing Connection to %s:%d... "
@@ -1681,11 +1684,20 @@ def readable(self):
 
     def writable(self):
         with mininode_lock:
+            pre_connection = self.state == "connecting"
             length = len(self.sendbuf)
-        return (length > 0)
+        return (length > 0 or pre_connection)
 
     def handle_write(self):
         with mininode_lock:
+            # asyncore does not expose socket connection, only the first read/write
+            # event, thus we must check connection manually here to know when we
+            # actually connect
+            if self.state == "connecting":
+                self.handle_connect()
+            if not self.writable():
+                return
+
             try:
                 sent = self.send(self.sendbuf)
             except:

From 8650bbb660eaf8c81d714f1518ecc8c35ae17463 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Tue, 7 Feb 2017 17:40:28 -0500
Subject: [PATCH 5/6] qa: Expose on-connection to mininode listeners

---
 qa/rpc-tests/test_framework/mininode.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qa/rpc-tests/test_framework/mininode.py b/qa/rpc-tests/test_framework/mininode.py
index 696a065282c62..5b563c58ae1e1 100755
--- a/qa/rpc-tests/test_framework/mininode.py
+++ b/qa/rpc-tests/test_framework/mininode.py
@@ -1540,6 +1540,7 @@ def on_ping(self, conn, message):
         if conn.ver_send > BIP0031_VERSION:
             conn.send_message(msg_pong(message.nonce))
     def on_reject(self, conn, message): pass
+    def on_open(self, conn): pass
     def on_close(self, conn): pass
     def on_mempool(self, conn): pass
     def on_pong(self, conn, message): pass
@@ -1657,6 +1658,7 @@ def handle_connect(self):
         if self.state != "connected":
             self.show_debug_msg("MiniNode: Connected & Listening: \n")
             self.state = "connected"
+            self.cb.on_open(self)
 
     def handle_close(self):
         self.show_debug_msg("MiniNode: Closing Connection to %s:%d... "

From d9434918d277bba534933ebc8c63ba81e613f603 Mon Sep 17 00:00:00 2001
From: Cory Fields <cory-nospam-@coryfields.com>
Date: Wed, 8 Feb 2017 01:17:58 -0500
Subject: [PATCH 6/6] 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.
---
 qa/pull-tester/rpc-tests.py   |   1 +
 qa/rpc-tests/p2p-leaktests.py | 145 ++++++++++++++++++++++++++++++++++
 2 files changed, 146 insertions(+)
 create mode 100755 qa/rpc-tests/p2p-leaktests.py

diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py
index 26bc6a73dfa28..31018125416a2 100755
--- a/qa/pull-tester/rpc-tests.py
+++ b/qa/pull-tester/rpc-tests.py
@@ -154,6 +154,7 @@
     'bumpfee.py',
     'rpcnamedargs.py',
     'listsinceblock.py',
+    'p2p-leaktests.py',
 ]
 if ENABLE_ZMQ:
     testScripts.append('zmq_test.py')
diff --git a/qa/rpc-tests/p2p-leaktests.py b/qa/rpc-tests/p2p-leaktests.py
new file mode 100755
index 0000000000000..41ca84d779890
--- /dev/null
+++ b/qa/rpc-tests/p2p-leaktests.py
@@ -0,0 +1,145 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+from test_framework.mininode import *
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+
+'''
+Test for message sending before handshake completion
+
+A node should never send anything other than VERSION/VERACK/REJECT until it's
+received a VERACK.
+
+This test connects to a node and sends it a few messages, trying to intice it
+into sending us something it shouldn't.
+'''
+
+banscore = 10
+
+class CLazyNode(NodeConnCB):
+    def __init__(self):
+        self.connection = None
+        self.unexpected_msg = False
+        self.connected = False
+        super().__init__()
+
+    def add_connection(self, conn):
+        self.connection = conn
+
+    def send_message(self, message):
+        self.connection.send_message(message)
+
+    def bad_message(self, message):
+        self.unexpected_msg = True
+        print("should not have received message: %s" % message.command)
+
+    def on_open(self, conn):
+        self.connected = True
+
+    def on_version(self, conn, message): self.bad_message(message)
+    def on_verack(self, conn, message): self.bad_message(message)
+    def on_reject(self, conn, message): self.bad_message(message)
+    def on_inv(self, conn, message): self.bad_message(message)
+    def on_addr(self, conn, message): self.bad_message(message)
+    def on_alert(self, conn, message): self.bad_message(message)
+    def on_getdata(self, conn, message): self.bad_message(message)
+    def on_getblocks(self, conn, message): self.bad_message(message)
+    def on_tx(self, conn, message): self.bad_message(message)
+    def on_block(self, conn, message): self.bad_message(message)
+    def on_getaddr(self, conn, message): self.bad_message(message)
+    def on_headers(self, conn, message): self.bad_message(message)
+    def on_getheaders(self, conn, message): self.bad_message(message)
+    def on_ping(self, conn, message): self.bad_message(message)
+    def on_mempool(self, conn): self.bad_message(message)
+    def on_pong(self, conn, message): self.bad_message(message)
+    def on_feefilter(self, conn, message): self.bad_message(message)
+    def on_sendheaders(self, conn, message): self.bad_message(message)
+    def on_sendcmpct(self, conn, message): self.bad_message(message)
+    def on_cmpctblock(self, conn, message): self.bad_message(message)
+    def on_getblocktxn(self, conn, message): self.bad_message(message)
+    def on_blocktxn(self, conn, message): self.bad_message(message)
+
+# Node that never sends a version. We'll use this to send a bunch of messages
+# anyway, and eventually get disconnected.
+class CNodeNoVersionBan(CLazyNode):
+    def __init__(self):
+        super().__init__()
+
+    # send a bunch of veracks without sending a message. This should get us disconnected.
+    # NOTE: implementation-specific check here. Remove if bitcoind ban behavior changes
+    def on_open(self, conn):
+        super().on_open(conn)
+        for i in range(banscore):
+            self.send_message(msg_verack())
+
+    def on_reject(self, conn, message): pass
+
+# Node that never sends a version. This one just sits idle and hopes to receive
+# any message (it shouldn't!)
+class CNodeNoVersionIdle(CLazyNode):
+    def __init__(self):
+        super().__init__()
+
+# Node that sends a version but not a verack.
+class CNodeNoVerackIdle(CLazyNode):
+    def __init__(self):
+        self.version_received = False
+        super().__init__()
+
+    def on_reject(self, conn, message): pass
+    def on_verack(self, conn, message): pass
+    # When version is received, don't reply with a verack. Instead, see if the
+    # node will give us a message that it shouldn't. This is not an exhaustive
+    # list!
+    def on_version(self, conn, message):
+        self.version_received = True
+        conn.send_message(msg_ping())
+        conn.send_message(msg_getaddr())
+
+class P2PLeakTest(BitcoinTestFramework):
+    def __init__(self):
+        super().__init__()
+        self.num_nodes = 1
+    def setup_network(self):
+        extra_args = [['-debug', '-banscore='+str(banscore)]
+                      for i in range(self.num_nodes)]
+        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, extra_args)
+
+    def run_test(self):
+        no_version_bannode = CNodeNoVersionBan()
+        no_version_idlenode = CNodeNoVersionIdle()
+        no_verack_idlenode = CNodeNoVerackIdle()
+
+        connections = []
+        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], no_version_bannode, send_version=False))
+        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], no_version_idlenode, send_version=False))
+        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], no_verack_idlenode))
+        no_version_bannode.add_connection(connections[0])
+        no_version_idlenode.add_connection(connections[1])
+        no_verack_idlenode.add_connection(connections[2])
+
+        NetworkThread().start()  # Start up network handling in another thread
+
+        assert(wait_until(lambda: no_version_bannode.connected and no_version_idlenode.connected and no_verack_idlenode.version_received, timeout=10))
+
+        # Mine a block and make sure that it's not sent to the connected nodes
+        self.nodes[0].generate(1)
+
+        #Give the node enough time to possibly leak out a message
+        time.sleep(5)
+
+        #This node should have been banned
+        assert(no_version_bannode.connection.state == "closed")
+
+        [conn.disconnect_node() for conn in connections]
+
+        # Make sure no unexpected messages came in
+        assert(no_version_bannode.unexpected_msg == False)
+        assert(no_version_idlenode.unexpected_msg == False)
+        assert(no_verack_idlenode.unexpected_msg == False)
+
+if __name__ == '__main__':
+    P2PLeakTest().main()