From 2716647ebf60cea05fc9edce6a18dcce4e7727ad Mon Sep 17 00:00:00 2001 From: Troy Giorshev Date: Mon, 29 Jun 2020 14:09:42 -0400 Subject: Give V1TransportDeserializer an m_node_id member This is intended to only be used for logging. This will allow log messages in the following commits to keep recording the peer's ID, even when logging is moved into V1TransportDeserializer. --- src/net.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index e7d3a146ff..73029655ce 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -699,10 +699,11 @@ CNetMessage V1TransportDeserializer::GetMessage(const CMessageHeader::MessageSta msg.m_valid_checksum = (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0); if (!msg.m_valid_checksum) { - LogPrint(BCLog::NET, "CHECKSUM ERROR (%s, %u bytes), expected %s was %s\n", + LogPrint(BCLog::NET, "CHECKSUM ERROR (%s, %u bytes), expected %s was %s, peer=%d\n", SanitizeString(msg.m_command), msg.m_message_size, HexStr(Span(hash.begin(), hash.begin() + CMessageHeader::CHECKSUM_SIZE)), - HexStr(hdr.pchChecksum)); + HexStr(hdr.pchChecksum), + m_node_id); } // store receive time @@ -2828,7 +2829,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } - m_deserializer = MakeUnique(V1TransportDeserializer(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION)); + m_deserializer = MakeUnique(V1TransportDeserializer(Params().MessageStart(), GetId(), SER_NETWORK, INIT_PROTO_VERSION)); m_serializer = MakeUnique(V1TransportSerializer()); } -- cgit v1.2.3 From 890b1d7c2b8312d41d048d2db124586c5dbc8a49 Mon Sep 17 00:00:00 2001 From: Troy Giorshev Date: Mon, 29 Jun 2020 14:15:06 -0400 Subject: Move checksum check from net_processing to net This removes the m_valid_checksum member from CNetMessage. Instead, GetMessage() returns an Optional. Additionally, GetMessage() has been given an out parameter to be used to hold error information. For now it is specifically a uint32_t used to hold the raw size of the corrupt message. The checksum check is now done in GetMessage. --- src/net.cpp | 47 +++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index 73029655ce..3e015a6810 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -595,25 +595,33 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete while (nBytes > 0) { // absorb network data int handled = m_deserializer->Read(pch, nBytes); - if (handled < 0) return false; + if (handled < 0) { + return false; + } pch += handled; nBytes -= handled; if (m_deserializer->Complete()) { // decompose a transport agnostic CNetMessage from the deserializer - CNetMessage msg = m_deserializer->GetMessage(Params().MessageStart(), time); + uint32_t out_err_raw_size{0}; + Optional result{m_deserializer->GetMessage(Params().MessageStart(), time, out_err_raw_size)}; + if (!result) { + // store the size of the corrupt message + mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER)->second += out_err_raw_size; + continue; + } //store received bytes per message command //to prevent a memory DOS, only allow valid commands - mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.m_command); + mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(result->m_command); if (i == mapRecvBytesPerMsgCmd.end()) i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER); assert(i != mapRecvBytesPerMsgCmd.end()); - i->second += msg.m_raw_message_size; + i->second += result->m_raw_message_size; // push the message to the process queue, - vRecvMsg.push_back(std::move(msg)); + vRecvMsg.push_back(std::move(*result)); complete = true; } @@ -679,37 +687,36 @@ const uint256& V1TransportDeserializer::GetMessageHash() const return data_hash; } -CNetMessage V1TransportDeserializer::GetMessage(const CMessageHeader::MessageStartChars& message_start, const std::chrono::microseconds time) +Optional V1TransportDeserializer::GetMessage(const CMessageHeader::MessageStartChars& message_start, const std::chrono::microseconds time, uint32_t& out_err_raw_size) { // decompose a single CNetMessage from the TransportDeserializer - CNetMessage msg(std::move(vRecv)); + Optional msg(std::move(vRecv)); // store state about valid header, netmagic and checksum - msg.m_valid_header = hdr.IsValid(message_start); - msg.m_valid_netmagic = (memcmp(hdr.pchMessageStart, message_start, CMessageHeader::MESSAGE_START_SIZE) == 0); + msg->m_valid_header = hdr.IsValid(message_start); + msg->m_valid_netmagic = (memcmp(hdr.pchMessageStart, message_start, CMessageHeader::MESSAGE_START_SIZE) == 0); uint256 hash = GetMessageHash(); - // store command string, payload size - msg.m_command = hdr.GetCommand(); - msg.m_message_size = hdr.nMessageSize; - msg.m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE; + // store command string, time, and sizes + msg->m_command = hdr.GetCommand(); + msg->m_time = time; + msg->m_message_size = hdr.nMessageSize; + msg->m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE; // We just received a message off the wire, harvest entropy from the time (and the message checksum) RandAddEvent(ReadLE32(hash.begin())); - msg.m_valid_checksum = (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0); - if (!msg.m_valid_checksum) { + if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) { LogPrint(BCLog::NET, "CHECKSUM ERROR (%s, %u bytes), expected %s was %s, peer=%d\n", - SanitizeString(msg.m_command), msg.m_message_size, + SanitizeString(msg->m_command), msg->m_message_size, HexStr(Span(hash.begin(), hash.begin() + CMessageHeader::CHECKSUM_SIZE)), HexStr(hdr.pchChecksum), m_node_id); + out_err_raw_size = msg->m_raw_message_size; + msg = nullopt; } - // store receive time - msg.m_time = time; - - // reset the network deserializer (prepare for the next message) + // Always reset the network deserializer (prepare for the next message) Reset(); return msg; } -- cgit v1.2.3 From 1ca20c1af8f08f07c407c3183c37b467ddf0f413 Mon Sep 17 00:00:00 2001 From: Troy Giorshev Date: Tue, 26 May 2020 16:01:03 -0400 Subject: Add doxygen comment for ReceiveMsgBytes --- src/net.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index 3e015a6810..fdb76d3b83 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -585,6 +585,16 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) } #undef X +/** + * Receive bytes from the buffer and deserialize them into messages. + * + * @param[in] pch A pointer to the raw data + * @param[in] nBytes Size of the data + * @param[out] complete Set True if at least one message has been + * deserialized and is ready to be processed + * @return True if the peer should stay connected, + * False if the peer should be disconnected from. + */ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete) { complete = false; -- cgit v1.2.3 From 5bceef6b12fa16d20287693be377dace3dfec3e5 Mon Sep 17 00:00:00 2001 From: Troy Giorshev Date: Mon, 8 Jun 2020 22:37:55 -0400 Subject: Change CMessageHeader Constructor This commit removes the single-parameter contructor of CMessageHeader and replaces it with a default constructor. The single parameter contructor isn't used anywhere except for tests. There is no reason to initialize a CMessageHeader with a particular messagestart. This messagestart should always be replaced when deserializing an actual message header so that we can run checks on it. The default constructor initializes it to zero, just like the command and checksum. This also removes a parameter of a V1TransportDeserializer constructor, as it was only used for this purpose. --- src/net.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index fdb76d3b83..1ae4b8fe08 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2846,7 +2846,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } - m_deserializer = MakeUnique(V1TransportDeserializer(Params().MessageStart(), GetId(), SER_NETWORK, INIT_PROTO_VERSION)); + m_deserializer = MakeUnique(V1TransportDeserializer(GetId(), SER_NETWORK, INIT_PROTO_VERSION)); m_serializer = MakeUnique(V1TransportSerializer()); } -- cgit v1.2.3 From 52d4ae46ab822d0f54e246a6f2364415cda149bd Mon Sep 17 00:00:00 2001 From: Troy Giorshev Date: Mon, 8 Jun 2020 22:26:22 -0400 Subject: Give V1TransportDeserializer CChainParams& member This adds a CChainParams& member to V1TransportDeserializer member, and use it in place of many Params() calls. In addition to reducing the number of calls to a global, this removes a parameter from GetMessage (and will later allow us to remove one from CMessageHeader::IsValid()) --- src/net.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index 1ae4b8fe08..941ea3c4ac 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -10,7 +10,6 @@ #include #include -#include #include #include #include @@ -615,7 +614,7 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete if (m_deserializer->Complete()) { // decompose a transport agnostic CNetMessage from the deserializer uint32_t out_err_raw_size{0}; - Optional result{m_deserializer->GetMessage(Params().MessageStart(), time, out_err_raw_size)}; + Optional result{m_deserializer->GetMessage(time, out_err_raw_size)}; if (!result) { // store the size of the corrupt message mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER)->second += out_err_raw_size; @@ -697,15 +696,14 @@ const uint256& V1TransportDeserializer::GetMessageHash() const return data_hash; } -Optional V1TransportDeserializer::GetMessage(const CMessageHeader::MessageStartChars& message_start, const std::chrono::microseconds time, uint32_t& out_err_raw_size) +Optional V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, uint32_t& out_err_raw_size) { // decompose a single CNetMessage from the TransportDeserializer Optional msg(std::move(vRecv)); // store state about valid header, netmagic and checksum - msg->m_valid_header = hdr.IsValid(message_start); - msg->m_valid_netmagic = (memcmp(hdr.pchMessageStart, message_start, CMessageHeader::MESSAGE_START_SIZE) == 0); - uint256 hash = GetMessageHash(); + msg->m_valid_header = hdr.IsValid(m_chain_params.MessageStart()); + msg->m_valid_netmagic = (memcmp(hdr.pchMessageStart, m_chain_params.MessageStart(), CMessageHeader::MESSAGE_START_SIZE) == 0); // store command string, time, and sizes msg->m_command = hdr.GetCommand(); @@ -713,6 +711,8 @@ Optional V1TransportDeserializer::GetMessage(const CMessageHeader:: msg->m_message_size = hdr.nMessageSize; msg->m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE; + uint256 hash = GetMessageHash(); + // We just received a message off the wire, harvest entropy from the time (and the message checksum) RandAddEvent(ReadLE32(hash.begin())); @@ -2846,7 +2846,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } - m_deserializer = MakeUnique(V1TransportDeserializer(GetId(), SER_NETWORK, INIT_PROTO_VERSION)); + m_deserializer = MakeUnique(V1TransportDeserializer(Params(), GetId(), SER_NETWORK, INIT_PROTO_VERSION)); m_serializer = MakeUnique(V1TransportSerializer()); } -- cgit v1.2.3 From deb52711a17236d0fca302701b5af585341ab42a Mon Sep 17 00:00:00 2001 From: Troy Giorshev Date: Tue, 26 May 2020 17:01:57 -0400 Subject: Remove header checks out of net_processing This moves header size and netmagic checking out of net_processing and into net. This check now runs in ReadHeader, so that net can exit early out of receiving bytes from the peer. IsValid is now slimmed down, so it no longer needs a MessageStartChars& parameter. Additionally this removes the rest of the m_valid_* members from CNetMessage. --- src/net.cpp | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) (limited to 'src/net.cpp') diff --git a/src/net.cpp b/src/net.cpp index 941ea3c4ac..633f9a2f7f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -605,6 +605,7 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete // absorb network data int handled = m_deserializer->Read(pch, nBytes); if (handled < 0) { + // Serious header problem, disconnect from the peer. return false; } @@ -616,6 +617,7 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete uint32_t out_err_raw_size{0}; Optional result{m_deserializer->GetMessage(time, out_err_raw_size)}; if (!result) { + // Message deserialization failed. Drop the message but don't disconnect the peer. // store the size of the corrupt message mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER)->second += out_err_raw_size; continue; @@ -657,11 +659,19 @@ int V1TransportDeserializer::readHeader(const char *pch, unsigned int nBytes) hdrbuf >> hdr; } catch (const std::exception&) { + LogPrint(BCLog::NET, "HEADER ERROR - UNABLE TO DESERIALIZE, peer=%d\n", m_node_id); + return -1; + } + + // Check start string, network magic + if (memcmp(hdr.pchMessageStart, m_chain_params.MessageStart(), CMessageHeader::MESSAGE_START_SIZE) != 0) { + LogPrint(BCLog::NET, "HEADER ERROR - MESSAGESTART (%s, %u bytes), received %s, peer=%d\n", hdr.GetCommand(), hdr.nMessageSize, HexStr(hdr.pchMessageStart), m_node_id); return -1; } // reject messages larger than MAX_SIZE or MAX_PROTOCOL_MESSAGE_LENGTH if (hdr.nMessageSize > MAX_SIZE || hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) { + LogPrint(BCLog::NET, "HEADER ERROR - SIZE (%s, %u bytes), peer=%d\n", hdr.GetCommand(), hdr.nMessageSize, m_node_id); return -1; } @@ -701,10 +711,6 @@ Optional V1TransportDeserializer::GetMessage(const std::chrono::mic // decompose a single CNetMessage from the TransportDeserializer Optional msg(std::move(vRecv)); - // store state about valid header, netmagic and checksum - msg->m_valid_header = hdr.IsValid(m_chain_params.MessageStart()); - msg->m_valid_netmagic = (memcmp(hdr.pchMessageStart, m_chain_params.MessageStart(), CMessageHeader::MESSAGE_START_SIZE) == 0); - // store command string, time, and sizes msg->m_command = hdr.GetCommand(); msg->m_time = time; @@ -716,6 +722,7 @@ Optional V1TransportDeserializer::GetMessage(const std::chrono::mic // We just received a message off the wire, harvest entropy from the time (and the message checksum) RandAddEvent(ReadLE32(hash.begin())); + // Check checksum and header command string if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) { LogPrint(BCLog::NET, "CHECKSUM ERROR (%s, %u bytes), expected %s was %s, peer=%d\n", SanitizeString(msg->m_command), msg->m_message_size, @@ -724,6 +731,11 @@ Optional V1TransportDeserializer::GetMessage(const std::chrono::mic m_node_id); out_err_raw_size = msg->m_raw_message_size; msg = nullopt; + } else if (!hdr.IsCommandValid()) { + LogPrint(BCLog::NET, "HEADER ERROR - COMMAND (%s, %u bytes), peer=%d\n", + hdr.GetCommand(), msg->m_message_size, m_node_id); + out_err_raw_size = msg->m_raw_message_size; + msg = nullopt; } // Always reset the network deserializer (prepare for the next message) -- cgit v1.2.3