60- seqnr_payload = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
61- },
62- [&] {
63- seqnr_aad = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
64+ uint32_t len = aead.DecryptLength(in.data());
65+ len = 0; // addressing the [[nodiscard]] and otherwise unused variable
0 (void)aead.DecryptLength(in.data());
Does this not work?
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | stratospher |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
772+ if (m_hdr_pos < CHACHA20_POLY1305_AEAD_AAD_LEN) {
773+ return copy_bytes;
774+ }
775+
776+ // we got the AAD bytes at this point (3 bytes encrypted packet length)
777+ // we keep the sequence numbers unchanged at this point. Once the message is authenticated and decrypted, we increase the sequence numbers (or the aad_pos)
This comment can be removed since it’s related to the AEAD cipher suite implementation and doesn’t play a role here.
776+ // we got the AAD bytes at this point (3 bytes encrypted packet length)
777+ // we keep the sequence numbers unchanged at this point. Once the message is authenticated and decrypted, we increase the sequence numbers (or the aad_pos)
778+ m_message_size = m_aead->DecryptLength((const uint8_t*)vRecv.data());
779+
780+ // reject messages larger than MAX_SIZE
781+ if (m_message_size > MAX_SIZE) {
803+ m_data_pos += copy_bytes;
804+
805+ return copy_bytes;
806+}
807+
808+std::optional<CNetMessage> V2TransportDeserializer::GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size)
882+ // message command without an assigned short-ID
883+ assert(msg.m_type.size() <= NET_P2P_V2_CMD_MAX_CHARS_SIZE);
884+ // encode as varstr, max 12 chars
885+ serialized_command_size = ::GetSerializeSize(msg.m_type, PROTOCOL_VERSION);
886+ }
887+ // prepare the packet length that will later be encrypted and part of the MAC (AD)
0 // prepare the packet length that will later be encrypted and part of the MAC (AAD)
905+ vector_writer << msg.m_type;
906+ }
907+
908+ // insert header directly into the CSerializedNetMsg data buffer (insert at begin)
909+ // TODO: if we refactor the ChaCha20Poly1350 crypt function to allow separate buffers for
910+ // the AD, payload and MAC, we could avoid a insert and thus a potential reallocation
0 // the AAD, payload and MAC, we could avoid a insert and thus a potential reallocation
383+private:
384+ std::unique_ptr<ChaCha20Poly1305AEAD> m_aead;
385+ const NodeId m_node_id; // Only for logging
386+ bool m_in_data = false; // parsing header (false) or data (true)
387+ uint32_t m_message_size = 0; // expected message size
388+ CDataStream vRecv; // received message data
0 CDataStream vRecv; // received message header and data
net covers both transport and p2p(application) layers to some degree. For the transport layer, perhaps the “header” is the encrypted length. For the application layer, the p2p message type is arguably part of the “header”. I updated the comment to be clear in a different way though.
904+ // or the ASCII command string
905+ vector_writer << msg.m_type;
906+ }
907+
908+ // insert header directly into the CSerializedNetMsg data buffer (insert at begin)
909+ // TODO: if we refactor the ChaCha20Poly1350 crypt function to allow separate buffers for
0 // TODO: if we refactor the ChaCha20Poly1305 crypt function to allow separate buffers for
805+ return copy_bytes;
806+}
807+
808+std::optional<CNetMessage> V2TransportDeserializer::GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size)
809+{
810+ // In v2, vRecv contains the encrypted payload plus the MAC tag (1+bytes serialized message command + ? bytes message payload + 16 byte mac tag)
0 // In v2, vRecv contains the AAD, encrypted payload plus the MAC tag (3 byte AAD + 1+bytes serialized message command + ? bytes message payload + 16 byte mac tag)
740+ Span<const uint8_t> span_header(serialized_header.data(), serialized_header.size());
741+ if (serialized_header.size() > 0) read_bytes += deserializer->Read(span_header);
742+ // second: read the encrypted payload (if required)
743+ Span<const uint8_t> span_msg(msg.data.data(), msg.data.size());
744+ if (msg.data.size() > 0) read_bytes += deserializer->Read(span_msg);
745+ if (msg.data.size() > read_bytes && msg.data.size() - read_bytes > 0) {
0 if (msg.data.size() > read_bytes) {
748+ }
749+ BOOST_CHECK(deserializer->Complete());
750+ BOOST_CHECK_EQUAL(read_bytes, msg.data.size() + serialized_header.size());
751+ // message must be complete
752+ uint32_t out_err_raw_size{0};
753+ std::optional<CNetMessage> result{deserializer->GetMessage(GetTime<std::chrono::microseconds>(), out_err_raw_size)};
This can be updated to keep it consistent with 22735’s change.
0 bool reject_message{false};
1 CNetMessage result{deserializer->GetMessage(GetTime<std::chrono::microseconds>(), reject_message)};
25+ break;
26+ }
27+ if (deserializer.Complete()) {
28+ const std::chrono::microseconds m_time{std::numeric_limits<int64_t>::max()};
29+ uint32_t out_err_raw_size{0};
30+ std::optional<CNetMessage> result{deserializer.GetMessage(m_time, out_err_raw_size)};
0 bool reject_message{false};
1 CNetMessage result{deserializer.GetMessage(m_time, reject_message)};
Here also.
Concept ACK ab5b49d. This PR nicely implements the v2 transport serialisation and deserialisation process.
allNetMessageTypes[] is represented as a map and functions to convert short-ID to a message and vice versa are added. Verified that changes don’t need to be made to other files due to this commit.readHeader() : copies the header from msg_bytes to location in vRecv[] which is addressed by m_hdr_pos. After the entire header is read, it switches to read message data mode(m_in_data = true).readData() : copies the data from msg_bytes to location in vRecv[] which is addressed by CHACHA20_POLY1305_AEAD_AAD_LEN + m_data_pos. vRecv[] is resized to at max 256 KiB forward if there’s no space to hold msg_bytes.GetMessage() : performs decryption of vRecv[] and chops off the AAD and MAC tag if successful decryption occurs. It reads the first byte of the payload in size_or_shortid. If size_or_shortid is a number between 1 and 12, then it’s a string command and the next size_or_shortid bytes is read into command_name. If size_or_shortid is larger than 12, then it’s a Message-Type-ID and if it’s string mapping is found, it’s stored in command_name. Otherwise the size_or_shortid is appended with the prefix ‘unknown’ and stored in command_name. CNetMessage msg is constructed and returned.prepareForTransport() : prepares the CSerializedNetMsg msg for serialisation. serialized_command_size is used to store the size of short id (1 byte) or the size of message command string if short id isn’t assigned. packet_length contains the length of the message type id and payload which is stored in a LE representation in serialized_header. The short id/ command string is also appended to it. serialized_header is then inserted into the beginning of msg buffer. Encryption is performed on msg and returns true when successful.
859+ Reset();
860+ reject_message = true;
861+ } else if (!valid_checksum) {
862+ LogPrint(BCLog::NET, "DECRYPTION INVALID MAC, peer=%d\n", m_node_id);
863+ Reset();
864+ reject_message = true;
@stratospher found a couple crashing fuzz seeds. They were occurring because the fuzz test allowed for a mac_assist to be provided even if the length was not assisted. And when that happens, the test assertion for a valid mac tag would fail.
Rebased with master due to unrelated tests failing on CI.
Ready for further review.
45@@ -46,46 +46,44 @@ const char *CFCHECKPT="cfcheckpt";
46 const char *WTXIDRELAY="wtxidrelay";
47 } // namespace NetMsgType
48
49-/** All known message types. Keep this in the same order as the list of
50+/** All known message types including the short-ID. Keep this in the same order as the list of
119+ {39, NetMsgType::CFILTER},
120+ {40, NetMsgType::GETCFHEADERS},
121+ {41, NetMsgType::CFHEADERS},
122+ {42, NetMsgType::GETCFCHECKPT},
123+ {43, NetMsgType::CFCHECKPT},
124+ {44, NetMsgType::WTXIDRELAY}};
678- if (aad_pos + CHACHA20_POLY1305_AEAD_AAD_LEN > CHACHA20_ROUND_OUTPUT) {
679- aad_pos = 0;
680- seqnr_aad++;
681- }
682+ // compare at iteration 999 against the test vector
683+ if (i == 999) BOOST_CHECK(memcmp(ciphertext_buf.data(), expected_ciphertext_and_mac_sequence999.data(), expected_ciphertext_and_mac_sequence999.size()) == 0);
23+ V2TransportSerializer serializer{k1, k2};
24+ FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
25+
26+ bool length_assist = fuzzed_data_provider.ConsumeBool();
27+
28+ // There is no sense is providing a mac assist if the length is incorrect.
31+
32+ if (payload_bytes.size() >= CHACHA20_POLY1305_AEAD_AAD_LEN + CHACHA20_POLY1305_AEAD_TAG_LEN) {
33+ if (length_assist) {
34+ uint32_t packet_length = payload_bytes.size() - CHACHA20_POLY1305_AEAD_AAD_LEN - CHACHA20_POLY1305_AEAD_TAG_LEN;
35+ packet_length = htole32(packet_length);
36+ memcpy(payload_bytes.data(), &packet_length, 3);
WriteLE32 here instead of htole32 and memcmp directly
I might be missing something, but, wouldn’t WriteLE32 here mean that the fourth byte (which is the first byte in the ciphertext payload) would be written over. Is there a way for me to use WriteLE32 only for 3 bytes?
I guess that is still deterministic, so the fuzz test would work, but I’m not sure if it will work against the fuzzing engine.
Oh you’re right, here you see how easy it is to miss a off-by-one like that with memcpy! Never mind my comment. What about writing it out explicitly?
0payload_bytes.data[0] = packet_length & 0xff;
1payload_bytes.data[1] = (packet_length >> 8) & 0xff;
2payload_bytes.data[2] = (packet_length >> 16) & 0xff;
274+ * returns the short ID for a message type (if known) */
275+std::optional<uint8_t> GetShortIDFromMessageType(const std::string& message_type);
276+
277+/** returns the message type (string) from a short ID
278+ * returns an empty string if short ID has not been found */
279+bool GetMessageTypeFromShortID(const uint8_t shortID, std::string& message_type);
std::optional<std::string> instead of a separate bool and output argument (more symmetrical with GetShortIDFromMessageType as well).
73+ChaCha20Forward4064::~ChaCha20Forward4064()
74+{
75+ memory_cleanse(m_keystream, KEYSTREAM_SIZE);
76+}
77+
78+ChaCha20Poly1305AEAD::ChaCha20Poly1305AEAD(const unsigned char* K_1,
Span instead of `const unsigned char*, size_t`` pairs (here and in some other places)
222@@ -225,3 +223,23 @@ GenTxid ToGenTxid(const CInv& inv)
223 assert(inv.IsGenTxMsg());
224 return inv.IsMsgWtx() ? GenTxid::Wtxid(inv.hash) : GenTxid::Txid(inv.hash);
225 }
226+
227+std::optional<uint8_t> GetShortIDFromMessageType(const std::string& message_type)
228+{
229+ for (const std::pair<uint8_t, std::string> entry : allNetMessageTypes) {
230+ if (entry.second == message_type) {
152+ * (using the ChaCha20 stream keyed with K_2) and append it to the encrypted
153+ * length. Finally, calculate a MAC tag (using poly1305 key from stream keyed with K_1)
154+ * and append it.
155 */
156
157+const int KEYSTREAM_SIZE = 4096;
size_t?
109@@ -107,6 +110,8 @@ const std::string NET_MESSAGE_COMMAND_OTHER = "*other*";
110 static const uint64_t RANDOMIZER_ID_NETGROUP = 0x6c0edd8036ef4036ULL; // SHA256("netgroup")[0:8]
111 static const uint64_t RANDOMIZER_ID_LOCALHOSTNONCE = 0xd93e69e2bbfa5735ULL; // SHA256("localhostnonce")[0:8]
112 static const uint64_t RANDOMIZER_ID_ADDRCACHE = 0x1cf2e4ddd306dda9ULL; // SHA256("addrcache")[0:8]
113+
114+static constexpr uint8_t NET_P2P_V2_CMD_MAX_CHARS_SIZE = 12; //maximal length for V2 (BIP324) string message commands
//
790@@ -776,7 +791,160 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds
791 return msg;
792 }
793
794-void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) {
795+int V2TransportDeserializer::readHeader(Span<const uint8_t> msg_bytes)
796+{
797+ // copy data to temporary parsing buffer
798+ const unsigned int remaining = CHACHA20_POLY1305_AEAD_AAD_LEN - m_hdr_pos;
799+ const unsigned int copy_bytes = std::min<unsigned int>(remaining, msg_bytes.size());
unsigned int for offsets/sizes here and in readData instead of size_t?
864+ uint8_t size_or_shortid = 0;
865+ try {
866+ vRecv >> size_or_shortid;
867+ } catch (const std::ios_base::failure&) {
868+ LogPrint(BCLog::NET, "Invalid message type, peer=%d\n", m_node_id);
869+ reject_message = true;
size_or_shortid will stay at 0, and it can simply continue below?
msg below hold and that makes fuzz testing easier. The client of this function is expected to reject the message in this case.
Pushed to address #23233 (review)
Ready for further review.
Rebased. Ready for further review.
git range-diff 02e1d8d06f a999f11 ed5f00d
Updated code due to bug found while fuzzing and testing a new BIP324 PR which is WIP. Please review diff with: git range-diff refs/heads/master d7afc96 HEAD
Ready for further review.
Rebased. git range-diff e0680bbce 8264b14 92c51da7d.
Ready for further review.
Modified code to bring in upstream interface changes from #25361 and exposed the BIP324CipherSuite AAD via the V2TransportSerializer and V2TransportDeserializerclasses to facilitate the authentication of the garbage bytes in the shapable handshake.
Ready for further review.
Latest push:
260@@ -261,7 +261,16 @@ extern const char* WTXIDRELAY;
261 }; // namespace NetMsgType
262
263 /* Get a vector of all valid message types (see above) */
811@@ -797,7 +812,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds
812 return msg;
813 }
814
815-void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const
816+bool V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const
header to transport_header.
Latest push:
Latest push:
Co-authored-by: Jonas Schnelli <dev@jonasschnelli.ch>
Co-authored-by: Jonas Schnelli <dev@jonasschnelli.ch>
