Skip to content

Commit e87ce95

Browse files
committed
Merge #9720: net: fix banning and disallow sending messages before receiving verack
d943491 qa: add a test to detect leaky p2p messages (Cory Fields) 8650bbb qa: Expose on-connection to mininode listeners (Matt Corallo) 5b5e4f8 qa: mininode learns when a socket connects, not its first action (Matt Corallo) cbfc5a6 net: require a verack before responding to anything else (Cory Fields) 8502e7a net: parse reject earlier (Cory Fields) c45b9fb net: correctly ban before the handshake is complete (Cory Fields)
2 parents b08656e + d943491 commit e87ce95

File tree

3 files changed

+214
-49
lines changed

3 files changed

+214
-49
lines changed

qa/pull-tester/rpc-tests.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@
154154
'bumpfee.py',
155155
'rpcnamedargs.py',
156156
'listsinceblock.py',
157+
'p2p-leaktests.py',
157158
]
158159
if ENABLE_ZMQ:
159160
testScripts.append('zmq_test.py')

qa/rpc-tests/p2p-leaktests.py

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
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+
10+
'''
11+
Test for message sending before handshake completion
12+
13+
A node should never send anything other than VERSION/VERACK/REJECT until it's
14+
received a VERACK.
15+
16+
This test connects to a node and sends it a few messages, trying to intice it
17+
into sending us something it shouldn't.
18+
'''
19+
20+
banscore = 10
21+
22+
class CLazyNode(NodeConnCB):
23+
def __init__(self):
24+
self.connection = None
25+
self.unexpected_msg = False
26+
self.connected = False
27+
super().__init__()
28+
29+
def add_connection(self, conn):
30+
self.connection = conn
31+
32+
def send_message(self, message):
33+
self.connection.send_message(message)
34+
35+
def bad_message(self, message):
36+
self.unexpected_msg = True
37+
print("should not have received message: %s" % message.command)
38+
39+
def on_open(self, conn):
40+
self.connected = True
41+
42+
def on_version(self, conn, message): self.bad_message(message)
43+
def on_verack(self, conn, message): self.bad_message(message)
44+
def on_reject(self, conn, message): self.bad_message(message)
45+
def on_inv(self, conn, message): self.bad_message(message)
46+
def on_addr(self, conn, message): self.bad_message(message)
47+
def on_alert(self, conn, message): self.bad_message(message)
48+
def on_getdata(self, conn, message): self.bad_message(message)
49+
def on_getblocks(self, conn, message): self.bad_message(message)
50+
def on_tx(self, conn, message): self.bad_message(message)
51+
def on_block(self, conn, message): self.bad_message(message)
52+
def on_getaddr(self, conn, message): self.bad_message(message)
53+
def on_headers(self, conn, message): self.bad_message(message)
54+
def on_getheaders(self, conn, message): self.bad_message(message)
55+
def on_ping(self, conn, message): self.bad_message(message)
56+
def on_mempool(self, conn): self.bad_message(message)
57+
def on_pong(self, conn, message): self.bad_message(message)
58+
def on_feefilter(self, conn, message): self.bad_message(message)
59+
def on_sendheaders(self, conn, message): self.bad_message(message)
60+
def on_sendcmpct(self, conn, message): self.bad_message(message)
61+
def on_cmpctblock(self, conn, message): self.bad_message(message)
62+
def on_getblocktxn(self, conn, message): self.bad_message(message)
63+
def on_blocktxn(self, conn, message): self.bad_message(message)
64+
65+
# Node that never sends a version. We'll use this to send a bunch of messages
66+
# anyway, and eventually get disconnected.
67+
class CNodeNoVersionBan(CLazyNode):
68+
def __init__(self):
69+
super().__init__()
70+
71+
# send a bunch of veracks without sending a message. This should get us disconnected.
72+
# NOTE: implementation-specific check here. Remove if bitcoind ban behavior changes
73+
def on_open(self, conn):
74+
super().on_open(conn)
75+
for i in range(banscore):
76+
self.send_message(msg_verack())
77+
78+
def on_reject(self, conn, message): pass
79+
80+
# Node that never sends a version. This one just sits idle and hopes to receive
81+
# any message (it shouldn't!)
82+
class CNodeNoVersionIdle(CLazyNode):
83+
def __init__(self):
84+
super().__init__()
85+
86+
# Node that sends a version but not a verack.
87+
class CNodeNoVerackIdle(CLazyNode):
88+
def __init__(self):
89+
self.version_received = False
90+
super().__init__()
91+
92+
def on_reject(self, conn, message): pass
93+
def on_verack(self, conn, message): pass
94+
# When version is received, don't reply with a verack. Instead, see if the
95+
# node will give us a message that it shouldn't. This is not an exhaustive
96+
# list!
97+
def on_version(self, conn, message):
98+
self.version_received = True
99+
conn.send_message(msg_ping())
100+
conn.send_message(msg_getaddr())
101+
102+
class P2PLeakTest(BitcoinTestFramework):
103+
def __init__(self):
104+
super().__init__()
105+
self.num_nodes = 1
106+
def setup_network(self):
107+
extra_args = [['-debug', '-banscore='+str(banscore)]
108+
for i in range(self.num_nodes)]
109+
self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, extra_args)
110+
111+
def run_test(self):
112+
no_version_bannode = CNodeNoVersionBan()
113+
no_version_idlenode = CNodeNoVersionIdle()
114+
no_verack_idlenode = CNodeNoVerackIdle()
115+
116+
connections = []
117+
connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], no_version_bannode, send_version=False))
118+
connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], no_version_idlenode, send_version=False))
119+
connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], no_verack_idlenode))
120+
no_version_bannode.add_connection(connections[0])
121+
no_version_idlenode.add_connection(connections[1])
122+
no_verack_idlenode.add_connection(connections[2])
123+
124+
NetworkThread().start() # Start up network handling in another thread
125+
126+
assert(wait_until(lambda: no_version_bannode.connected and no_version_idlenode.connected and no_verack_idlenode.version_received, timeout=10))
127+
128+
# Mine a block and make sure that it's not sent to the connected nodes
129+
self.nodes[0].generate(1)
130+
131+
#Give the node enough time to possibly leak out a message
132+
time.sleep(5)
133+
134+
#This node should have been banned
135+
assert(no_version_bannode.connection.state == "closed")
136+
137+
[conn.disconnect_node() for conn in connections]
138+
139+
# Make sure no unexpected messages came in
140+
assert(no_version_bannode.unexpected_msg == False)
141+
assert(no_version_idlenode.unexpected_msg == False)
142+
assert(no_verack_idlenode.unexpected_msg == False)
143+
144+
if __name__ == '__main__':
145+
P2PLeakTest().main()

src/net_processing.cpp

Lines changed: 68 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1190,8 +1190,31 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
11901190
}
11911191
}
11921192

1193+
if (strCommand == NetMsgType::REJECT)
1194+
{
1195+
if (fDebug) {
1196+
try {
1197+
std::string strMsg; unsigned char ccode; std::string strReason;
1198+
vRecv >> LIMITED_STRING(strMsg, CMessageHeader::COMMAND_SIZE) >> ccode >> LIMITED_STRING(strReason, MAX_REJECT_MESSAGE_LENGTH);
11931199

1194-
if (strCommand == NetMsgType::VERSION)
1200+
std::ostringstream ss;
1201+
ss << strMsg << " code " << itostr(ccode) << ": " << strReason;
1202+
1203+
if (strMsg == NetMsgType::BLOCK || strMsg == NetMsgType::TX)
1204+
{
1205+
uint256 hash;
1206+
vRecv >> hash;
1207+
ss << ": hash " << hash.ToString();
1208+
}
1209+
LogPrint("net", "Reject %s\n", SanitizeString(ss.str()));
1210+
} catch (const std::ios_base::failure&) {
1211+
// Avoid feedback loops by preventing reject messages from triggering a new reject message.
1212+
LogPrint("net", "Unparseable reject message received\n");
1213+
}
1214+
}
1215+
}
1216+
1217+
else if (strCommand == NetMsgType::VERSION)
11951218
{
11961219
// Each connection can only send one version message
11971220
if (pfrom->nVersion != 0)
@@ -1402,6 +1425,13 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
14021425
pfrom->fSuccessfullyConnected = true;
14031426
}
14041427

1428+
else if (!pfrom->fSuccessfullyConnected)
1429+
{
1430+
// Must have a verack message before anything else
1431+
LOCK(cs_main);
1432+
Misbehaving(pfrom->GetId(), 1);
1433+
return false;
1434+
}
14051435

14061436
else if (strCommand == NetMsgType::ADDR)
14071437
{
@@ -2549,31 +2579,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
25492579
pfrom->fRelayTxes = true;
25502580
}
25512581

2552-
2553-
else if (strCommand == NetMsgType::REJECT)
2554-
{
2555-
if (fDebug) {
2556-
try {
2557-
std::string strMsg; unsigned char ccode; std::string strReason;
2558-
vRecv >> LIMITED_STRING(strMsg, CMessageHeader::COMMAND_SIZE) >> ccode >> LIMITED_STRING(strReason, MAX_REJECT_MESSAGE_LENGTH);
2559-
2560-
std::ostringstream ss;
2561-
ss << strMsg << " code " << itostr(ccode) << ": " << strReason;
2562-
2563-
if (strMsg == NetMsgType::BLOCK || strMsg == NetMsgType::TX)
2564-
{
2565-
uint256 hash;
2566-
vRecv >> hash;
2567-
ss << ": hash " << hash.ToString();
2568-
}
2569-
LogPrint("net", "Reject %s\n", SanitizeString(ss.str()));
2570-
} catch (const std::ios_base::failure&) {
2571-
// Avoid feedback loops by preventing reject messages from triggering a new reject message.
2572-
LogPrint("net", "Unparseable reject message received\n");
2573-
}
2574-
}
2575-
}
2576-
25772582
else if (strCommand == NetMsgType::FEEFILTER) {
25782583
CAmount newFeeFilter = 0;
25792584
vRecv >> newFeeFilter;
@@ -2601,6 +2606,36 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
26012606
return true;
26022607
}
26032608

2609+
static bool SendRejectsAndCheckIfBanned(CNode* pnode, CConnman& connman)
2610+
{
2611+
AssertLockHeld(cs_main);
2612+
CNodeState &state = *State(pnode->GetId());
2613+
2614+
BOOST_FOREACH(const CBlockReject& reject, state.rejects) {
2615+
connman.PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, (std::string)NetMsgType::BLOCK, reject.chRejectCode, reject.strRejectReason, reject.hashBlock));
2616+
}
2617+
state.rejects.clear();
2618+
2619+
if (state.fShouldBan) {
2620+
state.fShouldBan = false;
2621+
if (pnode->fWhitelisted)
2622+
LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode->addr.ToString());
2623+
else if (pnode->fAddnode)
2624+
LogPrintf("Warning: not punishing addnoded peer %s!\n", pnode->addr.ToString());
2625+
else {
2626+
pnode->fDisconnect = true;
2627+
if (pnode->addr.IsLocal())
2628+
LogPrintf("Warning: not banning local peer %s!\n", pnode->addr.ToString());
2629+
else
2630+
{
2631+
connman.Ban(pnode->addr, BanReasonNodeMisbehaving);
2632+
}
2633+
}
2634+
return true;
2635+
}
2636+
return false;
2637+
}
2638+
26042639
bool ProcessMessages(CNode* pfrom, CConnman& connman, const std::atomic<bool>& interruptMsgProc)
26052640
{
26062641
const CChainParams& chainparams = Params();
@@ -2711,8 +2746,12 @@ bool ProcessMessages(CNode* pfrom, CConnman& connman, const std::atomic<bool>& i
27112746
PrintExceptionContinue(NULL, "ProcessMessages()");
27122747
}
27132748

2714-
if (!fRet)
2749+
if (!fRet) {
27152750
LogPrintf("%s(%s, %u bytes) FAILED peer=%d\n", __func__, SanitizeString(strCommand), nMessageSize, pfrom->id);
2751+
}
2752+
2753+
LOCK(cs_main);
2754+
SendRejectsAndCheckIfBanned(pfrom, connman);
27162755

27172756
return fMoreWork;
27182757
}
@@ -2778,30 +2817,10 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& interr
27782817
if (!lockMain)
27792818
return true;
27802819

2820+
if (SendRejectsAndCheckIfBanned(pto, connman))
2821+
return true;
27812822
CNodeState &state = *State(pto->GetId());
27822823

2783-
BOOST_FOREACH(const CBlockReject& reject, state.rejects)
2784-
connman.PushMessage(pto, msgMaker.Make(NetMsgType::REJECT, (std::string)NetMsgType::BLOCK, reject.chRejectCode, reject.strRejectReason, reject.hashBlock));
2785-
state.rejects.clear();
2786-
2787-
if (state.fShouldBan) {
2788-
state.fShouldBan = false;
2789-
if (pto->fWhitelisted)
2790-
LogPrintf("Warning: not punishing whitelisted peer %s!\n", pto->addr.ToString());
2791-
else if (pto->fAddnode)
2792-
LogPrintf("Warning: not punishing addnoded peer %s!\n", pto->addr.ToString());
2793-
else {
2794-
pto->fDisconnect = true;
2795-
if (pto->addr.IsLocal())
2796-
LogPrintf("Warning: not banning local peer %s!\n", pto->addr.ToString());
2797-
else
2798-
{
2799-
connman.Ban(pto->addr, BanReasonNodeMisbehaving);
2800-
}
2801-
return true;
2802-
}
2803-
}
2804-
28052824
// Address refresh broadcast
28062825
int64_t nNow = GetTimeMicros();
28072826
if (!IsInitialBlockDownload() && pto->nNextLocalAddrSend < nNow) {

0 commit comments

Comments
 (0)