Skip to content

Commit

Permalink
net: Drop v2 garbage authentication packet
Browse files Browse the repository at this point in the history 
See also bitcoin/bips#1498

The benefit is a simpler implementation:
 - The protocol state machine does 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.
  • Loading branch information
@real-or-random
real-or-random committed Sep 24, 2023
1 parent ac9fa6e commit 796d21c
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 74 deletions.
42 changes: 12 additions & 30 deletions src/net.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1037,9 +1037,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:
Expand Down Expand Up @@ -1171,24 +1168,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.
}
Expand All @@ -1202,11 +1190,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.
// 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_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.
Expand All @@ -1225,8 +1213,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
Expand All @@ -1250,7 +1237,7 @@ bool V2Transport::ProcessReceivedPacketBytes() noexcept
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);
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,
Expand All @@ -1266,18 +1253,16 @@ bool V2Transport::ProcessReceivedPacketBytes() noexcept
// 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) {
// 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) {
Expand Down Expand Up @@ -1323,7 +1308,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,
Expand Down Expand Up @@ -1377,7 +1361,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
Expand Down Expand Up @@ -1421,7 +1404,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;
Expand Down
33 changes: 14 additions & 19 deletions src/net.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,10 +460,10 @@ class V2Transport final : public Transport
*
* 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
*/
Expand All @@ -485,24 +485,19 @@ class V2Transport final : public Transport
/** 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. 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 AAD. If decryption/verification (of the first non-decoy
* decoy) 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. */
VERSION,

/** Application packet.
Expand Down Expand Up @@ -578,12 +573,12 @@ class V2Transport final : public Transport

/** 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. */
/** During VERSION, the garbage received during GARB_GARBTERM. */
std::vector<uint8_t> m_recv_garbage 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);
Expand Down Expand Up @@ -624,7 +619,7 @@ class V2Transport final : public Transport
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:
Expand Down
48 changes: 23 additions & 25 deletions src/test/net_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -1193,25 +1195,26 @@ class V2TransportTester

/** 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)
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;
// Include AAD only in first version 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 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.
Expand All @@ -1232,10 +1235,12 @@ class V2TransportTester
/*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;
}
Expand All @@ -1252,18 +1257,15 @@ class V2TransportTester
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).
Expand Down Expand Up @@ -1340,7 +1342,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());
Expand Down Expand Up @@ -1380,7 +1382,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());
Expand Down Expand Up @@ -1408,10 +1410,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). */
Expand All @@ -1432,7 +1430,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);
Expand Down Expand Up @@ -1476,7 +1474,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);
}
Expand All @@ -1489,7 +1487,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);
}
Expand All @@ -1514,7 +1512,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());
Expand Down

0 comments on commit 796d21c

Please sign in to comment.