net: make V2Transport send uniformly random number garbage bytes

Author: sipa

src/net.cpp

@@ -987,20 +987,26 @@ V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int versio
     m_recv_state{initiating ? RecvState::KEY : RecvState::KEY_MAYBE_V1},
     m_send_state{initiating ? SendState::AWAITING_KEY : SendState::MAYBE_V1}
 {
-    // Initialize the send buffer with ellswift pubkey.
-    m_send_buffer.resize(EllSwiftPubKey::size());
+    // Construct garbage (including its length) using a FastRandomContext.
+    FastRandomContext rng;
+    size_t garbage_len = rng.randrange(MAX_GARBAGE_LEN + 1);
+    // Initialize the send buffer with ellswift pubkey + garbage.
+    m_send_buffer.resize(EllSwiftPubKey::size() + garbage_len);
     std::copy(std::begin(m_cipher.GetOurPubKey()), std::end(m_cipher.GetOurPubKey()), MakeWritableByteSpan(m_send_buffer).begin());
+    rng.fillrand(MakeWritableByteSpan(m_send_buffer).subspan(EllSwiftPubKey::size()));
 }
 
-V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span<const std::byte> ent32) noexcept :
+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 :
     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_state{initiating ? SendState::AWAITING_KEY : SendState::MAYBE_V1}
 {
-    // Initialize the send buffer with ellswift pubkey.
-    m_send_buffer.resize(EllSwiftPubKey::size());
+    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());
 }
 
 void V2Transport::SetReceiveState(RecvState recv_state) noexcept
@@ -1126,16 +1132,18 @@ void 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(),
                   MakeWritableByteSpan(m_send_buffer).last(BIP324Cipher::GARBAGE_TERMINATOR_LEN).begin());
 
-        // Construct garbage authentication packet in the send buffer.
+        // 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=*/{}, /* empty garbage for now */
+            /*aad=*/MakeByteSpan(m_send_buffer).subspan(EllSwiftPubKey::size(), garbage_len),
             /*ignore=*/false,
             /*output=*/MakeWritableByteSpan(m_send_buffer).last(BIP324Cipher::EXPANSION));
 
@@ -1490,7 +1498,10 @@ void V2Transport::MarkBytesSent(size_t bytes_sent) noexcept
 
     m_send_pos += bytes_sent;
     Assume(m_send_pos <= m_send_buffer.size());
-    if (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()) {
         m_send_pos = 0;
         m_send_buffer = {};
     }

src/net.h

@@ -556,9 +556,9 @@ class V2Transport final : public Transport
         /** 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 which may still be there). In this
-         * state a message can be provided if the send buffer is empty. */
+         * entered, the garbage, garbage terminator, garbage authentication packet, and version
+         * packet are appended to the send buffer (in addition to the key 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.
@@ -635,8 +635,8 @@ class V2Transport final : public Transport
      */
     V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept;
 
-    /** Construct a V2 transport with specified keys (test use only). */
-    V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span<const std::byte> ent32) 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;
 
     // Receive side functions.
     bool ReceivedMessageComplete() const noexcept override EXCLUSIVE_LOCKS_REQUIRED(!m_recv_mutex);

src/test/fuzz/p2p_transport_serialization.cpp

@@ -341,11 +341,32 @@ std::unique_ptr<Transport> MakeV2Transport(NodeId nodeid, bool initiator, RNG& r
     // Retrieve key
     auto key = ConsumePrivateKey(provider);
     if (!key.IsValid()) return {};
+    // Construct garbage
+    size_t garb_len = provider.ConsumeIntegralInRange<size_t>(0, V2Transport::MAX_GARBAGE_LEN);
+    std::vector<uint8_t> garb;
+    if (garb_len <= 64) {
+        // When the garbage length is up to 64 bytes, read it directly from the fuzzer input.
+        garb = provider.ConsumeBytes<uint8_t>(garb_len);
+        garb.resize(garb_len);
+    } else {
+        // If it's longer, generate it from the RNG. This avoids having large amounts of
+        // (hopefully) irrelevant data needing to be stored in the fuzzer data.
+        for (auto& v : garb) v = uint8_t(rng());
+    }
     // Retrieve entropy
     auto ent = provider.ConsumeBytes<std::byte>(32);
     ent.resize(32);
-
-    return std::make_unique<V2Transport>(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent);
+    // Use as entropy SHA256(ent || garbage). This prevents a situation where the fuzzer manages to
+    // include the garbage terminator (which is a function of both ellswift keys) in the garbage.
+    // This is extremely unlikely (~2^-116) with random keys/garbage, but the fuzzer can choose
+    // both non-randomly and dependently. Since the entropy is hashed anyway inside the ellswift
+    // computation, no coverage should be lost by using a hash as entropy, and it removes the
+    // possibility of garbage that happens to contain what is effectively a hash of the keys.
+    CSHA256().Write(UCharCast(ent.data()), ent.size())
+             .Write(garb.data(), garb.size())
+             .Finalize(UCharCast(ent.data()));
+
+    return std::make_unique<V2Transport>(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent, garb);
 }
 
 } // namespace