merge bitcoin#28525: Drop v2 garbage authentication packet

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
+    // We cannot wipe m_send_garbage as it will still be used as AAD later in the handshake.
-    // authentication packet.
 }
 
 V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span<const std::byte> ent32, std::vector<uint8_t> 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
+        // Construct version packet in the send buffer, with the sent garbage data as AAD.
-        // 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.
         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.
+            // Garbage terminator received. Store garbage to authenticate it as AAD later.
-            m_recv_garbage = std::move(m_recv_buffer);
+            m_recv_aad = std::move(m_recv_buffer);
-            m_recv_garbage.resize(m_recv_garbage.size() - BIP324Cipher::GARBAGE_TERMINATOR_LEN);
+            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 ||
+    Assume(m_recv_state == RecvState::VERSION || m_recv_state == RecvState::APP);
-           m_recv_state == RecvState::APP);
 
     // The maximum permitted contents length for a packet, consisting of:
     // - 0x00 byte: indicating long message type encoding
@@ -1346,46 +1332,39 @@ 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<const std::byte> 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
+        // At this point we have a valid packet decrypted into m_recv_decode_buffer. If it's not a
-        // the current state, decide what to do with it.
+        // decoy, which we simply ignore, use the current state to 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) {
+            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) {
                 // 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);
             }
+        }
         // Wipe the receive buffer where the next packet will be received into.
         m_recv_buffer = {};
         // In all but APP_READY state, we can wipe the decoded contents.
@@ -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<const uint8_t>& 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<const uint8_t>& msg_bytes) noexcept
             if (!ProcessReceivedGarbageBytes()) return false;
             break;
 
-        case RecvState::GARBAUTH:
         case RecvState::VERSION:
         case RecvState::APP:
             if (!ProcessReceivedPacketBytes()) return false;

src/net.h

@@ -499,7 +499,7 @@ private:
      *        |  start(initiator)                           /---------\
      *        |          |                                  |         |
      *        v          v                                  v         |
-     *  KEY_MAYBE_V1 -> KEY -> GARB_GARBTERM -> GARBAUTH -> VERSION -> APP -> APP_READY
+     *  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,
+         * A packet is received, and decrypted/verified. If that fails, the connection aborts. The
-         * and the decrypted contents is interpreted as version negotiation (currently, that means
+         * first received packet in this state (whether it's a decoy or not) is expected to
-         * ignoring it, but it can be used for negotiating future extensions). If it fails, the
+         * authenticate the garbage received during the GARB_GARBTERM state as associated
-         * connection aborts. */
+         * 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
+         * entered, the garbage terminator and version packet are appended to the send buffer (in
-         * packet are appended to the send buffer (in addition to the key and garbage which may
+         * addition to the key and garbage which may still be there). In this state a message can be
-         * still be there). In this state a message can be provided if the send buffer is empty. */
+         * 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<uint8_t> m_recv_buffer GUARDED_BY(m_recv_mutex);
-    /** During GARBAUTH, the garbage received during GARB_GARBTERM. */
+    /** AAD expected in next received packet (currently used only for garbage). */
-    std::vector<uint8_t> m_recv_garbage GUARDED_BY(m_recv_mutex);
+    std::vector<uint8_t> m_recv_aad GUARDED_BY(m_recv_mutex);
     /** Buffer to put decrypted contents in, for converting to CNetMessage. */
     std::vector<uint8_t> 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:

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<uint8_t> m_sent_garbage; //!< The garbage we've sent to m_transport.
+    std::vector<uint8_t> m_recv_garbage; //!< The garbage we've received from m_transport.
     std::vector<uint8_t> m_to_send; //!< Bytes we have queued up to send to m_transport.
     std::vector<uint8_t> m_received; //!< Bytes we have received from m_transport.
     std::deque<CSerializedNetMsg> 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<const uint8_t> garbage)
     {
-        // Remember the specified garbage (so we can use it for constructing the garbage
+        // Remember the specified garbage (so we can use it as AAD).
-        // authentication packet).
         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
+    /** Schedule garbage terminator to be sent to the transport (only after ReceiveKey). */
-     *  after ReceiveKey). */
+    void SendGarbageTerm()
-    void SendGarbageTermAuth(size_t garb_auth_data_len = 0, bool garb_auth_ignore = false)
     {
-        // Generate random data to include in the garbage authentication packet (ignored by peer).
-        auto garb_auth_data = g_insecure_rand_ctx.randbytes<uint8_t>(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<const uint8_t> version_data = {}, bool vers_ignore = false)
     {
-        SendPacket(/*content=*/version_data, /*aad=*/{}, /*ignore=*/vers_ignore);
+        Span<const std::uint8_t> 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. */
+     *  (only after ReceiveKey). Decoys are skipped. Optional associated authenticated data (AAD) is
-    std::vector<uint8_t> ReceivePacket(Span<const std::byte> aad = {}, bool skip_decoy = true)
+     *  expected in the first received packet, no matter if that is a decoy or not. */
+    std::vector<uint8_t> ReceivePacket(Span<const std::byte> aad = {})
     {
         std::vector<uint8_t> 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.
+            // Stop if the ignore bit is not set on this packet.
-            if (!ignore || !skip_decoy) break;
+            if (!ignore) break;
         }
         return contents;
     }
 
-    /** Expect garbage, garbage terminator, and garbage auth packet to have been received, and
+    /** Expect garbage and garbage terminator to have been received, and process them (only after
-     *  process them (only after ReceiveKey). */
+     *  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<uint8_t> 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<uint8_t>(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());