From 63b13aa51915a647ce702ed229e912762aad7ffc Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 13 Sep 2024 09:29:01 +0000 Subject: [PATCH] merge bitcoin#28525: Drop v2 garbage authentication packet --- src/net.cpp | 76 +++++++++++++++--------------------------- src/net.h | 41 ++++++++++------------- src/test/net_tests.cpp | 58 +++++++++++++++----------------- 3 files changed, 71 insertions(+), 104 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index db17c1b881..76723a4fd5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1098,8 +1098,7 @@ void V2Transport::StartSendingHandshake() noexcept m_send_buffer.resize(EllSwiftPubKey::size() + m_send_garbage.size()); std::copy(std::begin(m_cipher.GetOurPubKey()), std::end(m_cipher.GetOurPubKey()), MakeWritableByteSpan(m_send_buffer).begin()); std::copy(m_send_garbage.begin(), m_send_garbage.end(), m_send_buffer.begin() + EllSwiftPubKey::size()); - // We cannot wipe m_send_garbage as it will still be used to construct the garbage - // authentication packet. + // We cannot wipe m_send_garbage as it will still be used as AAD later in the handshake. } V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span ent32, std::vector garbage) noexcept : @@ -1133,9 +1132,6 @@ void V2Transport::SetReceiveState(RecvState recv_state) noexcept Assume(recv_state == RecvState::GARB_GARBTERM); break; case RecvState::GARB_GARBTERM: - Assume(recv_state == RecvState::GARBAUTH); - break; - case RecvState::GARBAUTH: Assume(recv_state == RecvState::VERSION); break; case RecvState::VERSION: @@ -1267,25 +1263,16 @@ bool V2Transport::ProcessReceivedKeyBytes() noexcept m_cipher.GetSendGarbageTerminator().end(), MakeWritableByteSpan(m_send_buffer).last(BIP324Cipher::GARBAGE_TERMINATOR_LEN).begin()); - // Construct garbage authentication packet in the send buffer (using the garbage data which - // is still there). - m_send_buffer.resize(m_send_buffer.size() + BIP324Cipher::EXPANSION); - m_cipher.Encrypt( - /*contents=*/{}, - /*aad=*/MakeByteSpan(m_send_garbage), - /*ignore=*/false, - /*output=*/MakeWritableByteSpan(m_send_buffer).last(BIP324Cipher::EXPANSION)); - // We no longer need the garbage. - m_send_garbage.clear(); - m_send_garbage.shrink_to_fit(); - - // Construct version packet in the send buffer. + // Construct version packet in the send buffer, with the sent garbage data as AAD. m_send_buffer.resize(m_send_buffer.size() + BIP324Cipher::EXPANSION + VERSION_CONTENTS.size()); m_cipher.Encrypt( /*contents=*/VERSION_CONTENTS, - /*aad=*/{}, + /*aad=*/MakeByteSpan(m_send_garbage), /*ignore=*/false, /*output=*/MakeWritableByteSpan(m_send_buffer).last(BIP324Cipher::EXPANSION + VERSION_CONTENTS.size())); + // We no longer need the garbage. + m_send_garbage.clear(); + m_send_garbage.shrink_to_fit(); } else { // We still have to receive more key bytes. } @@ -1299,11 +1286,11 @@ bool V2Transport::ProcessReceivedGarbageBytes() noexcept Assume(m_recv_buffer.size() <= MAX_GARBAGE_LEN + BIP324Cipher::GARBAGE_TERMINATOR_LEN); if (m_recv_buffer.size() >= BIP324Cipher::GARBAGE_TERMINATOR_LEN) { if (MakeByteSpan(m_recv_buffer).last(BIP324Cipher::GARBAGE_TERMINATOR_LEN) == m_cipher.GetReceiveGarbageTerminator()) { - // Garbage terminator received. Switch to receiving garbage authentication packet. - m_recv_garbage = std::move(m_recv_buffer); - m_recv_garbage.resize(m_recv_garbage.size() - BIP324Cipher::GARBAGE_TERMINATOR_LEN); + // Garbage terminator received. Store garbage to authenticate it as AAD later. + m_recv_aad = std::move(m_recv_buffer); + m_recv_aad.resize(m_recv_aad.size() - BIP324Cipher::GARBAGE_TERMINATOR_LEN); m_recv_buffer.clear(); - SetReceiveState(RecvState::GARBAUTH); + SetReceiveState(RecvState::VERSION); } else if (m_recv_buffer.size() == MAX_GARBAGE_LEN + BIP324Cipher::GARBAGE_TERMINATOR_LEN) { // We've reached the maximum length for garbage + garbage terminator, and the // terminator still does not match. Abort. @@ -1322,8 +1309,7 @@ bool V2Transport::ProcessReceivedGarbageBytes() noexcept bool V2Transport::ProcessReceivedPacketBytes() noexcept { AssertLockHeld(m_recv_mutex); - Assume(m_recv_state == RecvState::GARBAUTH || m_recv_state == RecvState::VERSION || - m_recv_state == RecvState::APP); + Assume(m_recv_state == RecvState::VERSION || m_recv_state == RecvState::APP); // The maximum permitted contents length for a packet, consisting of: // - 0x00 byte: indicating long message type encoding @@ -1346,45 +1332,38 @@ bool V2Transport::ProcessReceivedPacketBytes() noexcept // as GetMaxBytesToProcess only allows up to LENGTH_LEN into the buffer before that point. m_recv_decode_buffer.resize(m_recv_len); bool ignore{false}; - Span aad; - if (m_recv_state == RecvState::GARBAUTH) aad = MakeByteSpan(m_recv_garbage); bool ret = m_cipher.Decrypt( /*input=*/MakeByteSpan(m_recv_buffer).subspan(BIP324Cipher::LENGTH_LEN), - /*aad=*/aad, + /*aad=*/MakeByteSpan(m_recv_aad), /*ignore=*/ignore, /*contents=*/MakeWritableByteSpan(m_recv_decode_buffer)); if (!ret) { LogPrint(BCLog::NET, "V2 transport error: packet decryption failure (%u bytes), peer=%d\n", m_recv_len, m_nodeid); return false; } + // We have decrypted a valid packet with the AAD we expected, so clear the expected AAD. + m_recv_aad.clear(); + m_recv_aad.shrink_to_fit(); // Feed the last 4 bytes of the Poly1305 authentication tag (and its timing) into our RNG. RandAddEvent(ReadLE32(m_recv_buffer.data() + m_recv_buffer.size() - 4)); - // At this point we have a valid packet decrypted into m_recv_decode_buffer. Depending on - // the current state, decide what to do with it. - switch (m_recv_state) { - case RecvState::GARBAUTH: - // Ignore flag does not matter for garbage authentication. Any valid packet functions - // as authentication. Receive and process the version packet next. - SetReceiveState(RecvState::VERSION); - m_recv_garbage = {}; - break; - case RecvState::VERSION: - if (!ignore) { + // At this point we have a valid packet decrypted into m_recv_decode_buffer. If it's not a + // decoy, which we simply ignore, use the current state to decide what to do with it. + if (!ignore) { + switch (m_recv_state) { + case RecvState::VERSION: // Version message received; transition to application phase. The contents is // ignored, but can be used for future extensions. SetReceiveState(RecvState::APP); - } - break; - case RecvState::APP: - if (!ignore) { + break; + case RecvState::APP: // Application message decrypted correctly. It can be extracted using GetMessage(). SetReceiveState(RecvState::APP_READY); + break; + default: + // Any other state is invalid (this function should not have been called). + Assume(false); } - break; - default: - // Any other state is invalid (this function should not have been called). - Assume(false); } // Wipe the receive buffer where the next packet will be received into. m_recv_buffer = {}; @@ -1420,7 +1399,6 @@ size_t V2Transport::GetMaxBytesToProcess() noexcept case RecvState::GARB_GARBTERM: // Process garbage bytes one by one (because terminator may appear anywhere). return 1; - case RecvState::GARBAUTH: case RecvState::VERSION: case RecvState::APP: // These three states all involve decoding a packet. Process the length descriptor first, @@ -1474,7 +1452,6 @@ bool V2Transport::ReceivedBytes(Span& msg_bytes) noexcept // bytes). m_recv_buffer.reserve(MAX_GARBAGE_LEN + BIP324Cipher::GARBAGE_TERMINATOR_LEN); break; - case RecvState::GARBAUTH: case RecvState::VERSION: case RecvState::APP: { // During states where a packet is being received, as much as is expected but never @@ -1518,7 +1495,6 @@ bool V2Transport::ReceivedBytes(Span& msg_bytes) noexcept if (!ProcessReceivedGarbageBytes()) return false; break; - case RecvState::GARBAUTH: case RecvState::VERSION: case RecvState::APP: if (!ProcessReceivedPacketBytes()) return false; diff --git a/src/net.h b/src/net.h index d63d625b51..33c6c8908c 100644 --- a/src/net.h +++ b/src/net.h @@ -496,10 +496,10 @@ private: * * start(responder) * | - * | start(initiator) /---------\ - * | | | | - * v v v | - * KEY_MAYBE_V1 -> KEY -> GARB_GARBTERM -> GARBAUTH -> VERSION -> APP -> APP_READY + * | start(initiator) /---------\ + * | | | | + * v v v | + * KEY_MAYBE_V1 -> KEY -> GARB_GARBTERM -> VERSION -> APP -> APP_READY * | * \-------> V1 */ @@ -521,24 +521,19 @@ private: /** Garbage and garbage terminator. * * Whenever a byte is received, the last 16 bytes are compared with the expected garbage - * terminator. When that happens, the state becomes GARBAUTH. If no matching terminator is + * terminator. When that happens, the state becomes VERSION. If no matching terminator is * received in 4111 bytes (4095 for the maximum garbage length, and 16 bytes for the * terminator), the connection aborts. */ GARB_GARBTERM, - /** Garbage authentication packet. - * - * A packet is received, and decrypted/verified with AAD set to the garbage received during - * the GARB_GARBTERM state. If that succeeds, the state becomes VERSION. If it fails the - * connection aborts. */ - GARBAUTH, - /** Version packet. * - * A packet is received, and decrypted/verified. If that succeeds, the state becomes APP, - * and the decrypted contents is interpreted as version negotiation (currently, that means - * ignoring it, but it can be used for negotiating future extensions). If it fails, the - * connection aborts. */ + * A packet is received, and decrypted/verified. If that fails, the connection aborts. The + * first received packet in this state (whether it's a decoy or not) is expected to + * authenticate the garbage received during the GARB_GARBTERM state as associated + * authenticated data (AAD). The first non-decoy packet in this state is interpreted as + * version negotiation (currently, that means ignoring the contents, but it can be used for + * negotiating future extensions), and afterwards the state becomes APP. */ VERSION, /** Application packet. @@ -592,9 +587,9 @@ private: /** Normal sending state. * * In this state, the ciphers are initialized, so packets can be sent. When this state is - * entered, the garbage terminator, garbage authentication packet, and version - * packet are appended to the send buffer (in addition to the key and garbage which may - * still be there). In this state a message can be provided if the send buffer is empty. */ + * entered, the garbage terminator and version packet are appended to the send buffer (in + * addition to the key and garbage which may still be there). In this state a message can be + * provided if the send buffer is empty. */ READY, /** This transport is using v1 fallback. @@ -614,13 +609,13 @@ private: /** Lock for receiver-side fields. */ mutable Mutex m_recv_mutex ACQUIRED_BEFORE(m_send_mutex); - /** In {GARBAUTH, VERSION, APP}, the decrypted packet length, if m_recv_buffer.size() >= + /** In {VERSION, APP}, the decrypted packet length, if m_recv_buffer.size() >= * BIP324Cipher::LENGTH_LEN. Unspecified otherwise. */ uint32_t m_recv_len GUARDED_BY(m_recv_mutex) {0}; /** Receive buffer; meaning is determined by m_recv_state. */ std::vector m_recv_buffer GUARDED_BY(m_recv_mutex); - /** During GARBAUTH, the garbage received during GARB_GARBTERM. */ - std::vector m_recv_garbage GUARDED_BY(m_recv_mutex); + /** AAD expected in next received packet (currently used only for garbage). */ + std::vector m_recv_aad GUARDED_BY(m_recv_mutex); /** Buffer to put decrypted contents in, for converting to CNetMessage. */ std::vector m_recv_decode_buffer GUARDED_BY(m_recv_mutex); /** Deserialization type. */ @@ -660,7 +655,7 @@ private: bool ProcessReceivedKeyBytes() noexcept EXCLUSIVE_LOCKS_REQUIRED(m_recv_mutex, !m_send_mutex); /** Process bytes in m_recv_buffer, while in GARB_GARBTERM state. */ bool ProcessReceivedGarbageBytes() noexcept EXCLUSIVE_LOCKS_REQUIRED(m_recv_mutex); - /** Process bytes in m_recv_buffer, while in GARBAUTH/VERSION/APP state. */ + /** Process bytes in m_recv_buffer, while in VERSION/APP state. */ bool ProcessReceivedPacketBytes() noexcept EXCLUSIVE_LOCKS_REQUIRED(m_recv_mutex); public: diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 591efd01b0..241bfe64ec 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -1038,9 +1038,11 @@ class V2TransportTester bool m_test_initiator; //!< Whether m_transport is the initiator (true) or responder (false) std::vector m_sent_garbage; //!< The garbage we've sent to m_transport. + std::vector m_recv_garbage; //!< The garbage we've received from m_transport. std::vector m_to_send; //!< Bytes we have queued up to send to m_transport. std::vector m_received; //!< Bytes we have received from m_transport. std::deque m_msg_to_send; //!< Messages to be sent *by* m_transport to us. + bool m_sent_aad{false}; public: /** Construct a tester object. test_initiator: whether the tested transport is initiator. */ @@ -1138,8 +1140,7 @@ public: /** Schedule specified garbage to be sent to the transport. */ void SendGarbage(Span garbage) { - // Remember the specified garbage (so we can use it for constructing the garbage - // authentication packet). + // Remember the specified garbage (so we can use it as AAD). m_sent_garbage.assign(garbage.begin(), garbage.end()); // Schedule it for sending. Send(m_sent_garbage); @@ -1198,27 +1199,27 @@ public: Send(ciphertext); } - /** Schedule garbage terminator and authentication packet to be sent to the transport (only - * after ReceiveKey). */ - void SendGarbageTermAuth(size_t garb_auth_data_len = 0, bool garb_auth_ignore = false) + /** Schedule garbage terminator to be sent to the transport (only after ReceiveKey). */ + void SendGarbageTerm() { - // Generate random data to include in the garbage authentication packet (ignored by peer). - auto garb_auth_data = g_insecure_rand_ctx.randbytes(garb_auth_data_len); // Schedule the garbage terminator to be sent. Send(m_cipher.GetSendGarbageTerminator()); - // Schedule the garbage authentication packet to be sent. - SendPacket(/*content=*/garb_auth_data, /*aad=*/m_sent_garbage, /*ignore=*/garb_auth_ignore); } /** Schedule version packet to be sent to the transport (only after ReceiveKey). */ void SendVersion(Span version_data = {}, bool vers_ignore = false) { - SendPacket(/*content=*/version_data, /*aad=*/{}, /*ignore=*/vers_ignore); + Span aad; + // Set AAD to garbage only for first packet. + if (!m_sent_aad) aad = m_sent_garbage; + SendPacket(/*content=*/version_data, /*aad=*/aad, /*ignore=*/vers_ignore); + m_sent_aad = true; } /** Expect a packet to have been received from transport, process it, and return its contents - * (only after ReceiveKey). By default, decoys are skipped. */ - std::vector ReceivePacket(Span aad = {}, bool skip_decoy = true) + * (only after ReceiveKey). Decoys are skipped. Optional associated authenticated data (AAD) is + * expected in the first received packet, no matter if that is a decoy or not. */ + std::vector ReceivePacket(Span aad = {}) { std::vector contents; // Loop as long as there are ignored packets that are to be skipped. @@ -1239,16 +1240,18 @@ public: /*ignore=*/ignore, /*contents=*/MakeWritableByteSpan(contents)); BOOST_CHECK(ret); + // Don't expect AAD in further packets. + aad = {}; // Strip the processed packet's bytes off the front of the receive buffer. m_received.erase(m_received.begin(), m_received.begin() + size + BIP324Cipher::EXPANSION); - // Stop if the ignore bit is not set on this packet, or if we choose to not honor it. - if (!ignore || !skip_decoy) break; + // Stop if the ignore bit is not set on this packet. + if (!ignore) break; } return contents; } - /** Expect garbage, garbage terminator, and garbage auth packet to have been received, and - * process them (only after ReceiveKey). */ + /** Expect garbage and garbage terminator to have been received, and process them (only after + * ReceiveKey). */ void ReceiveGarbage() { // Figure out the garbage length. @@ -1259,18 +1262,15 @@ public: if (term_span == m_cipher.GetReceiveGarbageTerminator()) break; } // Copy the garbage to a buffer. - std::vector garbage(m_received.begin(), m_received.begin() + garblen); + m_recv_garbage.assign(m_received.begin(), m_received.begin() + garblen); // Strip garbage + garbage terminator off the front of the receive buffer. m_received.erase(m_received.begin(), m_received.begin() + garblen + BIP324Cipher::GARBAGE_TERMINATOR_LEN); - // Process the expected garbage authentication packet. Such a packet still functions as one - // even when its ignore bit is set to true, so we do not skip decoy packets here. - ReceivePacket(/*aad=*/MakeByteSpan(garbage), /*skip_decoy=*/false); } /** Expect version packet to have been received, and process it (only after ReceiveKey). */ void ReceiveVersion() { - auto contents = ReceivePacket(); + auto contents = ReceivePacket(/*aad=*/MakeByteSpan(m_recv_garbage)); // Version packets from real BIP324 peers are expected to be empty, despite the fact that // this class supports *sending* non-empty version packets (to test that BIP324 peers // correctly ignore version packet contents). @@ -1347,7 +1347,7 @@ BOOST_AUTO_TEST_CASE(v2transport_test) tester.SendKey(); tester.SendGarbage(); tester.ReceiveKey(); - tester.SendGarbageTermAuth(); + tester.SendGarbageTerm(); tester.SendVersion(); ret = tester.Interact(); BOOST_REQUIRE(ret && ret->empty()); @@ -1387,7 +1387,7 @@ BOOST_AUTO_TEST_CASE(v2transport_test) auto ret = tester.Interact(); BOOST_REQUIRE(ret && ret->empty()); tester.ReceiveKey(); - tester.SendGarbageTermAuth(); + tester.SendGarbageTerm(); tester.SendVersion(); ret = tester.Interact(); BOOST_REQUIRE(ret && ret->empty()); @@ -1415,10 +1415,6 @@ BOOST_AUTO_TEST_CASE(v2transport_test) bool initiator = InsecureRandBool(); /** Use either 0 bytes or the maximum possible (4095 bytes) garbage length. */ size_t garb_len = InsecureRandBool() ? 0 : V2Transport::MAX_GARBAGE_LEN; - /** Sometimes, use non-empty contents in the garbage authentication packet (which is to be ignored). */ - size_t garb_auth_data_len = InsecureRandBool() ? 0 : InsecureRandRange(100000); - /** Whether to set the ignore bit on the garbage authentication packet (it still functions as garbage authentication). */ - bool garb_ignore = InsecureRandBool(); /** How many decoy packets to send before the version packet. */ unsigned num_ignore_version = InsecureRandRange(10); /** What data to send in the version packet (ignored by BIP324 peers, but reserved for future extensions). */ @@ -1439,7 +1435,7 @@ BOOST_AUTO_TEST_CASE(v2transport_test) tester.SendGarbage(garb_len); } tester.ReceiveKey(); - tester.SendGarbageTermAuth(garb_auth_data_len, garb_ignore); + tester.SendGarbageTerm(); for (unsigned v = 0; v < num_ignore_version; ++v) { size_t ver_ign_data_len = InsecureRandBool() ? 0 : InsecureRandRange(1000); auto ver_ign_data = g_insecure_rand_ctx.randbytes(ver_ign_data_len); @@ -1483,7 +1479,7 @@ BOOST_AUTO_TEST_CASE(v2transport_test) tester.SendKey(); tester.SendGarbage(V2Transport::MAX_GARBAGE_LEN + 1); tester.ReceiveKey(); - tester.SendGarbageTermAuth(); + tester.SendGarbageTerm(); ret = tester.Interact(); BOOST_CHECK(!ret); } @@ -1496,7 +1492,7 @@ BOOST_AUTO_TEST_CASE(v2transport_test) auto ret = tester.Interact(); BOOST_REQUIRE(ret && ret->empty()); tester.ReceiveKey(); - tester.SendGarbageTermAuth(); + tester.SendGarbageTerm(); ret = tester.Interact(); BOOST_CHECK(!ret); } @@ -1521,7 +1517,7 @@ BOOST_AUTO_TEST_CASE(v2transport_test) // the first 15 of them match. garbage[len_before + 15] ^= (uint8_t(1) << InsecureRandRange(8)); tester.SendGarbage(garbage); - tester.SendGarbageTermAuth(); + tester.SendGarbageTerm(); tester.SendVersion(); ret = tester.Interact(); BOOST_REQUIRE(ret && ret->empty());