Cleanup some of the fragmented levin handling
What the developer wrote
Authored by Lee *!* Clagett
Cleanup some of the fragmented levin handling
What changed, and why it matters
This commit tightens how Monero's network code reassembles split-up network messages. Previously, when fragments were stitched back together, the code trusted the size written in the message header without checking whether the actual reassembled buffer was at least that large. That could let a malformed message cause the code to read past the end of its buffer. The patch adds a size check and trims the buffer to exactly the claimed size before further processing. A new test confirms the handler now rejects a case where the header claims more data than the fragments actually provide.
Treat this as a hardening fix for a likely memory-safety bug in network message reassembly. Review whether the same validation is needed in any other reassembly paths, and consider requesting a CVE if a reproducible crash or information leak can be demonstrated. Users running nodes should upgrade to a release containing this commit.
Security signals we found
Buffer size validation added before slicing reassembled payload
Potential out-of-bounds read in fragmented message reassembly addressed
New negative unit test for inconsistent fragment payload size
No explicit CVE or vendor security advisory referenced in commit
Evidence from the diff
In contrib/epee/include/net/levin_protocol_handler_async.h, the fragmented levin reassembly path now validates that the payload size (inner_size from bucket_head2.m_cb) does not exceed the actual reassembled buffer length before slicing buff_to_invoke. It also replaces std::move+clear with swap and clamps buff_to_invoke to inner_size. The unit test removes an old padding resize and adds a ‘handles_bad_cb’ test that sends a BEGIN fragment with a 2-byte payload claim followed by an END fragment with only 1 byte, expecting handle_recv to return false.
Changed components
contrib/epee/include/net/levin_protocol_handler_async.htests/unit_tests/epee_levin_protocol_handler_async.cppMonero P2P levin protocol handlerInspect captured patch +33 / −5
diff --git a/contrib/epee/include/net/levin_protocol_handler_async.h b/contrib/epee/include/net/levin_protocol_handler_async.h
index 341522b..d5eff5b 100644
--- a/contrib/epee/include/net/levin_protocol_handler_async.h
+++ b/contrib/epee/include/net/levin_protocol_handler_async.h
@@ -470,18 +470,26 @@ public:
return false;
}
- temp = std::move(m_fragment_buffer);
- m_fragment_buffer.clear();
+ temp.swap(m_fragment_buffer);
std::memcpy(std::addressof(m_current_head), std::addressof(temp[0]), sizeof(bucket_head2));
+ const std::uint64_t inner_size = SWAP64LE(m_current_head.m_cb);
+ buff_to_invoke = {reinterpret_cast<const uint8_t*>(temp.data()) + sizeof(bucket_head2), temp.size() - sizeof(bucket_head2)};
+ if (buff_to_invoke.size() < inner_size)
+ {
+ MERROR(m_connection_context << "Invalid fragmented buffer size: " << buff_to_invoke.size() << " vs " << inner_size);
+ return false;
+ }
+
+ buff_to_invoke = {buff_to_invoke.data(), std::size_t(inner_size)};
+
const size_t max_bytes = m_connection_context.get_max_bytes(m_current_head.m_command);
- if(m_current_head.m_cb > std::min<size_t>(max_packet_size, max_bytes))
+ if(buff_to_invoke.size() > std::min<size_t>(max_packet_size, max_bytes))
{
MERROR(m_connection_context << "Maximum packet size exceed!, m_max_packet_size = " << std::min<size_t>(max_packet_size, max_bytes)
<< ", packet header received " << m_current_head.m_cb << ", command " << m_current_head.m_command
<< ", connection will be closed.");
return false;
}
- buff_to_invoke = {reinterpret_cast<const uint8_t*>(temp.data()) + sizeof(bucket_head2), temp.size() - sizeof(bucket_head2)};
}
bool is_response = (m_oponent_protocol_ver == LEVIN_PROTOCOL_VER_1 && m_current_head.m_flags&LEVIN_PACKET_RESPONSE);
diff --git a/tests/unit_tests/epee_levin_protocol_handler_async.cpp b/tests/unit_tests/epee_levin_protocol_handler_async.cpp
index 9677081..729b9e0 100644
--- a/tests/unit_tests/epee_levin_protocol_handler_async.cpp
+++ b/tests/unit_tests/epee_levin_protocol_handler_async.cpp
@@ -506,7 +506,6 @@ TEST_F(positive_test_connection_to_levin_protocol_handler_calls, handler_process
}
std::string compare_buffer(1024 * 4, 'c');
- compare_buffer.resize(((1024 - sizeof(epee::levin::bucket_head2)) * 5) - sizeof(epee::levin::bucket_head2)); // add padding zeroes
ASSERT_EQ(4u, m_commands_handler.notify_counter());
ASSERT_EQ(0u, m_commands_handler.invoke_counter());
@@ -652,3 +651,24 @@ TEST_F(test_levin_protocol_handler__hanle_recv_with_invalid_data, handles_short_
ASSERT_FALSE(m_conn->m_protocol_handler.handle_recv(m_buf.data(), m_buf.size()));
}
+
+TEST_F(test_levin_protocol_handler__hanle_recv_with_invalid_data, handles_bad_cb)
+{
+ m_req_head.m_cb = sizeof(epee::levin::bucket_head2);
+ m_req_head.m_flags = LEVIN_PACKET_BEGIN;
+ m_req_head.m_command = 0;
+
+ epee::levin::bucket_head2 inner{};
+ inner.m_cb = 2;
+ m_in_data.resize(sizeof(epee::levin::bucket_head2));
+ prepare_buf();
+
+ ASSERT_TRUE(m_conn->m_protocol_handler.handle_recv(m_buf.data(), m_buf.size()));
+
+ m_req_head.m_cb = 1;
+ m_req_head.m_flags = LEVIN_PACKET_END;
+ m_in_data.resize(1);
+ prepare_buf();
+
+ ASSERT_FALSE(m_conn->m_protocol_handler.handle_recv(m_buf.data(), m_buf.size()));
+}
Why this scored 58/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.