From b6934fd03f080d437acb1fd2b665503c3d6de785 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 8 Sep 2023 11:48:09 -0400 Subject: [PATCH 1/3] net: merge V2Transport constructors, move key gen This removes the ability for BIP324Cipher to generate its own key, moving that responsibility to the caller (mostly, V2Transport). This allows us to write the random-key V2Transport constructor by delegating to the explicit-key one. --- src/bip324.cpp | 7 ------- src/bip324.h | 4 ++-- src/net.cpp | 29 +++++++++++++++++------------ src/test/net_tests.cpp | 9 +++++++++ 4 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/bip324.cpp b/src/bip324.cpp index 314e756829f..f579a25193a 100644 --- a/src/bip324.cpp +++ b/src/bip324.cpp @@ -22,13 +22,6 @@ #include #include -BIP324Cipher::BIP324Cipher() noexcept -{ - m_key.MakeNewKey(true); - uint256 entropy = GetRandHash(); - m_our_pubkey = m_key.EllSwiftCreate(MakeByteSpan(entropy)); -} - BIP324Cipher::BIP324Cipher(const CKey& key, Span ent32) noexcept : m_key(key) { diff --git a/src/bip324.h b/src/bip324.h index 0238c479c08..28e7c411eaa 100644 --- a/src/bip324.h +++ b/src/bip324.h @@ -41,8 +41,8 @@ class BIP324Cipher std::array m_recv_garbage_terminator; public: - /** Initialize a BIP324 cipher with securely generated random keys. */ - BIP324Cipher() noexcept; + /** No default constructor; keys must be provided to create a BIP324Cipher. */ + BIP324Cipher() = delete; /** Initialize a BIP324 cipher with specified key and encoding entropy (testing only). */ BIP324Cipher(const CKey& key, Span ent32) noexcept; diff --git a/src/net.cpp b/src/net.cpp index 3955005dfa6..98ca7c2bed3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -979,23 +979,24 @@ class V2MessageMap const V2MessageMap V2_MESSAGE_MAP; -} // namespace +CKey GenerateRandomKey() noexcept +{ + CKey key; + key.MakeNewKey(/*fCompressed=*/true); + return key; +} -V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept : - m_cipher{}, 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} +std::vector GenerateRandomGarbage() noexcept { - // Construct garbage (including its length) using a FastRandomContext. + std::vector ret; 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())); + ret.resize(rng.randrange(V2Transport::MAX_GARBAGE_LEN + 1)); + rng.fillrand(MakeWritableByteSpan(ret)); + return ret; } +} // namespace + V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span ent32, Span 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}, @@ -1009,6 +1010,10 @@ V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int versio std::copy(garbage.begin(), garbage.end(), m_send_buffer.begin() + EllSwiftPubKey::size()); } +V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept : + V2Transport{nodeid, initiating, type_in, version_in, GenerateRandomKey(), + MakeByteSpan(GetRandHash()), GenerateRandomGarbage()} { } + void V2Transport::SetReceiveState(RecvState recv_state) noexcept { AssertLockHeld(m_recv_mutex); diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 900e311d225..eac8e8146ae 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -1008,6 +1008,14 @@ BOOST_AUTO_TEST_CASE(advertise_local_address) namespace { +CKey GenerateRandomTestKey() noexcept +{ + CKey key; + uint256 key_data = InsecureRand256(); + key.Set(key_data.begin(), key_data.end(), true); + return key; +} + /** A class for scenario-based tests of V2Transport * * Each V2TransportTester encapsulates a V2Transport (the one being tested), and can be told to @@ -1031,6 +1039,7 @@ class V2TransportTester /** Construct a tester object. test_initiator: whether the tested transport is initiator. */ V2TransportTester(bool test_initiator) : m_transport(0, test_initiator, SER_NETWORK, INIT_PROTO_VERSION), + m_cipher{GenerateRandomTestKey(), MakeByteSpan(InsecureRand256())}, m_test_initiator(test_initiator) {} /** Data type returned by Interact: From 9bde93df2c84b6d5f333aa56cbd0b28b6ad337b0 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 8 Sep 2023 13:55:47 -0400 Subject: [PATCH 2/3] 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. --- src/net.cpp | 46 ++++++++++++------- src/net.h | 10 ++-- src/test/fuzz/p2p_transport_serialization.cpp | 2 +- 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 98ca7c2bed3..af2855932de 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -997,17 +997,32 @@ std::vector GenerateRandomGarbage() noexcept } // namespace -V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span ent32, Span 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 ent32, std::vector 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 = {}; } diff --git a/src/net.h b/src/net.h index cf7a240202e..81fbbfd195b 100644 --- a/src/net.h +++ b/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 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 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 GetMessageType(Span& 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 ent32, Span garbage) noexcept; + V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span ent32, std::vector garbage) noexcept; // Receive side functions. bool ReceivedMessageComplete() const noexcept override EXCLUSIVE_LOCKS_REQUIRED(!m_recv_mutex); diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 6e3ef2a707b..88d6e96eacb 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -366,7 +366,7 @@ std::unique_ptr MakeV2Transport(NodeId nodeid, bool initiator, RNG& r .Write(garb.data(), garb.size()) .Finalize(UCharCast(ent.data())); - return std::make_unique(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent, garb); + return std::make_unique(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent, std::move(garb)); } } // namespace From 64704386b28ce3a1ab607a946ec729286da8faa6 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 8 Sep 2023 14:45:56 -0400 Subject: [PATCH 3/3] doc: fix typos and mistakes in BIP324 code comments --- src/net.h | 12 ++++++------ src/test/net_tests.cpp | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/net.h b/src/net.h index 81fbbfd195b..e1d8995a8eb 100644 --- a/src/net.h +++ b/src/net.h @@ -548,17 +548,17 @@ class V2Transport final : public Transport /** Waiting for the other side's public key. * - * This is the initial state for initiators. The public key is sent out. When the receiver - * receives the other side's public key and transitions to GARB_GARBTERM, the sender state - * becomes READY. */ + * This is the initial state for initiators. The public key and garbage is sent out. When + * the receiver receives the other side's public key and transitions to GARB_GARBTERM, the + * sender state becomes READY. */ AWAITING_KEY, /** Normal sending state. * * In this state, the ciphers are initialized, so packets can be sent. When this state is - * 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. */ + * 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. */ READY, /** This transport is using v1 fallback. diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index eac8e8146ae..3eb7bdec62f 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -1021,7 +1021,7 @@ CKey GenerateRandomTestKey() noexcept * Each V2TransportTester encapsulates a V2Transport (the one being tested), and can be told to * interact with it. To do so, it also encapsulates a BIP324Cipher to act as the other side. A * second V2Transport is not used, as doing so would not permit scenarios that involve sending - * invalid data, or ones scenarios using BIP324 features that are not implemented on the sending + * invalid data, or ones using BIP324 features that are not implemented on the sending * side (like decoy packets). */ class V2TransportTester