Alter the ChaCha20Poly1305@Bitcoin AEAD to the new specification

jonasschnelli commented at 10:06 am on January 19, 2021: contributor

During discussions and reviews of BIP324 (p2p transport encryption), a more efficient AEAD was proposed.

The main difference is how the construct implements re-keying. Since both, the original and the new BIP324 AEAD will not repeat the key handshake over time (no re-keying on the ECDH level), a simpler form of direct re-keying has been proposed.

The new AEAD uses a ChaCha20 stream cipher where byte 4064 till byte 4096 (last 32 bytes in a 4096 window) are used to re-key the same instance. Re-keying in that context means re-setting the ChaCha20 key (thus setting the constant “expand 32-byte k” again, resetting the counter) to the last 32bytes of the current 4096 chunk. We never encrypt more than 4096bytes with the same key. On every re-key, we increase the IV by 1 (a sequence number). This should allow forward secrecy in the same way as the old (and slightly cumbersome) rekeying approach.

The AEAD is currently only used in tests. The serializer and deserializer for the V2 transport are in #23233.

BIP324 proposal can be found here: https://gist.github.com/dhruv/5b1275751bc98f3b64bcafce7876b489

jonasschnelli added the label P2P on Jan 19, 2021

jonasschnelli assigned sipa on Jan 19, 2021

DrahtBot commented at 11:59 pm on January 24, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25172 (refactor: use std:: prefix for std lib funcs by fanquake)

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.

DrahtBot added the label Needs rebase on Jan 26, 2021

jonasschnelli force-pushed on Jan 29, 2021

jonasschnelli commented at 8:47 am on January 29, 2021: contributor

Rebased

DrahtBot removed the label Needs rebase on Jan 29, 2021

PastaPastaPasta approved

PastaPastaPasta commented at 7:45 pm on April 13, 2021: contributor

utACK;

this PR matches to the best of my understanding what is outlined in the BIP. I saw no significant formatting problems

That’s basically where my expertise stops. However, I’m not sure that we should be using the p2p tag since this is pure crypto but #15649 had p2p tag so I guess that’s okay.

Is this PR waiting on some other PR / BIP change, or is the blocker here review by sipa and others?

PastaPastaPasta commented at 5:36 pm on May 23, 2021: contributor

@sipa @jonasschnelli Is there a reason why this PR has stalled?

in src/crypto/chacha_poly_aead.h:76 in c22f607b62 outdated

148+*
149+* If we reach bytes 4064 on the ChaCha20 stream, use the next 32 bytes (byte
150+* 4065-4096) and set is as the new ChaCha20 key, reset the counter to 0 while
151+* incrementing the sequence number + 1 and set is as IV (little endian encoding).
152+*
153+* For the payload, use the ChaCha20 stream keyed with K_1 and apply the same

dhruv commented at 8:32 pm on June 25, 2021:

Should this be K_2?

dhruv commented at 7:38 pm on August 23, 2021:

Done.

in src/crypto/chacha_poly_aead.h:136 in c22f607b62 outdated

218+    bool Crypt(unsigned char* dest, size_t dest_len, const unsigned char* src, size_t src_len, bool is_encrypt);
219 
220-    /** decrypts the 3 bytes AAD data and decodes it into a uint32_t field */
221-    bool GetLength(uint32_t* len24_out, uint64_t seqnr_aad, int aad_pos, const uint8_t* ciphertext);
222+    /** decrypts the 3 bytes AAD data (the packet length) and decodes it into a uint32_t field
223+        the ciphertext will not be manipulated but the AEAD state changes (can't be called multiple times)

dhruv commented at 10:29 pm on June 25, 2021:

(nit): “AEAD state changes (can’t be called multiple times)” -> “AAD keystream will advance. As a result, DecryptLength() cannot be called multiple times to get the same result. The caller must cache the result for re-use.”

dhruv commented at 7:38 pm on August 23, 2021:

Done.

in src/crypto/chacha_poly_aead.h:133 in c22f607b62 outdated

213         src, the AAD+payload to encrypt or the AAD+payload+MAC to decrypt
214         src_len, the length of the source buffer
215         is_encrypt, set to true if we encrypt (creates and appends the MAC instead of verifying it)
216         */
217-    bool Crypt(uint64_t seqnr_payload, uint64_t seqnr_aad, int aad_pos, unsigned char* dest, size_t dest_len, const unsigned char* src, size_t src_len, bool is_encrypt);
218+    bool Crypt(unsigned char* dest, size_t dest_len, const unsigned char* src, size_t src_len, bool is_encrypt);

dhruv commented at 10:49 pm on June 25, 2021:

nit: [[nodiscard]] ?

Also, can we add to the comments: “Returns true if encipher succeeds. Upon failure, the data at dest should not be used”

dhruv commented at 7:38 pm on August 23, 2021:

Done.

in src/crypto/chacha_poly_aead.h:139 in c22f607b62 outdated

221-    bool GetLength(uint32_t* len24_out, uint64_t seqnr_aad, int aad_pos, const uint8_t* ciphertext);
222+    /** decrypts the 3 bytes AAD data (the packet length) and decodes it into a uint32_t field
223+        the ciphertext will not be manipulated but the AEAD state changes (can't be called multiple times)
224+        Ciphertext needs to stay encrypted due to the MAC check that will follow (requires encrypted length)
225+        */
226+    bool DecryptLength(uint32_t* len24_out, const uint8_t* ciphertext);

dhruv commented at 10:49 pm on June 25, 2021:

Could this function signature be [[nodiscard]] uint32_t DecryptLength(const uint8_t* ciphertext) ? I’m curious why we return value using the pointer argument ?

dhruv commented at 7:38 pm on August 23, 2021:

Done.

in src/crypto/chacha_poly_aead.h:111 in c22f607b62 outdated

183+    uint64_t m_seqnr{0};
184+    size_t m_keystream_pos{0};
185+    unsigned char m_keystream[KEYSTREAM_SIZE] = {0};
186+public:
187+    ~ChaCha20ReKey4096();
188+    void SetKey(const unsigned char* key, size_t keylen);

dhruv commented at 0:07 am on June 26, 2021:

Would it make sense for this to be done in a constructor since this class implements re-keying internally and ideally, the user should not call this function more than once on an instance?

dhruv commented at 7:39 pm on August 23, 2021:

Done.

in src/crypto/chacha_poly_aead.cpp:49 in c22f607b62 outdated

44+void ChaCha20ReKey4096::Crypt(const unsigned char* input, unsigned char* output, size_t bytes) {
45+    size_t message_pos = 0;
46+
47+    // TODO: speedup with a block approach (rather then looping over every byte)
48+    while (bytes > message_pos) {
49+        output[message_pos] = input[message_pos] ^ ReadLE32(&m_keystream[m_keystream_pos]);

dhruv commented at 5:40 am on June 26, 2021:

To make sure I am understanding this correctly (did try it in godbolt):

ReadLE32(&m_keystream[m_keystream_pos]) will interpret 4 bytes starting at m_keystream_pos as a little endian encoded uint32_t. This means the LSB of the uint32_t will be the byte at m_keystream[m_keystream_pos]. The XOR with unsigned char will only use this LSB. Perhaps this was done for batching but not needed right now?

I applied the patch below and crypto_tests/chacha20_poly1305_aead_testvector passed.

 0diff --git a/src/crypto/chacha_poly_aead.cpp b/src/crypto/chacha_poly_aead.cpp
 1index bf261df55..139168fd7 100644
 2--- a/src/crypto/chacha_poly_aead.cpp
 3+++ b/src/crypto/chacha_poly_aead.cpp
 4@@ -46,8 +46,7 @@ void ChaCha20ReKey4096::Crypt(const unsigned char* input, unsigned char* output,
 5 
 6     // TODO: speedup with a block approach (rather then looping over every byte)
 7     while (bytes > message_pos) {
 8-        output[message_pos] = input[message_pos] ^ ReadLE32(&m_keystream[m_keystream_pos]);
 9-        m_keystream_pos++;
10+        output[message_pos] = input[message_pos] ^ m_keystream[m_keystream_pos++];
11         message_pos++;
12         if (m_keystream_pos == KEYSTREAM_SIZE-CHACHA20_POLY1305_AEAD_KEY_LEN) {
13             // we reached the end of the keystream

dhruv commented at 7:39 pm on August 23, 2021:

Done.

in src/crypto/chacha_poly_aead.cpp:53 in c22f607b62 outdated

50+        m_keystream_pos++;
51+        message_pos++;
52+        if (m_keystream_pos == KEYSTREAM_SIZE-CHACHA20_POLY1305_AEAD_KEY_LEN) {
53+            // we reached the end of the keystream
54+            // rekey with the remaining and last 32 bytes and precompute the next 4096 bytes
55+            m_ctx.SetKey(&m_keystream[m_keystream_pos], CHACHA20_POLY1305_AEAD_KEY_LEN);

dhruv commented at 5:44 am on June 26, 2021:

Could we leave a comment here that SetKey() also sets the counter and IV to zero?

dhruv commented at 7:32 pm on August 23, 2021:

Done.

in src/crypto/chacha_poly_aead.cpp:75 in c22f607b62 outdated

76-    // we can't hit uint64 max since the rekey rule (which resets the sequence number) is 1GB
77-    m_cached_aad_seqnr = std::numeric_limits<uint64_t>::max();
78 }
79 
80-bool ChaCha20Poly1305AEAD::Crypt(uint64_t seqnr_payload, uint64_t seqnr_aad, int aad_pos, unsigned char* dest, size_t dest_len /* length of the output buffer for sanity checks */, const unsigned char* src, size_t src_len, bool is_encrypt)
81+bool ChaCha20Poly1305AEAD::Crypt(unsigned char* dest, size_t dest_len /* length of the output buffer for sanity checks */, const unsigned char* src, size_t src_len, bool is_encrypt)

dhruv commented at 5:51 am on June 26, 2021:

Would it be useful to completely hide the details of the encrypted length from the user of this class?

That way, for encryption: src=plaintext; dest=encrypted length + ciphertext + MAC

for decryption: src=encrypted length + ciphertext + MAC; dest=plaintext

This would:

Eliminate the awkwardness around the user of this class accidentally calling DecryptLength multiple times.
Eliminate the possibility that the user of this class forgets to terminate the connection if the decrypted length does not match the length of the payload.

Or perhaps I am missing a reason the user of the class might need to know the decrypted length.

dhruv commented at 7:33 pm on August 23, 2021:

Or perhaps I am missing a reason the user of the class might need to know the decrypted length.

I was missing the reason: Tried to do this and realized that the interface for TransportDeserializer actually requires the client of this class to be aware of the length because the bytes come in as a stream.

laanwj commented at 12:02 pm on June 2, 2022:

Yes, I had a similar idea, it’s a bit awkward like this. But apparently there’s a good reason.

dhruv commented at 3:55 am on June 13, 2022:

Yeah, we need a way to know the boundaries of the messages coming in a single stream.

in src/crypto/chacha_poly_aead.cpp:95 in c22f607b62 outdated

 96+    // 1. AAD (the encrypted packet length), use the header-keystream
 97+    if (is_encrypt) {
 98+        m_chacha_header.Crypt(src, dest, 3);
 99+    } else {
100+        // we must use ChaCha20Poly1305AEAD::DecryptLength before calling ChaCha20Poly1305AEAD::Crypt
101+        // thus the length has already been encrypted, avoid doing it again and messing up the keystream position

dhruv commented at 6:01 am on June 26, 2021:

should this be “the length has already been decrypted”?

dhruv commented at 7:40 pm on August 23, 2021:

Done.

in src/crypto/chacha_poly_aead.h:60 in c22f607b62 outdated

132+* (assuming key derivation, ChaCha20 and Poly1305 are secure). Active observers
133+* can still obtain the message length (ex. active ciphertext bit flipping or
134+* traffic shemantics analysis)
135+*
136+* The AEAD is constructed as follows: generate two ChaCha20 streams, initially
137+* keyed with K_1 and K_2 and IV of 0 and a block counter of 0 and a sequence

dhruv commented at 3:22 pm on June 26, 2021:

(nit): “IV of 0 and a block counter of 0 and a sequence number 0 as IV” -> “sequence number 0 as IV and a block counter of 0”

dhruv commented at 7:40 pm on August 23, 2021:

Done.

in src/crypto/chacha_poly_aead.h:81 in c22f607b62 outdated

156+*
157+* ==== Packet Handling ====
158+*
159+* When receiving a packet, the length must be decrypted first. When 3 bytes of
160+* ciphertext length have been received, they MUST be decrypted.
161+*

dhruv commented at 3:27 pm on June 26, 2021:

In #18242, I see that the bytes are accessible as a Span and Span::size() seems available. Would it be useful to say something like: “If the decrypted length does not match the payload length, the connection MUST be immediately terminated?”

At first I thought that is implicitly delegated to the MAC, but we don’t seem to confirm that the length is correct when encrypting either.

dhruv commented at 7:37 pm on August 23, 2021:

Thinking through this more, I realized that:

Sender errors in encrypted length cannot be corrected since the bytes for multiple p2p messages are in a single TCP stream. Such sender errors are protocol errors.
MITM errors/attacks will be caught by the MAC check.

in src/crypto/chacha_poly_aead.h:98 in c22f607b62 outdated

170+* Detection of an invalid MAC MUST lead to immediate connection termination.
171+*
172+* To send a packet, first encode the 3 byte length and encrypt it using the
173+* ChaCha20 stream keyed with K_1 as described above. Encrypt the packet payload
174+* (using the ChaCha20 stream keyed with K_2) and append it to the encrypted
175+* length. Finally, calculate a MAC tag and append it.

dhruv commented at 3:30 pm on June 26, 2021:

(nit): “Finally, calculate a MAC tag and append it.” -> “Finally, calculate a MAC tag(using poly1305 key from stream keyed with K_1) and append it.”

dhruv commented at 7:40 pm on August 23, 2021:

Done.

in src/crypto/chacha_poly_aead.h:90 in c22f607b62 outdated

162+* Once the entire packet has been received, the MAC MUST be checked before
163+* decryption. A per-packet Poly1305 key is generated as described above and the
164+* MAC tag is calculated using Poly1305 with this key over the ciphertext of the
165+* packet length and the payload together. The calculated MAC is then compared in
166+* constant time with the one appended to the packet and the packet decrypted
167+* using ChaCha20 as described above (with K_2, the packet sequence number as

dhruv commented at 3:32 pm on June 26, 2021:

should “(with K_2, the packet sequence number as nonce and a starting block counter of 1)” now be just “(using stream keyed with K_2)”? IIUC, the block counter could be anything in [0, 7] (since 8 * 512 = 4096), right?

dhruv commented at 7:41 pm on August 23, 2021:

Done.

in src/test/crypto_tests.cpp:619 in c22f607b62 outdated

627-              (ciphertext_buf[1] ^ cmp_ctx_buffer[aad_pos + 1]) << 8 |
628-              (ciphertext_buf[2] ^ cmp_ctx_buffer[aad_pos + 2]) << 16;
629-    BOOST_CHECK_EQUAL(len_cmp, expected_aad_length);
630-
631-    // encrypt / decrypt 1000 packets
632+    BOOST_CHECK(aead_in.DecryptLength(&out_len, ciphertext_buf.data()));

dhruv commented at 6:01 pm on June 28, 2021:

This line implies that the decrypted length is len(AAD) + len(payload). I interpreted, this to mean that the decrypted length is the length of the ciphertext of the payload alone.

If this is intentional, can we make it clearer in the BIP? IIUC, typically, in other protocols, the length in the preamble is the length of the payload that follows. Did we want to do it that way?

dhruv commented at 7:42 pm on August 23, 2021:

The BIP, the unit test and the bench test have been updated to be consistently clear that the encrypted length is len(ciphertext_payload) and not len(ciphertext_payload + ciphertext_aad)

dhruv commented at 6:11 pm on June 28, 2021: member

Approach ACK c22f607b62af59969b9378d4cd8ed72b866dec11

I reviewed the code to reconcile with the new, accepted recommendation in BIP324 and ran the test.

The test vector changes are harder to review. I am happy to re-implement the AEAD in python so we can fuzz the C++ implementation against it(different language and author) if that is useful.

DrahtBot added the label Needs rebase on Aug 19, 2021

dhruv force-pushed on Aug 23, 2021

dhruv commented at 8:39 pm on August 23, 2021: member

I will be taking on this PR. Rebased with master. Addressed my own comments. Ready for further review.

DrahtBot removed the label Needs rebase on Aug 23, 2021

in src/crypto/chacha_poly_aead.h:11 in 424e0100fb outdated

 5@@ -6,6 +6,7 @@
 6 #define BITCOIN_CRYPTO_CHACHA_POLY_AEAD_H
 7 
 8 #include <crypto/chacha20.h>
 9+#include <crypto/poly1305.h>
10 
11 #include <cmath>
12

stratospher commented at 6:23 pm on August 25, 2021:

AAD_PACKAGES_PER_ROUND is defined but not used anywhere. Couldn’t we remove it?

dhruv commented at 9:47 pm on September 12, 2021:

Good catch. Fixed.

in src/crypto/chacha_poly_aead.cpp:37 in 424e0100fb outdated

35-    assert(K_2_len == CHACHA20_POLY1305_AEAD_KEY_LEN);
36+    assert(keylen == 32);
37+    m_ctx.SetKey(key, keylen);
38+
39+    // set initial sequence number
40+    m_seqnr = 0;

stratospher commented at 6:33 pm on August 25, 2021:

Since we’re already initialising m_seqnr with 0 in the class definition of ChaCha20Forward4064, https://github.com/bitcoin/bitcoin/blob/424e0100fbea0ac8106b25a9b23698f2c2cd7a4f/src/crypto/chacha_poly_aead.h#L107 Couldn’t we remove this line from the constructor?

dhruv commented at 9:47 pm on September 12, 2021:

Done.

in src/crypto/chacha_poly_aead.cpp:42 in 424e0100fb outdated

40+    m_seqnr = 0;
41+    m_ctx.SetIV(m_seqnr);
42+
43+    // precompute first chunk of keystream
44+    m_ctx.Keystream(m_keystream, KEYSTREAM_SIZE);
45+    m_keystream_pos = 0;

stratospher commented at 6:35 pm on August 25, 2021:

Since we’re already initialising m_keystream_pos with 0 in the class definition of ChaCha20Forward4064, https://github.com/bitcoin/bitcoin/blob/424e0100fbea0ac8106b25a9b23698f2c2cd7a4f/src/crypto/chacha_poly_aead.h#L108 Couldn’t we remove this line from the constructor?

dhruv commented at 9:47 pm on September 12, 2021:

Done.

in src/crypto/chacha_poly_aead.cpp:75 in 424e0100fb outdated

82+                                           size_t K_1_len,
83+                                           const unsigned char* K_2,
84+                                           size_t K_2_len) : m_chacha_header(K_1, CHACHA20_POLY1305_AEAD_KEY_LEN), m_chacha_main(K_2, CHACHA20_POLY1305_AEAD_KEY_LEN)
85+{
86+    assert(K_1_len == CHACHA20_POLY1305_AEAD_KEY_LEN);
87+    assert(K_2_len == CHACHA20_POLY1305_AEAD_KEY_LEN);

stratospher commented at 6:44 pm on August 25, 2021:

We are checking whether the key length is 32 on this line https://github.com/bitcoin/bitcoin/blob/424e0100fbea0ac8106b25a9b23698f2c2cd7a4f/src/crypto/chacha_poly_aead.cpp#L33 when the constructors of m_chacha_header and m_chacha_main get called. So couldn’t we remove these 2 lines?

dhruv commented at 9:48 pm on September 12, 2021:

I find these asserts clarifying and afaict, C++ asserts are optimized away in release builds so it’s not slowing anything down. Leaving these in here for now.

dhruv commented at 0:27 am on September 17, 2021:

Update: Turns out this was mistaken. assert() is not optimized away in optimized Bitcoin Core builds.

stratospher commented at 7:14 pm on August 25, 2021: contributor

Approach ACK. It would be nice to remove a few redundant lines of code.

dhruv force-pushed on Sep 12, 2021

dhruv commented at 9:48 pm on September 12, 2021: member

Comments from @stratospher addressed. Ready for further review.

dhruv force-pushed on Oct 10, 2021

dhruv commented at 4:52 am on October 10, 2021: member

Addressed #23233 (review) - ready for further review

in src/crypto/chacha_poly_aead.h:11 in 0b93e3d2e8 outdated

 5@@ -6,13 +6,13 @@
 6 #define BITCOIN_CRYPTO_CHACHA_POLY_AEAD_H
 7 
 8 #include <crypto/chacha20.h>
 9+#include <crypto/poly1305.h>
10 
11 #include <cmath>

stratospher commented at 2:11 pm on October 15, 2021:

#include <cmath> isn’t being used.

dhruv commented at 6:10 pm on October 22, 2021:

Done

in src/crypto/chacha_poly_aead.h:56 in 0b93e3d2e8 outdated

128+* checking the MAC. By using an independently-keyed cipher instance to encrypt
129+* the length, an active attacker seeking to exploit the packet input handling as
130+* a decryption oracle can learn nothing about the payload contents or its MAC
131+* (assuming key derivation, ChaCha20 and Poly1305 are secure). Active observers
132+* can still obtain the message length (ex. active ciphertext bit flipping or
133+* traffic shemantics analysis)

stratospher commented at 2:14 pm on October 15, 2021:

a typo for semantics? :)

dhruv commented at 6:11 pm on October 22, 2021:

:) done.

in src/crypto/chacha_poly_aead.cpp:10 in 0b93e3d2e8 outdated

 3@@ -4,6 +4,7 @@
 4 
 5 #include <crypto/chacha_poly_aead.h>
 6 
 7+#include <crypto/common.h>
 8 #include <crypto/poly1305.h>
 9 #include <support/cleanse.h>
10

stratospher commented at 2:25 pm on October 15, 2021:

These imports can be removed since they aren’t used:

#include <string.h>
#include <cstdio>
#include <limits>

dhruv commented at 6:12 pm on October 22, 2021:

Need string.h for size_t: https://en.cppreference.com/w/c/types/size_t Added cstring for memset: https://en.cppreference.com/w/cpp/string/byte/memset

Removed cstdio and limits

in src/test/fuzz/crypto_chacha20_poly1305_aead.cpp:19 in 0b93e3d2e8 outdated

20@@ -21,9 +21,6 @@ FUZZ_TARGET(crypto_chacha20_poly1305_aead)
21     const std::vector<uint8_t> k2 = ConsumeFixedLengthByteVector(fuzzed_data_provider, CHACHA20_POLY1305_AEAD_KEY_LEN);

stratospher commented at 3:17 pm on October 15, 2021:

These header files aren’t used too and can be removed:

EDIT: I’m a bit confused about whether cassert needs to be removed. Even though an assert statement isn’t used anywhere in the fuzz test file, it maybe needed in the fuzzing environment?

dhruv commented at 6:14 pm on October 22, 2021:

Done.

in src/bench/chacha_poly_aead.cpp:18 in 0b93e3d2e8 outdated

18@@ -19,43 +19,29 @@ static constexpr uint64_t BUFFER_SIZE_LARGE = 1024 * 1024;
19 static const unsigned char k1[32] = {0};

stratospher commented at 3:18 pm on October 15, 2021:

#include <limits> can be removed since it’s usage in the file has been removed.

dhruv commented at 6:14 pm on October 22, 2021:

Done.

in src/bench/chacha_poly_aead.cpp:40 in 0b93e3d2e8 outdated

47         if (include_decryption) {
48             // if we decrypt, include the GetLength
49-            const bool get_length_ok = aead.GetLength(&len, seqnr_aad, aad_pos, in.data());
50-            assert(get_length_ok);
51-            const bool crypt_ok_2 = aead.Crypt(seqnr_payload, seqnr_aad, aad_pos, out.data(), out.size(), in.data(), buffersize, true);
52+            auto len = aead_in.DecryptLength(out.data());

stratospher commented at 3:22 pm on October 15, 2021:

This comment suggestion can be applied here too.

dhruv commented at 6:14 pm on October 22, 2021:

Ah, I was looking for another place this was useful and didn’t remember correctly. Thanks, @stratospher !

stratospher commented at 3:41 pm on October 15, 2021: contributor

Concept ACK 0b93e3d

It’s super awesome to see BIP 324 shaping up! This PR captures the ChaCha20Forward4064-Poly1305@Bitcoin cipher suite proposed in BIP 324 beautifully.

	BIP 324	PR
Prerequisites	ChaCha20 PRFPoly1305 MACChaCha20Forward4064-Poly1305@Bitcoin cipher suite2 separate instances of ChaCha20Forward4064 DRBG using keys k1 and k2 called F and V respectively.	Defined in: chacha20.hpoly1305.hchacha_poly_aead.hF is m_chacha_header and V is m_chacha_main
Encryption		Crypt() is in line with the BIP’s specifications.
Decryption		Crypt() handles both encryption and decryption.

I noticed some more refactoring which could be done.

DrahtBot added the label Needs rebase on Oct 21, 2021

dhruv force-pushed on Oct 22, 2021

dhruv commented at 6:15 pm on October 22, 2021: member

Thank you for the great diagrams, @stratospher ! They’ll come in handy at a future review club meeting and in docs.

Review comments addressed. Rebased. Ready for further review.

DrahtBot removed the label Needs rebase on Oct 22, 2021

in src/crypto/chacha_poly_aead.h:38 in 380b137556 outdated

110- * 2^70 bytes under the same {key, nonce}.
111- *
112- * We use message sequence numbers for both communication directions.
113- */
114+The chacha20-poly1305@bitcoin cipher requires two 256 bits of key material as
115+* output from the key exchange. Each key (K_1 and K_2) are used by two separate

0xB10C commented at 7:31 am on October 23, 2021:

indention seem to be off-by-one for the remainder of this comment and a * is missing in line 37

dhruv commented at 7:34 pm on November 4, 2021:

thanks for the catch. fixed.

dhruv force-pushed on Nov 4, 2021

dhruv commented at 7:35 pm on November 4, 2021: member

Addressed comments. Rebased against master to get the changes from #22735 which are needed to bring them into #23233 (downstream of this PR branch). Ready for further review.

ryihan approved

dhruv force-pushed on Jan 21, 2022

dhruv commented at 4:46 am on January 21, 2022: member

Addressed some comments for this PR left over at #23233 by @laanwj:

#23233 (review) #23233 (review)

I also rebased against master because at one point I had trouble running fuzz tests(but it turned out to be unrelated).

Ready for further review.

DrahtBot added the label Needs rebase on Jan 31, 2022

dhruv force-pushed on Feb 2, 2022

dhruv commented at 2:50 am on February 2, 2022: member

Rebased. Ready for further review.

git range-diff 02e1d8d06f 41deee4 8b474d1

DrahtBot removed the label Needs rebase on Feb 2, 2022

jonatack commented at 3:28 pm on March 3, 2022: member

Concept ACK, will review soon.

dhruv force-pushed on Mar 21, 2022

dhruv commented at 8:12 pm on March 21, 2022: member

Thanks to @stratospher for finding a bug in the test code. We were using WriteLE32 to write the 3 byte header plaintext after the payload. This was overwriting the first byte in the payload.

git range-diff 91d12344b 8b474d1 97b768f

Ready for further review.

dhruv added this to the "Needs review" column in a project

in src/crypto/chacha_poly_aead.cpp:91 in 97b768f952 outdated

92-    // (throws away 32 unused bytes (upper 32) from this ChaCha20 round)
93-    m_chacha_main.Seek(0);
94-    m_chacha_main.Crypt(poly_key, poly_key, sizeof(poly_key));
95+    // 1. AAD (the encrypted packet length), use the header-keystream
96+    if (is_encrypt) {
97+        m_chacha_header.Crypt(src, dest, 3);

laanwj commented at 11:52 am on June 2, 2022:

The ChaCha20Poly1305AEAD::Crypt function uses argument order dest, len, src The ChaCha20Forward4064::Crypt function uses argument order input, output, bytes Let’s standardize on one, please.

dhruv commented at 3:43 am on June 13, 2022:

Done. New implementation uses spans and I went with input, output everywhere.

in src/bench/chacha_poly_aead.cpp:19 in 97b768f952 outdated

20 
21-static const unsigned char k1[32] = {0};
22-static const unsigned char k2[32] = {0};
23-
24-static ChaCha20Poly1305AEAD aead(k1, 32, k2, 32);
25+static std::vector<unsigned char> zero_key(32, 0x00);

laanwj commented at 12:06 pm on June 2, 2022:

Having this mutable is slightly risky. Can we add const or constexpr? Edit: ok, this is “only” bench code, but still.

dhruv commented at 3:50 am on June 13, 2022:

Done.

in src/crypto/chacha_poly_aead.cpp:29 in 97b768f952 outdated

25@@ -27,20 +26,53 @@ int timingsafe_bcmp(const unsigned char* b1, const unsigned char* b2, size_t n)
26 
27 #endif // TIMINGSAFE_BCMP
28 
29-ChaCha20Poly1305AEAD::ChaCha20Poly1305AEAD(const unsigned char* K_1, size_t K_1_len, const unsigned char* K_2, size_t K_2_len)
30+ChaCha20Forward4064::ChaCha20Forward4064(const Span<unsigned char> key)

laanwj commented at 12:09 pm on June 2, 2022:

If your intent is to have the contents of the Span const (which I think it is), this should be Span<const unsigned char>. (more of these below)

dhruv commented at 3:45 am on June 13, 2022:

Done across the board.

Reduce wasted pseudorandom bytes in ChaCha20 467cacc8f1

RFC8439 nonce and counter for ChaCha20 c1a303c281

RFC8439 implementation and tests d634b99a0a

Adding forward secure FSChaCha20 aba4f87012

BIP324 Cipher Suite aaf516682c

RFC8439Encrypt can take multiple plaintexts b6166568a3

in src/crypto/chacha_poly_aead.h:133 in 97b768f952 outdated

194         destlen, length of the destination buffer
195         src, the AAD+payload to encrypt or the AAD+payload+MAC to decrypt
196         src_len, the length of the source buffer
197         is_encrypt, set to true if we encrypt (creates and appends the MAC instead of verifying it)
198+
199+        Returns true if encipher succeeds. Upon failure, the data at dest should not be used.

laanwj commented at 12:12 pm on June 2, 2022:

Is “encipher” a word? If it means the same, I prefer “encrypt or decrypt”.

dhruv commented at 3:45 am on June 13, 2022:

Done.

dhruv force-pushed on Jun 13, 2022

dhruv commented at 3:53 am on June 13, 2022: member

First commit is from #25354 and should be reviewed there. This PR just depends on it.

Updated the implementation of the BIP324 Cipher Suite to be in accordance with the latest draft in the working group(still pending public release). The changes to intent and code from the original PR are large and since I didn’t originally open this PR, I can’t change the PR description. I suspect it makes sense to close this PR and open a new one, but we’d lose the history. I’d welcome thoughts on that from reviewers and maintainers.

Ready for further review.

dhruv commented at 3:42 pm on June 13, 2022: member

Superseded by #25361 because:

I’d like to keep the PR description up to date myself
The code and the purpose of the PR have deviated significantly

dhruv closed this on Jun 13, 2022

dhruv moved this from the "Needs review" to the "Closed unmerged (Superseded)" column in a project

DrahtBot locked this on Jun 13, 2023

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

Conflicts