BIP324: Add encrypted p2p transport {de}serializer #23233

dhruv · 2021-10-08T19:01:33Z

Revives #18242. Depends on #25361 (please review that first, the last 4 commits are to be reviewed here).

This PR adds a p2p message transport {de}serializer for encrypted packets leveraging the BIP324 specification.

The dependency tree for BIP324 PRs is here.

dhruv · 2021-10-08T22:45:38Z

Rebased and lint errors fixed. Ready for review.

maflcko · 2021-10-09T06:28:27Z

src/test/fuzz/crypto_chacha20_poly1305_aead.cpp

+                uint32_t len = aead.DecryptLength(in.data());
+                len = 0; // addressing the [[nodiscard]] and otherwise unused variable


Suggested change

uint32_t len = aead.DecryptLength(in.data());

len = 0; // addressing the [[nodiscard]] and otherwise unused variable

(void)aead.DecryptLength(in.data());

Does this not work?

That does indeed work. Thank you.

I've updated the commit in PR #20962 (where it originally belongs) and also here(this PR is based off that one).

DrahtBot · 2021-10-09T19:00:13Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	stratospher

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27534 (rpc: add 'getnetmsgstats', new rpc to view network message statistics by satsie)
#27530 (Remove now-unnecessary poll, fcntl includes from net(base).cpp by Empact)
#27479 (BIP324: ElligatorSwift integrations by sipa)
#27411 (p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting by mzumsande)
#27407 (net, refactor: Privatise CNode send queue by dergoegge)
#27385 (net, refactor: extract Network and BIP155Network logic to node/network by jonatack)
#26684 (bench: add readblock benchmark by andrewtoth)
#26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25268 (refactor: Introduce EvictionManager by dergoegge)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

dhruv · 2021-10-10T04:51:32Z

Addressed #23233 (comment) - ready for further review.

dhruv · 2021-10-22T18:24:42Z

Rebased. Ready for further review.

stratospher

Concept ACK ab5b49d. This PR nicely implements the v2 transport serialisation and deserialisation process.

4be7638
- To introduce short-IDs, allNetMessageTypes[] is represented as a map and functions to convert short-ID to a message and vice versa are added. Verified that changes don’t need to be made to other files due to this commit.
a53a06c
- The v2TransportDeserializer contains:
  - readHeader() : copies the header from msg_bytes to location in vRecv[] which is addressed by m_hdr_pos. After the entire header is read, it switches to read message data mode(m_in_data = true).
  - readData() : copies the data from msg_bytes to location in vRecv[] which is addressed by CHACHA20_POLY1305_AEAD_AAD_LEN + m_data_pos. vRecv[] is resized to at max 256 KiB forward if there’s no space to hold msg_bytes.
  - GetMessage() : performs decryption of vRecv[] and chops off the AAD and MAC tag if successful decryption occurs. It reads the first byte of the payload in size_or_shortid. If size_or_shortid is a number between 1 and 12, then it’s a string command and the next size_or_shortid bytes is read into command_name. If size_or_shortid is larger than 12, then it’s a Message-Type-ID and if it's string mapping is found, it's stored in command_name. Otherwise the size_or_shortid is appended with the prefix ‘unknown’ and stored in command_name. CNetMessage msg is constructed and returned.
- The v2TransportSerializer contains:
  - prepareForTransport() : prepares the CSerializedNetMsg msg for serialisation. serialized_command_size is used to store the size of short id (1 byte) or the size of message command string if short id isn’t assigned. packet_length contains the length of the message type id and payload which is stored in a LE representation in serialized_header. The short id/ command string is also appended to it. serialized_header is then inserted into the beginning of msg buffer. Encryption is performed on msg and returns true when successful.

stratospher · 2021-11-05T04:44:41Z

src/net.cpp

+    }
+
+    // we got the AAD bytes at this point (3 bytes encrypted packet length)
+    // we keep the sequence numbers unchanged at this point. Once the message is authenticated and decrypted, we increase the sequence numbers (or the aad_pos)


Suggested change

// we keep the sequence numbers unchanged at this point. Once the message is authenticated and decrypted, we increase the sequence numbers (or the aad_pos)

This comment can be removed since it's related to the AEAD cipher suite implementation and doesn't play a role here.

Good find. Removed.

stratospher · 2021-11-05T04:51:46Z

src/net.cpp

+    m_message_size = m_aead->DecryptLength((const uint8_t*)vRecv.data());
+
+    // reject messages larger than MAX_SIZE
+    if (m_message_size > MAX_SIZE) {


This will need to be updated to be consistent with the BIP since MAX_SIZE is 2^25 bytes.

Thanks for the catch. Fixed.

stratospher · 2021-11-05T05:33:17Z

src/net.cpp

+    return copy_bytes;
+}
+
+std::optional<CNetMessage> V2TransportDeserializer::GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size)


This function can be modified to keep it's format consistent with the v1 GetMessage since #22735 got merged.

Done. Rebased with master.

stratospher · 2021-11-05T05:34:47Z

src/net.cpp

+        // encode as varstr, max 12 chars
+        serialized_command_size = ::GetSerializeSize(msg.m_type, PROTOCOL_VERSION);
+    }
+    // prepare the packet length that will later be encrypted and part of the MAC (AD)


Suggested change

// prepare the packet length that will later be encrypted and part of the MAC (AD)

// prepare the packet length that will later be encrypted and part of the MAC (AAD)

stratospher · 2021-11-05T05:36:07Z

src/net.cpp

+
+    // insert header directly into the CSerializedNetMsg data buffer (insert at begin)
+    // TODO: if we refactor the ChaCha20Poly1350 crypt function to allow separate buffers for
+    //       the AD, payload and MAC, we could avoid a insert and thus a potential reallocation


Suggested change

// the AD, payload and MAC, we could avoid a insert and thus a potential reallocation

// the AAD, payload and MAC, we could avoid a insert and thus a potential reallocation

stratospher · 2021-11-05T05:54:07Z

src/net.cpp

+    }
+
+    // insert header directly into the CSerializedNetMsg data buffer (insert at begin)
+    // TODO: if we refactor the ChaCha20Poly1350 crypt function to allow separate buffers for


Suggested change

// TODO: if we refactor the ChaCha20Poly1350 crypt function to allow separate buffers for

// TODO: if we refactor the ChaCha20Poly1305 crypt function to allow separate buffers for

stratospher · 2021-11-05T06:08:56Z

src/net.cpp

+
+std::optional<CNetMessage> V2TransportDeserializer::GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size)
+{
+    // In v2, vRecv contains the encrypted payload plus the MAC tag (1+bytes serialized message command + ? bytes message payload + 16 byte mac tag)


Suggested change

// In v2, vRecv contains the encrypted payload plus the MAC tag (1+bytes serialized message command + ? bytes message payload + 16 byte mac tag)

// In v2, vRecv contains the AAD, encrypted payload plus the MAC tag (3 byte AAD + 1+bytes serialized message command + ? bytes message payload + 16 byte mac tag)

stratospher · 2021-11-05T06:10:54Z

src/test/net_tests.cpp

+            //  second: read the encrypted payload (if required)
+            Span<const uint8_t> span_msg(msg.data.data(), msg.data.size());
+            if (msg.data.size() > 0) read_bytes += deserializer->Read(span_msg);
+            if (msg.data.size() > read_bytes && msg.data.size() - read_bytes > 0) {


Suggested change

if (msg.data.size() > read_bytes && msg.data.size() - read_bytes > 0) {

if (msg.data.size() > read_bytes) {

Oh, that was quite silly. Thanks for reviewing closely. Done.

stratospher · 2021-11-05T06:18:50Z

src/test/net_tests.cpp

+            uint32_t out_err_raw_size{0};
+            std::optional<CNetMessage> result{deserializer->GetMessage(GetTime<std::chrono::microseconds>(), out_err_raw_size)};


This can be updated to keep it consistent with 22735's change.

Suggested change

uint32_t out_err_raw_size{0};

std::optional<CNetMessage> result{deserializer->GetMessage(GetTime<std::chrono::microseconds>(), out_err_raw_size)};

bool reject_message{false};

CNetMessage result{deserializer->GetMessage(GetTime<std::chrono::microseconds>(), reject_message)};

stratospher · 2021-11-05T06:21:55Z

src/test/fuzz/p2p_v2_transport_serialization.cpp

+            uint32_t out_err_raw_size{0};
+            std::optional<CNetMessage> result{deserializer.GetMessage(m_time, out_err_raw_size)};


Suggested change

uint32_t out_err_raw_size{0};

std::optional<CNetMessage> result{deserializer.GetMessage(m_time, out_err_raw_size)};

bool reject_message{false};

CNetMessage result{deserializer.GetMessage(m_time, reject_message)};

Here also.

dhruv · 2021-11-09T00:28:41Z

Rebased to bring in changes from #22735, and addressed comments from @stratospher (Thank you!). Ready for further review.

src/net.cpp

dhruv · 2021-11-23T23:29:58Z

Pushed changes to enforce connection termination upon finding an invalid mac tag. Ready for further review.

dhruv · 2023-02-02T06:27:06Z

Rebased. Brought in upstream changes.

dhruv · 2023-02-21T03:34:25Z

Rebased.

dhruv · 2023-03-08T15:26:47Z

Latest push:

Implemented changes from BIP324: Fixed length message type IDs bips#1428 involving fixed length message type ids (there's also a relevant discussion on the ML)
While implementing it, I found a bug in the arm of code that serialized/deserialized message types without assigned short ids. That branch of code was previously unexercised by any tests. This is no longer the case.

Co-authored-by: Jonas Schnelli <dev@jonasschnelli.ch>

dhruv · 2023-03-21T04:01:24Z

Latest push:

Upstream changes from BIP324: Cipher suite #25361
Rebased

fanquake · 2023-05-06T11:11:30Z

Closing for now. This will be picked up again later. BIP324 review attention should be directed towards #27479 and bitcoin-core/secp256k1#1129.

dhruv force-pushed the bip324-net-v2 branch 3 times, most recently from 988a1af to f81e8ad Compare October 8, 2021 19:46

DrahtBot added Build system P2P Utils/log/libs labels Oct 8, 2021

dhruv mentioned this pull request Oct 8, 2021

Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

Closed

maflcko reviewed Oct 9, 2021

View reviewed changes

dhruv force-pushed the bip324-net-v2 branch from f81e8ad to ced5151 Compare October 10, 2021 03:43

dhruv mentioned this pull request Oct 10, 2021

Alter the ChaCha20Poly1305@Bitcoin AEAD to the new specification #20962

Closed

This was referenced Oct 10, 2021

net: fix GetListenPort() to derive the proper port #20196

Merged

crypto: Fix K1/K2 use in the comments in ChaCha20-Poly1305 AEAD #23271

Merged

DrahtBot added the Needs rebase label Oct 21, 2021

dhruv force-pushed the bip324-net-v2 branch from ced5151 to ab5b49d Compare October 22, 2021 18:21

DrahtBot removed the Needs rebase label Oct 22, 2021

stratospher reviewed Nov 5, 2021

View reviewed changes

This was referenced Nov 5, 2021

p2p: Erlay support signaling #23443

Merged

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

ryihan approved these changes Nov 6, 2021

View reviewed changes

dhruv force-pushed the bip324-net-v2 branch from ab5b49d to 2f42113 Compare November 9, 2021 00:27

DrahtBot mentioned this pull request Nov 9, 2021

fuzz: Differential fuzzing for ChaCha20Forward4064-Poly1305@bitcoin cipher suite #23441

Closed

dhruv commented Nov 17, 2021

View reviewed changes

src/net.cpp Show resolved Hide resolved

dhruv force-pushed the bip324-net-v2 branch from 2f42113 to 0305c31 Compare November 23, 2021 23:29

DrahtBot mentioned this pull request Nov 26, 2021

BIP324: Handshake prerequisites #23561

Closed

DrahtBot added the Needs rebase label Feb 15, 2023

dhruv added 3 commits February 20, 2023 15:00

RFC8439 nonce and counter for ChaCha20

8e04f05

RFC8439 implementation and tests

7a9d2fb

Adding forward secure FSChaCha20

acd664e

dhruv force-pushed the bip324-net-v2 branch from fc3b5a9 to e38dc37 Compare February 21, 2023 03:32

DrahtBot removed the Needs rebase label Feb 21, 2023

dhruv force-pushed the bip324-net-v2 branch from e38dc37 to d801b65 Compare March 8, 2023 15:22

DrahtBot mentioned this pull request Mar 15, 2023

refactor: Extract util/fs from util/system #27254

Merged

dhruv and others added 7 commits March 20, 2023 13:34

BIP324 Cipher Suite

187761f

Allow for RFC8439 AD in cipher suite interface

8405856

Merge branch 'bip324-cipher-suite' into bip324-net-v2

0c3c7ab

Add BIP324 short-IDs to protocol.cpp

9d9c46f

Co-authored-by: Jonas Schnelli <dev@jonasschnelli.ch>

Add BIP324 v2 transport serializer and deserializer

c32329c

Co-authored-by: Jonas Schnelli <dev@jonasschnelli.ch>

fuzz: Add fuzz test for v2 transport {de}serialization

16eeb43

Expose BIP324CipherSuite AAD via transport classes

f019556

dhruv force-pushed the bip324-net-v2 branch from d801b65 to f019556 Compare March 21, 2023 04:00

This was referenced Apr 1, 2023

net, refactor: extract Network and BIP155Network logic to node/network #27385

Closed

net, refactor: Privatise CNode send queue #27407

Closed

This was referenced Apr 13, 2023

p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting #27411

Merged

BIP324: ElligatorSwift integrations #27479

Merged

This was referenced Apr 27, 2023

rpc: add 'getnetmsgstats', new rpc to view network message statistics #27534

Closed

Remove now-unnecessary poll, fcntl includes from net(base).cpp #27530

Merged

fanquake closed this May 6, 2023

sipa mentioned this pull request May 12, 2023

BIP324 tracking issue #27634

Closed

43 tasks

bitcoin locked and limited conversation to collaborators May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BIP324: Add encrypted p2p transport {de}serializer #23233

BIP324: Add encrypted p2p transport {de}serializer #23233

dhruv commented Oct 8, 2021 •

edited

Loading

dhruv commented Oct 8, 2021

maflcko Oct 9, 2021

dhruv Oct 10, 2021

DrahtBot commented Oct 9, 2021 •

edited

Loading

dhruv commented Oct 10, 2021

dhruv commented Oct 22, 2021

stratospher left a comment

stratospher Nov 5, 2021

dhruv Nov 9, 2021

stratospher Nov 5, 2021

dhruv Nov 9, 2021

stratospher Nov 5, 2021

dhruv Nov 9, 2021

stratospher Nov 5, 2021

dhruv Nov 9, 2021

stratospher Nov 5, 2021

dhruv Nov 9, 2021

stratospher Nov 5, 2021

dhruv Nov 9, 2021

stratospher Nov 5, 2021

dhruv Nov 9, 2021

stratospher Nov 5, 2021

dhruv Nov 9, 2021

stratospher Nov 5, 2021

dhruv Nov 9, 2021

stratospher Nov 5, 2021

dhruv Nov 9, 2021

dhruv commented Nov 9, 2021 •

edited

Loading

dhruv commented Nov 23, 2021

dhruv commented Feb 2, 2023

dhruv commented Feb 21, 2023

dhruv commented Mar 8, 2023

dhruv commented Mar 21, 2023

fanquake commented May 6, 2023

		uint32_t len = aead.DecryptLength(in.data());
		len = 0; // addressing the [[nodiscard]] and otherwise unused variable

	uint32_t len = aead.DecryptLength(in.data());
	len = 0; // addressing the [[nodiscard]] and otherwise unused variable
	(void)aead.DecryptLength(in.data());

	// prepare the packet length that will later be encrypted and part of the MAC (AD)
	// prepare the packet length that will later be encrypted and part of the MAC (AAD)

	// the AD, payload and MAC, we could avoid a insert and thus a potential reallocation
	// the AAD, payload and MAC, we could avoid a insert and thus a potential reallocation

	// TODO: if we refactor the ChaCha20Poly1350 crypt function to allow separate buffers for
	// TODO: if we refactor the ChaCha20Poly1305 crypt function to allow separate buffers for

	// In v2, vRecv contains the encrypted payload plus the MAC tag (1+bytes serialized message command + ? bytes message payload + 16 byte mac tag)
	// In v2, vRecv contains the AAD, encrypted payload plus the MAC tag (3 byte AAD + 1+bytes serialized message command + ? bytes message payload + 16 byte mac tag)

	if (msg.data.size() > read_bytes && msg.data.size() - read_bytes > 0) {
	if (msg.data.size() > read_bytes) {

		uint32_t out_err_raw_size{0};
		std::optional<CNetMessage> result{deserializer->GetMessage(GetTime<std::chrono::microseconds>(), out_err_raw_size)};

BIP324: Add encrypted p2p transport {de}serializer #23233

BIP324: Add encrypted p2p transport {de}serializer #23233

Conversation

dhruv commented Oct 8, 2021 • edited Loading

dhruv commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented Oct 9, 2021 • edited Loading

Reviews

Conflicts

dhruv commented Oct 10, 2021

dhruv commented Oct 22, 2021

stratospher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruv commented Nov 9, 2021 • edited Loading

dhruv commented Nov 23, 2021

dhruv commented Feb 2, 2023

dhruv commented Feb 21, 2023

dhruv commented Mar 8, 2023

dhruv commented Mar 21, 2023

fanquake commented May 6, 2023

dhruv commented Oct 8, 2021 •

edited

Loading

DrahtBot commented Oct 9, 2021 •

edited

Loading

dhruv commented Nov 9, 2021 •

edited

Loading