From e3720bca398820038b3e97f467adb2c45ef9ef5f Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Tue, 26 Sep 2023 13:26:27 +0200
Subject: [PATCH] net: Simplify v2 recv logic by decoupling AAD from state
 machine

---
 src/net.cpp | 38 ++++++++++++++++----------------------
 src/net.h   |  4 ++--
 2 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/src/net.cpp b/src/net.cpp
index 93b7bf40736..c540b2f7ae9 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1190,8 +1190,8 @@ bool V2Transport::ProcessReceivedGarbageBytes() noexcept
     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. Store garbage to authenticate it as AAD later.
-            m_recv_garbage = std::move(m_recv_buffer);
-            m_recv_garbage.resize(m_recv_garbage.size() - BIP324Cipher::GARBAGE_TERMINATOR_LEN);
+            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::VERSION);
         } else if (m_recv_buffer.size() == MAX_GARBAGE_LEN + BIP324Cipher::GARBAGE_TERMINATOR_LEN) {
@@ -1235,43 +1235,37 @@ 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::VERSION) 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.
+        ClearShrink(m_recv_aad);
         // 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::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);
-            }
-            // We have decrypted one valid packet (which may or may not have been a decoy) with the
-            // received garbage as AAD. We no longer need the received garbage and further packets
-            // are expected to use the empty string as AAD.
-            ClearShrink(m_recv_garbage);
-            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.
         ClearShrink(m_recv_buffer);
diff --git a/src/net.h b/src/net.h
index 5f99cd667a2..035cca2b13d 100644
--- a/src/net.h
+++ b/src/net.h
@@ -578,8 +578,8 @@ private:
     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 VERSION, the garbage received during GARB_GARBTERM. */
-    std::vector<uint8_t> m_recv_garbage GUARDED_BY(m_recv_mutex);
+    /** AAD expected in next received packet (currently used only for garbage). */
+    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. */