From f60b9059e4958245bda82e9656c52a31d5268ad9 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 24 May 2016 16:42:17 -0400 Subject: [PATCH] net: Pass best block known height into CConnman CConnman then passes the current best height into CNode at creation time. This way CConnman/CNode have no dependency on main for height, and the signals only move in one direction. This also helps to prevent identity leakage a tiny bit. Before this change, an attacker could theoretically make 2 connections on different interfaces. They would connect fully on one, and only establish the initial connection on the other. Once they receive a new block, they would relay it to your first connection, and immediately commence the version handshake on the second. Since the new block height is reflected immediately, they could attempt to learn whether the two connections were correlated. This is, of course, incredibly unlikely to work due to the small timings involved and receipt from other senders. But it doesn't hurt to lock-in nBestHeight at the time of connection, rather than letting the remote choose the time. --- src/init.cpp | 2 +- src/main.cpp | 10 ++-------- src/net.cpp | 36 ++++++++++++++++++++++++------------ src/net.h | 13 +++++++++---- src/test/DoS_tests.cpp | 8 ++++---- src/test/net_tests.cpp | 5 +++-- 6 files changed, 43 insertions(+), 31 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 599dac39b..4dce8be81 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1510,7 +1510,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) std::string strNodeError; int nMaxOutbound = std::min(MAX_OUTBOUND_CONNECTIONS, nMaxConnections); - if(!StartNode(connman, threadGroup, scheduler, nLocalServices, nRelevantServices, nMaxConnections, nMaxOutbound, strNodeError)) + if(!StartNode(connman, threadGroup, scheduler, nLocalServices, nRelevantServices, nMaxConnections, nMaxOutbound, chainActive.Height(), strNodeError)) return InitError(strNodeError); // ********************************************************* Step 12: finished diff --git a/src/main.cpp b/src/main.cpp index a916cbc12..1835d9712 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -325,12 +325,6 @@ CNodeState *State(NodeId pnode) { return &it->second; } -int GetHeight() -{ - LOCK(cs_main); - return chainActive.Height(); -} - void UpdatePreferredDownload(CNode* node, CNodeState* state) { nPreferredDownload -= state->fPreferredDownload; @@ -639,7 +633,6 @@ bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats) { void RegisterNodeSignals(CNodeSignals& nodeSignals) { - nodeSignals.GetHeight.connect(&GetHeight); nodeSignals.ProcessMessages.connect(&ProcessMessages); nodeSignals.SendMessages.connect(&SendMessages); nodeSignals.InitializeNode.connect(&InitializeNode); @@ -648,7 +641,6 @@ void RegisterNodeSignals(CNodeSignals& nodeSignals) void UnregisterNodeSignals(CNodeSignals& nodeSignals) { - nodeSignals.GetHeight.disconnect(&GetHeight); nodeSignals.ProcessMessages.disconnect(&ProcessMessages); nodeSignals.SendMessages.disconnect(&SendMessages); nodeSignals.InitializeNode.disconnect(&InitializeNode); @@ -3058,6 +3050,8 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, // When we reach this point, we switched to a new tip (stored in pindexNewTip). // Notifications/callbacks that can run without cs_main + if(connman) + connman->SetBestHeight(nNewHeight); // throw all transactions though the signal-interface // while _not_ holding the cs_main lock diff --git a/src/net.cpp b/src/net.cpp index fe4daaaeb..d51095255 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -392,7 +392,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo addrman.Attempt(addrConnect, fCountFailure); // Add node - CNode* pnode = new CNode(GetNewNodeId(), nLocalServices, hSocket, addrConnect, pszDest ? pszDest : "", false); + CNode* pnode = new CNode(GetNewNodeId(), nLocalServices, GetBestHeight(), hSocket, addrConnect, pszDest ? pszDest : "", false); GetNodeSignals().InitializeNode(pnode->GetId(), pnode); pnode->AddRef(); @@ -451,17 +451,15 @@ void CNode::CloseSocketDisconnect() void CNode::PushVersion() { - int nBestHeight = GetNodeSignals().GetHeight().get_value_or(0); - int64_t nTime = (fInbound ? GetAdjustedTime() : GetTime()); CAddress addrYou = (addr.IsRoutable() && !IsProxy(addr) ? addr : CAddress(CService(), addr.nServices)); CAddress addrMe = GetLocalAddress(&addr, nLocalServices); if (fLogIPs) - LogPrint("net", "send version message: version %d, blocks=%d, us=%s, them=%s, peer=%d\n", PROTOCOL_VERSION, nBestHeight, addrMe.ToString(), addrYou.ToString(), id); + LogPrint("net", "send version message: version %d, blocks=%d, us=%s, them=%s, peer=%d\n", PROTOCOL_VERSION, nMyStartingHeight, addrMe.ToString(), addrYou.ToString(), id); else - LogPrint("net", "send version message: version %d, blocks=%d, us=%s, peer=%d\n", PROTOCOL_VERSION, nBestHeight, addrMe.ToString(), id); + LogPrint("net", "send version message: version %d, blocks=%d, us=%s, peer=%d\n", PROTOCOL_VERSION, nMyStartingHeight, addrMe.ToString(), id); PushMessage(NetMsgType::VERSION, PROTOCOL_VERSION, (uint64_t)nLocalServices, nTime, addrYou, addrMe, - nLocalHostNonce, strSubVersion, nBestHeight, ::fRelayTxes); + nLocalHostNonce, strSubVersion, nMyStartingHeight, ::fRelayTxes); } @@ -1028,7 +1026,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { } } - CNode* pnode = new CNode(GetNewNodeId(), nLocalServices, hSocket, addr, "", true); + CNode* pnode = new CNode(GetNewNodeId(), nLocalServices, GetBestHeight(), hSocket, addr, "", true); GetNodeSignals().InitializeNode(pnode->GetId(), pnode); pnode->AddRef(); pnode->fWhitelisted = whitelisted; @@ -2038,13 +2036,14 @@ CConnman::CConnman() semOutbound = NULL; nMaxConnections = 0; nMaxOutbound = 0; + nBestHeight = 0; } -bool StartNode(CConnman& connman, boost::thread_group& threadGroup, CScheduler& scheduler, ServiceFlags nLocalServices, ServiceFlags nRelevantServices, int nMaxConnectionsIn, int nMaxOutboundIn, std::string& strNodeError) +bool StartNode(CConnman& connman, boost::thread_group& threadGroup, CScheduler& scheduler, ServiceFlags nLocalServices, ServiceFlags nRelevantServices, int nMaxConnectionsIn, int nMaxOutboundIn, int nBestHeightIn, std::string& strNodeError) { Discover(threadGroup); - bool ret = connman.Start(threadGroup, scheduler, nLocalServices, nRelevantServices, nMaxConnectionsIn, nMaxOutboundIn, strNodeError); + bool ret = connman.Start(threadGroup, scheduler, nLocalServices, nRelevantServices, nMaxConnectionsIn, nMaxOutboundIn, nBestHeightIn, strNodeError); return ret; } @@ -2054,7 +2053,7 @@ NodeId CConnman::GetNewNodeId() return nLastNodeId.fetch_add(1, std::memory_order_relaxed); } -bool CConnman::Start(boost::thread_group& threadGroup, CScheduler& scheduler, ServiceFlags nLocalServicesIn, ServiceFlags nRelevantServicesIn, int nMaxConnectionsIn, int nMaxOutboundIn, std::string& strNodeError) +bool CConnman::Start(boost::thread_group& threadGroup, CScheduler& scheduler, ServiceFlags nLocalServicesIn, ServiceFlags nRelevantServicesIn, int nMaxConnectionsIn, int nMaxOutboundIn, int nBestHeightIn, std::string& strNodeError) { nTotalBytesRecv = 0; nTotalBytesSent = 0; @@ -2071,6 +2070,8 @@ bool CConnman::Start(boost::thread_group& threadGroup, CScheduler& scheduler, Se nSendBufferMaxSize = 1000*GetArg("-maxsendbuffer", DEFAULT_MAXSENDBUFFER); nReceiveFloodSize = 1000*GetArg("-maxreceivebuffer", DEFAULT_MAXRECEIVEBUFFER); + SetBestHeight(nBestHeightIn); + uiInterface.InitMessage(_("Loading addresses...")); // Load addresses from peers.dat int64_t nStart = GetTimeMillis(); @@ -2115,7 +2116,7 @@ bool CConnman::Start(boost::thread_group& threadGroup, CScheduler& scheduler, Se if (pnodeLocalHost == NULL) { CNetAddr local; LookupHost("127.0.0.1", local, false); - pnodeLocalHost = new CNode(GetNewNodeId(), nLocalServices, INVALID_SOCKET, CAddress(CService(local, 0), nLocalServices)); + pnodeLocalHost = new CNode(GetNewNodeId(), nLocalServices, GetBestHeight(), INVALID_SOCKET, CAddress(CService(local, 0), nLocalServices)); GetNodeSignals().InitializeNode(pnodeLocalHost->GetId(), pnodeLocalHost); } @@ -2471,6 +2472,16 @@ ServiceFlags CConnman::GetLocalServices() const return nLocalServices; } +void CConnman::SetBestHeight(int height) +{ + nBestHeight.store(height, std::memory_order_release); +} + +int CConnman::GetBestHeight() const +{ + return nBestHeight.load(std::memory_order_acquire); +} + void CNode::Fuzz(int nChance) { if (!fSuccessfullyConnected) return; // Don't fuzz initial handshake @@ -2509,7 +2520,7 @@ void CNode::Fuzz(int nChance) unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } unsigned int CConnman::GetSendBufferSize() const{ return nSendBufferMaxSize; } -CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress& addrIn, const std::string& addrNameIn, bool fInboundIn) : +CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress& addrIn, const std::string& addrNameIn, bool fInboundIn) : ssSend(SER_NETWORK, INIT_PROTO_VERSION), addr(addrIn), nKeyedNetGroup(CalculateKeyedNetGroup(addrIn)), @@ -2567,6 +2578,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const nLocalServices = nLocalServicesIn; GetRandBytes((unsigned char*)&nLocalHostNonce, sizeof(nLocalHostNonce)); + nMyStartingHeight = nMyStartingHeightIn; BOOST_FOREACH(const std::string &msg, getAllNetMessageTypes()) mapRecvBytesPerMsgCmd[msg] = 0; diff --git a/src/net.h b/src/net.h index 0859190d3..852822d85 100644 --- a/src/net.h +++ b/src/net.h @@ -109,7 +109,7 @@ public: CConnman(); ~CConnman(); - bool Start(boost::thread_group& threadGroup, CScheduler& scheduler, ServiceFlags nLocalServicesIn, ServiceFlags nRelevantServicesIn, int nMaxConnectionsIn, int nMaxOutboundIn, std::string& strNodeError); + bool Start(boost::thread_group& threadGroup, CScheduler& scheduler, ServiceFlags nLocalServicesIn, ServiceFlags nRelevantServicesIn, int nMaxConnectionsIn, int nMaxOutboundIn, int nBestHeightIn, std::string& strNodeError); void Stop(); bool BindListenPort(const CService &bindAddr, std::string& strError, bool fWhitelisted = false); bool OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = NULL, const char *strDest = NULL, bool fOneShot = false, bool fFeeler = false); @@ -199,6 +199,10 @@ public: uint64_t GetTotalBytesRecv(); uint64_t GetTotalBytesSent(); + void SetBestHeight(int height); + int GetBestHeight() const; + + private: struct ListenSocket { SOCKET socket; @@ -288,12 +292,13 @@ private: CSemaphore *semOutbound; int nMaxConnections; int nMaxOutbound; + std::atomic nBestHeight; }; extern std::unique_ptr g_connman; void MapPort(bool fUseUPnP); unsigned short GetListenPort(); bool BindListenPort(const CService &bindAddr, std::string& strError, bool fWhitelisted = false); -bool StartNode(CConnman& connman, boost::thread_group& threadGroup, CScheduler& scheduler, ServiceFlags nLocalServices, ServiceFlags nRelevantServices, int nMaxConnections, int nMaxOutbound, std::string& strNodeError); +bool StartNode(CConnman& connman, boost::thread_group& threadGroup, CScheduler& scheduler, ServiceFlags nLocalServices, ServiceFlags nRelevantServices, int nMaxConnections, int nMaxOutbound, int nBestHeightIn, std::string& strNodeError); bool StopNode(CConnman& connman); size_t SocketSendData(CNode *pnode); @@ -315,7 +320,6 @@ struct CombinerAll // Signals for message handling struct CNodeSignals { - boost::signals2::signal GetHeight; boost::signals2::signal ProcessMessages; boost::signals2::signal SendMessages; boost::signals2::signal InitializeNode; @@ -558,7 +562,7 @@ public: CAmount lastSentFeeFilter; int64_t nextSendTimeFeeFilter; - CNode(NodeId id, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress &addrIn, const std::string &addrNameIn = "", bool fInboundIn = false); + CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, const std::string &addrNameIn = "", bool fInboundIn = false); ~CNode(); private: @@ -569,6 +573,7 @@ private: uint64_t nLocalHostNonce; ServiceFlags nLocalServices; + int nMyStartingHeight; public: NodeId GetId() const { diff --git a/src/test/DoS_tests.cpp b/src/test/DoS_tests.cpp index 2265f43a6..33f107d84 100644 --- a/src/test/DoS_tests.cpp +++ b/src/test/DoS_tests.cpp @@ -48,7 +48,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning) { connman->ClearBanned(); CAddress addr1(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode1(id++, NODE_NETWORK, INVALID_SOCKET, addr1, "", true); + CNode dummyNode1(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr1, "", true); GetNodeSignals().InitializeNode(dummyNode1.GetId(), &dummyNode1); dummyNode1.nVersion = 1; Misbehaving(dummyNode1.GetId(), 100); // Should get banned @@ -57,7 +57,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning) BOOST_CHECK(!connman->IsBanned(ip(0xa0b0c001|0x0000ff00))); // Different IP, not banned CAddress addr2(ip(0xa0b0c002), NODE_NONE); - CNode dummyNode2(id++, NODE_NETWORK, INVALID_SOCKET, addr2, "", true); + CNode dummyNode2(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr2, "", true); GetNodeSignals().InitializeNode(dummyNode2.GetId(), &dummyNode2); dummyNode2.nVersion = 1; Misbehaving(dummyNode2.GetId(), 50); @@ -74,7 +74,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) connman->ClearBanned(); mapArgs["-banscore"] = "111"; // because 11 is my favorite number CAddress addr1(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode1(id++, NODE_NETWORK, INVALID_SOCKET, addr1, "", true); + CNode dummyNode1(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr1, "", true); GetNodeSignals().InitializeNode(dummyNode1.GetId(), &dummyNode1); dummyNode1.nVersion = 1; Misbehaving(dummyNode1.GetId(), 100); @@ -96,7 +96,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) SetMockTime(nStartTime); // Overrides future calls to GetTime() CAddress addr(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode(id++, NODE_NETWORK, INVALID_SOCKET, addr, "", true); + CNode dummyNode(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr, "", true); GetNodeSignals().InitializeNode(dummyNode.GetId(), &dummyNode); dummyNode.nVersion = 1; diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 1019b12c1..bc9a98ab0 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -154,6 +154,7 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) { SOCKET hSocket = INVALID_SOCKET; NodeId id = 0; + int height = 0; in_addr ipv4Addr; ipv4Addr.s_addr = 0xa0b0c001; @@ -163,12 +164,12 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) bool fInboundIn = false; // Test that fFeeler is false by default. - CNode* pnode1 = new CNode(id++, NODE_NETWORK, hSocket, addr, pszDest, fInboundIn); + CNode* pnode1 = new CNode(id++, NODE_NETWORK, height, hSocket, addr, pszDest, fInboundIn); BOOST_CHECK(pnode1->fInbound == false); BOOST_CHECK(pnode1->fFeeler == false); fInboundIn = true; - CNode* pnode2 = new CNode(id++, NODE_NETWORK, hSocket, addr, pszDest, fInboundIn); + CNode* pnode2 = new CNode(id++, NODE_NETWORK, height, hSocket, addr, pszDest, fInboundIn); BOOST_CHECK(pnode2->fInbound == true); BOOST_CHECK(pnode2->fFeeler == false); }