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)
This commit is contained in:
Wladimir J. van der Laan
Alexander Block
parent
588b8e5caf
commit
fe0ef87cd3
@ -162,6 +162,7 @@ testScripts = [
|
||||
'import-rescan.py',
|
||||
'rpcnamedargs.py',
|
||||
'listsinceblock.py',
|
||||
'p2p-leaktests.py',
|
||||
]
|
||||
if ENABLE_ZMQ:
|
||||
testScripts.append('zmq_test.py')
|
||||
|
||||
145
qa/rpc-tests/p2p-leaktests.py
Executable file
145
qa/rpc-tests/p2p-leaktests.py
Executable file
@ -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()
|
||||
@ -1145,8 +1145,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);
|
||||
|
||||
if (strCommand == NetMsgType::VERSION)
|
||||
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::VERSION)
|
||||
{
|
||||
// Each connection can only send one version message
|
||||
if (pfrom->nVersion != 0)
|
||||
@ -1351,6 +1374,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)
|
||||
{
|
||||
@ -2235,31 +2265,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;
|
||||
@ -2311,6 +2316,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();
|
||||
@ -2421,8 +2456,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;
|
||||
}
|
||||
@ -2489,30 +2528,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) {
|
||||
|
||||
Loading…
Reference in New Issue
Block a user