net: do not use send buffer to store/cache garbage

Before this commit the V2Transport::m_send_buffer is used to store the
garbage:
* During MAYBE_V1 state, it's there despite not being sent.
* During AWAITING_KEY state, while it is being sent.
* At the end of the AWAITING_KEY state it cannot be wiped as it's still
  needed to compute the garbage authentication packet.

Change this by introducing a separate m_send_garbage field, taking over
the first and last role listed above. This means the garbage is only in
the send buffer when it's actually being sent, removing a few special
cases related to this.

Author: sipa

src/net.cpp

@@ -997,17 +997,32 @@ std::vector<uint8_t> GenerateRandomGarbage() noexcept
 
 } // namespace
 
-V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span<const std::byte> ent32, Span<const uint8_t> garbage) noexcept :
+void V2Transport::StartSendingHandshake() noexcept
+{
+    AssertLockHeld(m_send_mutex);
+    Assume(m_send_state == SendState::AWAITING_KEY);
+    Assume(m_send_buffer.empty());
+    // Initialize the send buffer with ellswift pubkey + provided garbage.
+    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.
+}
+
+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 :
     m_cipher{key, ent32}, m_initiating{initiating}, m_nodeid{nodeid},
     m_v1_fallback{nodeid, type_in, version_in}, m_recv_type{type_in}, m_recv_version{version_in},
     m_recv_state{initiating ? RecvState::KEY : RecvState::KEY_MAYBE_V1},
+    m_send_garbage{std::move(garbage)},
     m_send_state{initiating ? SendState::AWAITING_KEY : SendState::MAYBE_V1}
 {
-    assert(garbage.size() <= MAX_GARBAGE_LEN);
-    // Initialize the send buffer with ellswift pubkey + provided garbage.
-    m_send_buffer.resize(EllSwiftPubKey::size() + garbage.size());
-    std::copy(std::begin(m_cipher.GetOurPubKey()), std::end(m_cipher.GetOurPubKey()), MakeWritableByteSpan(m_send_buffer).begin());
-    std::copy(garbage.begin(), garbage.end(), m_send_buffer.begin() + EllSwiftPubKey::size());
+    Assume(m_send_garbage.size() <= MAX_GARBAGE_LEN);
+    // Start sending immediately if we're the initiator of the connection.
+    if (initiating) {
+        LOCK(m_send_mutex);
+        StartSendingHandshake();
+    }
 }
 
 V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept :
@@ -1092,9 +1107,10 @@ void V2Transport::ProcessReceivedMaybeV1Bytes() noexcept
     if (!std::equal(m_recv_buffer.begin(), m_recv_buffer.end(), v1_prefix.begin())) {
         // Mismatch with v1 prefix, so we can assume a v2 connection.
         SetReceiveState(RecvState::KEY); // Convert to KEY state, leaving received bytes around.
-        // Transition the sender to AWAITING_KEY state (if not already).
+        // Transition the sender to AWAITING_KEY state and start sending.
         LOCK(m_send_mutex);
         SetSendState(SendState::AWAITING_KEY);
+        StartSendingHandshake();
     } else if (m_recv_buffer.size() == v1_prefix.size()) {
         // Full match with the v1 prefix, so fall back to v1 behavior.
         LOCK(m_send_mutex);
@@ -1154,7 +1170,6 @@ bool V2Transport::ProcessReceivedKeyBytes() noexcept
         SetSendState(SendState::READY);
 
         // Append the garbage terminator to the send buffer.
-        size_t garbage_len = m_send_buffer.size() - EllSwiftPubKey::size();
         m_send_buffer.resize(m_send_buffer.size() + BIP324Cipher::GARBAGE_TERMINATOR_LEN);
         std::copy(m_cipher.GetSendGarbageTerminator().begin(),
                   m_cipher.GetSendGarbageTerminator().end(),
@@ -1165,9 +1180,12 @@ bool V2Transport::ProcessReceivedKeyBytes() noexcept
         m_send_buffer.resize(m_send_buffer.size() + BIP324Cipher::EXPANSION);
         m_cipher.Encrypt(
             /*contents=*/{},
-            /*aad=*/MakeByteSpan(m_send_buffer).subspan(EllSwiftPubKey::size(), garbage_len),
+            /*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());
@@ -1537,9 +1555,7 @@ Transport::BytesToSend V2Transport::GetBytesToSend(bool have_next_message) const
     LOCK(m_send_mutex);
     if (m_send_state == SendState::V1) return m_v1_fallback.GetBytesToSend(have_next_message);
 
-    // We do not send anything in MAYBE_V1 state (as we don't know if the peer is v1 or v2),
-    // despite there being data in the send buffer in that state.
-    if (m_send_state == SendState::MAYBE_V1) return {{}, false, m_send_type};
+    if (m_send_state == SendState::MAYBE_V1) Assume(m_send_buffer.empty());
     Assume(m_send_pos <= m_send_buffer.size());
     return {
         Span{m_send_buffer}.subspan(m_send_pos),
@@ -1558,10 +1574,8 @@ void V2Transport::MarkBytesSent(size_t bytes_sent) noexcept
 
     m_send_pos += bytes_sent;
     Assume(m_send_pos <= m_send_buffer.size());
-    // Only wipe the buffer when everything is sent in the READY state. In the AWAITING_KEY state
-    // we still need the garbage that's in the send buffer to construct the garbage authentication
-    // packet.
-    if (m_send_state == SendState::READY && m_send_pos == m_send_buffer.size()) {
+    // Wipe the buffer when everything is sent.
+    if (m_send_pos == m_send_buffer.size()) {
         m_send_pos = 0;
         m_send_buffer = {};
     }

src/net.h

@@ -540,8 +540,8 @@ class V2Transport final : public Transport
     enum class SendState : uint8_t {
         /** (Responder only) Not sending until v1 or v2 is detected.
          *
-         * This is the initial state for responders. The send buffer contains the public key to
-         * send, but nothing is sent in this state yet. When the receiver determines whether this
+         * This is the initial state for responders. The send buffer is empty.
+         * When the receiver determines whether this
          * is a V1 or V2 connection, the sender state becomes AWAITING_KEY (for v2) or V1 (for v1).
          */
         MAYBE_V1,
@@ -601,6 +601,8 @@ class V2Transport final : public Transport
     std::vector<uint8_t> m_send_buffer GUARDED_BY(m_send_mutex);
     /** How many bytes from the send buffer have been sent so far. */
     uint32_t m_send_pos GUARDED_BY(m_send_mutex) {0};
+    /** The garbage sent, or to be sent (MAYBE_V1 and AWAITING_KEY state only). */
+    std::vector<uint8_t> m_send_garbage GUARDED_BY(m_send_mutex);
     /** Type of the message being sent. */
     std::string m_send_type GUARDED_BY(m_send_mutex);
     /** Current sender state. */
@@ -614,6 +616,8 @@ class V2Transport final : public Transport
     static std::optional<std::string> GetMessageType(Span<const uint8_t>& contents) noexcept;
     /** Determine how many received bytes can be processed in one go (not allowed in V1 state). */
     size_t GetMaxBytesToProcess() noexcept EXCLUSIVE_LOCKS_REQUIRED(m_recv_mutex);
+    /** Put our public key + garbage in the send buffer. */
+    void StartSendingHandshake() noexcept EXCLUSIVE_LOCKS_REQUIRED(m_send_mutex);
     /** Process bytes in m_recv_buffer, while in KEY_MAYBE_V1 state. */
     void ProcessReceivedMaybeV1Bytes() noexcept EXCLUSIVE_LOCKS_REQUIRED(m_recv_mutex, !m_send_mutex);
     /** Process bytes in m_recv_buffer, while in KEY state. */
@@ -636,7 +640,7 @@ class V2Transport final : public Transport
     V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept;
 
     /** Construct a V2 transport with specified keys and garbage (test use only). */
-    V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span<const std::byte> ent32, Span<const uint8_t> garbage) noexcept;
+    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;
 
     // Receive side functions.
     bool ReceivedMessageComplete() const noexcept override EXCLUSIVE_LOCKS_REQUIRED(!m_recv_mutex);

src/test/fuzz/p2p_transport_serialization.cpp

@@ -366,7 +366,7 @@ std::unique_ptr<Transport> MakeV2Transport(NodeId nodeid, bool initiator, RNG& r
              .Write(garb.data(), garb.size())
              .Finalize(UCharCast(ent.data()));
 
-    return std::make_unique<V2Transport>(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent, garb);
+    return std::make_unique<V2Transport>(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent, std::move(garb));
 }
 
 } // namespace