aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAndrew Chow <github@achow101.com>2023-09-29 09:43:53 -0400
committerAndrew Chow <github@achow101.com>2023-09-29 09:50:02 -0400
commitd18a8f6f6969487021b8fd50391a2a8d1dc29844 (patch)
tree25993877d3ca369cc0dde1020eb86d4b95944198
parent9d5150ac47a2e8db0f23a2f04211fec3516afbe1 (diff)
parente3720bca398820038b3e97f467adb2c45ef9ef5f (diff)
Merge bitcoin/bitcoin#28525: net: Drop v2 garbage authentication packet
e3720bca398820038b3e97f467adb2c45ef9ef5f net: Simplify v2 recv logic by decoupling AAD from state machine (Tim Ruffing) b0f5175c044df956c0f07f540706d457c4912856 net: Drop v2 garbage authentication packet (Tim Ruffing) Pull request description: Note that this is a breaking change, see also https://github.com/bitcoin/bips/pull/1498 The benefit is a simpler implementation: - The protocol state machine does not need separate states for garbage authentication and version phases. - The special case of "ignoring the ignore bit" is removed. - The freedom to choose the contents of the garbage authentication packet is removed. This simplifies testing. ACKs for top commit: naumenkogs: ACK e3720bca398820038b3e97f467adb2c45ef9ef5f sipa: ACK e3720bca398820038b3e97f467adb2c45ef9ef5f. Re-ran the v2 transport fuzzer overnight. ajtowns: ACK e3720bca398820038b3e97f467adb2c45ef9ef5f - simpler and more flexible, nice achow101: ACK e3720bca398820038b3e97f467adb2c45ef9ef5f Sjors: utACK e3720bca398820038b3e97f467adb2c45ef9ef5f theStack: Code-review ACK e3720bca398820038b3e97f467adb2c45ef9ef5f Tree-SHA512: 16600ed868c8a346828de075c4072e37cf86440751d08ab099fe8b58ff71d8b371a90397d6a4247096796db68794275e7e0403f218859567d04838e0411dadd6
-rw-r--r--src/net.cpp73
-rw-r--r--src/net.h41
-rw-r--r--src/test/net_tests.cpp58
3 files changed, 69 insertions, 103 deletions
diff --git a/src/net.cpp b/src/net.cpp
index 0c6dac572d..c540b2f7ae 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1002,8 +1002,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
- // authentication packet.
+ // We cannot wipe m_send_garbage as it will still be used as AAD later in the handshake.
}
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 :
@@ -1037,9 +1036,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:
@@ -1171,24 +1167,15 @@ 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
- // 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.
- ClearShrink(m_send_garbage);
-
- // Construct version packet in the send buffer.
+ // Construct version packet in the send buffer, with the sent garbage data as AAD.
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.
+ ClearShrink(m_send_garbage);
} else {
// We still have to receive more key bytes.
}
@@ -1202,11 +1189,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.
- m_recv_garbage = std::move(m_recv_buffer);
- m_recv_garbage.resize(m_recv_garbage.size() - BIP324Cipher::GARBAGE_TERMINATOR_LEN);
+ // Garbage terminator received. Store garbage to authenticate it as AAD later.
+ 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::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.
@@ -1225,8 +1212,7 @@ bool V2Transport::ProcessReceivedGarbageBytes() noexcept
bool V2Transport::ProcessReceivedPacketBytes() noexcept
{
AssertLockHeld(m_recv_mutex);
- Assume(m_recv_state == RecvState::GARBAUTH || m_recv_state == RecvState::VERSION ||
- m_recv_state == RecvState::APP);
+ Assume(m_recv_state == RecvState::VERSION || m_recv_state == RecvState::APP);
// The maximum permitted contents length for a packet, consisting of:
// - 0x00 byte: indicating long message type encoding
@@ -1249,45 +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::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.
+ 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::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);
- ClearShrink(m_recv_garbage);
- break;
- 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);
- }
- 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);
@@ -1323,7 +1301,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,
@@ -1377,7 +1354,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
@@ -1421,7 +1397,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;
diff --git a/src/net.h b/src/net.h
index 5ab693876c..035cca2b13 100644
--- a/src/net.h
+++ b/src/net.h
@@ -460,10 +460,10 @@ private:
*
* start(responder)
* |
- * | start(initiator) /---------\
- * | | | |
- * v v v |
- * KEY_MAYBE_V1 -> KEY -> GARB_GARBTERM -> GARBAUTH -> VERSION -> APP -> APP_READY
+ * | start(initiator) /---------\
+ * | | | |
+ * v v v |
+ * KEY_MAYBE_V1 -> KEY -> GARB_GARBTERM -> VERSION -> APP -> APP_READY
* |
* \-------> V1
*/
@@ -485,24 +485,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,
- * and the decrypted contents is interpreted as version negotiation (currently, that means
- * ignoring it, but it can be used for negotiating future extensions). If it fails, the
- * connection aborts. */
+ * A packet is received, and decrypted/verified. If that fails, the connection aborts. The
+ * first received packet in this state (whether it's a decoy or not) is expected to
+ * authenticate the garbage received during the GARB_GARBTERM state as associated
+ * 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.
@@ -556,9 +551,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
- * 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. */
+ * entered, the garbage terminator 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.
@@ -578,13 +573,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. */
- 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. */
@@ -624,7 +619,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:
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 1e9c9d923b..1df08127ad 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -1031,9 +1031,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. */
@@ -1131,8 +1133,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
- // authentication packet).
+ // Remember the specified garbage (so we can use it as AAD).
m_sent_garbage.assign(garbage.begin(), garbage.end());
// Schedule it for sending.
Send(m_sent_garbage);
@@ -1191,27 +1192,27 @@ public:
Send(ciphertext);
}
- /** Schedule garbage terminator and authentication packet to be sent to the transport (only
- * after ReceiveKey). */
- void SendGarbageTermAuth(size_t garb_auth_data_len = 0, bool garb_auth_ignore = false)
+ /** Schedule garbage terminator to be sent to the transport (only after ReceiveKey). */
+ void SendGarbageTerm()
{
- // 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. */
- std::vector<uint8_t> ReceivePacket(Span<const std::byte> aad = {}, bool skip_decoy = true)
+ * (only after ReceiveKey). Decoys are skipped. Optional associated authenticated data (AAD) is
+ * 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.
@@ -1232,16 +1233,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.
- if (!ignore || !skip_decoy) break;
+ // Stop if the ignore bit is not set on this packet.
+ if (!ignore) break;
}
return contents;
}
- /** Expect garbage, garbage terminator, and garbage auth packet to have been received, and
- * process them (only after ReceiveKey). */
+ /** Expect garbage and garbage terminator to have been received, and process them (only after
+ * ReceiveKey). */
void ReceiveGarbage()
{
// Figure out the garbage length.
@@ -1252,18 +1255,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).
@@ -1340,7 +1340,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());
@@ -1380,7 +1380,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());
@@ -1408,10 +1408,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). */
@@ -1432,7 +1428,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);
@@ -1476,7 +1472,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);
}
@@ -1489,7 +1485,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);
}
@@ -1514,7 +1510,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());