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

jonasschnelli · 2021-01-19T10:06:25Z

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

DrahtBot · 2021-01-24T23:59:36Z

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.

jonasschnelli · 2021-01-29T08:47:43Z

Rebased

PastaPastaPasta

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 · 2021-05-23T17:36:26Z

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

dhruv

Approach ACK c22f607

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.

src/crypto/chacha_poly_aead.h

dhruv · 2021-06-26T15:27:30Z

src/crypto/chacha_poly_aead.h

+*
+* When receiving a packet, the length must be decrypted first. When 3 bytes of
+* ciphertext length have been received, they MUST be decrypted.
+*


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.

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.

src/crypto/chacha_poly_aead.h

dhruv · 2021-06-28T18:01:16Z

src/test/crypto_tests.cpp

-    BOOST_CHECK_EQUAL(len_cmp, expected_aad_length);
-
-    // encrypt / decrypt 1000 packets
+    BOOST_CHECK(aead_in.DecryptLength(&out_len, ciphertext_buf.data()));


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?

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 · 2021-08-23T20:39:25Z

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

stratospher

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

stratospher · 2021-08-25T18:23:49Z

src/crypto/chacha_poly_aead.h

@@ -6,6 +6,7 @@
 #define BITCOIN_CRYPTO_CHACHA_POLY_AEAD_H

 #include <crypto/chacha20.h>
+#include <crypto/poly1305.h>

 #include <cmath>



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

Good catch. Fixed.

stratospher · 2021-08-25T18:33:58Z

src/crypto/chacha_poly_aead.cpp

+    m_ctx.SetKey(key, keylen);
+
+    // set initial sequence number
+    m_seqnr = 0;


Since we're already initialising m_seqnr with 0 in the class definition of ChaCha20Forward4064,

bitcoin/src/crypto/chacha_poly_aead.h

Line 107 in 424e010

uint64_t m_seqnr{0};

Couldn't we remove this line from the constructor?

stratospher · 2021-08-25T18:35:28Z

src/crypto/chacha_poly_aead.cpp

+
+    // precompute first chunk of keystream
+    m_ctx.Keystream(m_keystream, KEYSTREAM_SIZE);
+    m_keystream_pos = 0;


Since we're already initialising m_keystream_pos with 0 in the class definition of ChaCha20Forward4064,

bitcoin/src/crypto/chacha_poly_aead.h

Line 108 in 424e010

size_t m_keystream_pos{0};

Couldn't we remove this line from the constructor?

stratospher · 2021-08-25T18:44:41Z

src/crypto/chacha_poly_aead.cpp

+    assert(K_1_len == CHACHA20_POLY1305_AEAD_KEY_LEN);
+    assert(K_2_len == CHACHA20_POLY1305_AEAD_KEY_LEN);


We are checking whether the key length is 32 on this line

bitcoin/src/crypto/chacha_poly_aead.cpp

Line 33 in 424e010

assert(keylen == 32);

when the constructors of m_chacha_header and m_chacha_main get called. So couldn't we remove these 2 lines?

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.

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

dhruv · 2021-09-12T21:48:47Z

Comments from @stratospher addressed. Ready for further review.

dhruv · 2021-10-10T04:52:13Z

Addressed #23233 (comment) - ready for further review

stratospher

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 PRF Poly1305 MAC ChaCha20Forward4064-Poly1305@Bitcoin cipher suite 2 separate instances of ChaCha20Forward4064 DRBG using keys k1 and k2 called F and V respectively.	Defined in: chacha20.h poly1305.h chacha_poly_aead.h F 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.

stratospher · 2021-10-15T14:11:57Z

src/crypto/chacha_poly_aead.h

@@ -6,13 +6,13 @@
 #define BITCOIN_CRYPTO_CHACHA_POLY_AEAD_H

 #include <crypto/chacha20.h>
+#include <crypto/poly1305.h>

 #include <cmath>


#include <cmath> isn't being used.

stratospher · 2021-10-15T14:14:46Z

src/crypto/chacha_poly_aead.h

+* a decryption oracle can learn nothing about the payload contents or its MAC
+* (assuming key derivation, ChaCha20 and Poly1305 are secure). Active observers
+* can still obtain the message length (ex. active ciphertext bit flipping or
+* traffic shemantics analysis)


a typo for semantics? :)

stratospher · 2021-10-15T14:25:50Z

src/crypto/chacha_poly_aead.cpp

@@ -4,6 +4,7 @@

 #include <crypto/chacha_poly_aead.h>

+#include <crypto/common.h>
 #include <crypto/poly1305.h>
 #include <support/cleanse.h>



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

#include <string.h>

#include <cstdio>

#include <limits>

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

stratospher · 2021-10-15T15:17:00Z

src/test/fuzz/crypto_chacha20_poly1305_aead.cpp

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


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

#include <cassert>

#include <limits>

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?

stratospher · 2021-10-15T15:18:10Z

src/bench/chacha_poly_aead.cpp

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


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

stratospher · 2021-10-15T15:22:01Z

src/bench/chacha_poly_aead.cpp

-            const bool get_length_ok = aead.GetLength(&len, seqnr_aad, aad_pos, in.data());
-            assert(get_length_ok);
-            const bool crypt_ok_2 = aead.Crypt(seqnr_payload, seqnr_aad, aad_pos, out.data(), out.size(), in.data(), buffersize, true);
+            auto len = aead_in.DecryptLength(out.data());


This comment suggestion can be applied here too.

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

dhruv · 2021-10-22T18:15:22Z

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.

dhruv · 2022-01-21T04:46:59Z

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

#23233 (comment)
#23233 (comment)

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.

dhruv · 2022-02-02T02:50:12Z

Rebased. Ready for further review.

git range-diff 02e1d8d06f 41deee4 8b474d1

jonatack · 2022-03-03T15:28:19Z

Concept ACK, will review soon.

dhruv · 2022-03-21T20:12:08Z

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.

laanwj · 2022-06-02T11:52:17Z

src/crypto/chacha_poly_aead.cpp

-    m_chacha_main.Crypt(poly_key, poly_key, sizeof(poly_key));
+    // 1. AAD (the encrypted packet length), use the header-keystream
+    if (is_encrypt) {
+        m_chacha_header.Crypt(src, dest, 3);


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.

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

laanwj · 2022-06-02T12:06:30Z

src/bench/chacha_poly_aead.cpp

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


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

laanwj · 2022-06-02T12:09:10Z

src/crypto/chacha_poly_aead.cpp

@@ -27,20 +26,53 @@ int timingsafe_bcmp(const unsigned char* b1, const unsigned char* b2, size_t n)

 #endif // TIMINGSAFE_BCMP

-ChaCha20Poly1305AEAD::ChaCha20Poly1305AEAD(const unsigned char* K_1, size_t K_1_len, const unsigned char* K_2, size_t K_2_len)
+ChaCha20Forward4064::ChaCha20Forward4064(const Span<unsigned char> key)


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)

Done across the board.

laanwj · 2022-06-02T12:12:39Z

src/crypto/chacha_poly_aead.h

        dest, output buffer, must be of a size equal or larger then CHACHA20_POLY1305_AEAD_AAD_LEN + payload (+ POLY1305_TAG_LEN in encryption) bytes
        destlen, length of the destination buffer
        src, the AAD+payload to encrypt or the AAD+payload+MAC to decrypt
        src_len, the length of the source buffer
        is_encrypt, set to true if we encrypt (creates and appends the MAC instead of verifying it)
+
+        Returns true if encipher succeeds. Upon failure, the data at dest should not be used.


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

dhruv · 2022-06-13T03:53:10Z

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 · 2022-06-13T15:42:09Z

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

jonasschnelli added the P2P label Jan 19, 2021

jonasschnelli assigned sipa Jan 19, 2021

jonasschnelli mentioned this pull request Jan 19, 2021

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

Closed

DrahtBot mentioned this pull request Jan 24, 2021

fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer #21000

Merged

DrahtBot added the Needs rebase label Jan 26, 2021

jonasschnelli force-pushed the 2020/12/aead_v2 branch from 8579374 to c22f607 Compare January 29, 2021 08:47

DrahtBot removed the Needs rebase label Jan 29, 2021

PastaPastaPasta approved these changes Apr 13, 2021

View reviewed changes

DrahtBot mentioned this pull request Jun 23, 2021

crypto: Fix K1/K2 use in ChaCha20-Poly1305 AEAD #22331

Merged

1 task

dhruv reviewed Jun 28, 2021

View reviewed changes

DrahtBot added the Needs rebase label Aug 19, 2021

dhruv force-pushed the 2020/12/aead_v2 branch 2 times, most recently from 1a868b1 to 424e010 Compare August 23, 2021 20:05

DrahtBot removed the Needs rebase label Aug 23, 2021

stratospher reviewed Aug 25, 2021

View reviewed changes

dhruv force-pushed the 2020/12/aead_v2 branch from 424e010 to 8a9daf5 Compare September 12, 2021 21:46

stratospher mentioned this pull request Sep 14, 2021

fuzz: Invoking python interpreter from a C++ file #22973

Closed

dhruv mentioned this pull request Oct 8, 2021

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

Closed

dhruv force-pushed the 2020/12/aead_v2 branch from 8a9daf5 to 0b93e3d Compare October 10, 2021 03:40

DrahtBot mentioned this pull request Oct 14, 2021

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

Merged

stratospher reviewed Oct 15, 2021

View reviewed changes

DrahtBot added the Needs rebase label Oct 21, 2021

dhruv force-pushed the 2020/12/aead_v2 branch from 0b93e3d to 0de3cb9 Compare October 22, 2021 18:09

stratospher mentioned this pull request Nov 5, 2021

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

Closed

ryihan approved these changes Nov 6, 2021

View reviewed changes

dhruv force-pushed the 2020/12/aead_v2 branch from 6a651b5 to 41deee4 Compare January 21, 2022 04:39

DrahtBot mentioned this pull request Jan 30, 2022

doc: Fix typos pointed out by lint-spelling #24203

Merged

DrahtBot added the Needs rebase label Jan 31, 2022

dhruv force-pushed the 2020/12/aead_v2 branch from 41deee4 to 8b474d1 Compare February 2, 2022 02:50

DrahtBot removed the Needs rebase label Feb 2, 2022

dhruv mentioned this pull request Mar 12, 2022

BIP324: Enable v2 P2P encrypted transport #24545

Closed

dhruv force-pushed the 2020/12/aead_v2 branch from 8b474d1 to 97b768f Compare March 21, 2022 20:08

DrahtBot mentioned this pull request May 20, 2022

refactor: use std:: prefix for std lib funcs #25172

Closed

laanwj reviewed Jun 2, 2022

View reviewed changes

dhruv added 6 commits June 12, 2022 14:54

Reduce wasted pseudorandom bytes in ChaCha20

467cacc

RFC8439 nonce and counter for ChaCha20

c1a303c

RFC8439 implementation and tests

d634b99

Adding forward secure FSChaCha20

aba4f87

BIP324 Cipher Suite

aaf5166

RFC8439Encrypt can take multiple plaintexts

b616656

dhruv force-pushed the 2020/12/aead_v2 branch from 97b768f to b616656 Compare June 13, 2022 03:50

dhruv mentioned this pull request Jun 13, 2022

BIP324: Cipher suite #25361

Closed

dhruv closed this Jun 13, 2022

bitcoin locked and limited conversation to collaborators Jun 13, 2023

		assert(K_1_len == CHACHA20_POLY1305_AEAD_KEY_LEN);
		assert(K_2_len == CHACHA20_POLY1305_AEAD_KEY_LEN);

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

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

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

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

Conversation

jonasschnelli commented Jan 19, 2021 • edited by dhruv

DrahtBot commented Jan 24, 2021 • edited

Conflicts

jonasschnelli commented Jan 29, 2021

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented May 23, 2021

dhruv 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

dhruv commented Aug 23, 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

dhruv commented Sep 12, 2021

dhruv commented Oct 10, 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

dhruv Oct 22, 2021 • edited

Choose a reason for hiding this comment

stratospher Oct 15, 2021 • edited

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 Oct 22, 2021

dhruv commented Jan 21, 2022

dhruv commented Feb 2, 2022 • edited

jonatack commented Mar 3, 2022

dhruv commented Mar 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj Jun 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj Jun 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj Jun 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruv commented Jun 13, 2022 • edited

dhruv commented Jun 13, 2022

jonasschnelli commented Jan 19, 2021 •

edited by dhruv

DrahtBot commented Jan 24, 2021 •

edited

dhruv Oct 22, 2021 •

edited

stratospher Oct 15, 2021 •

edited

dhruv commented Feb 2, 2022 •

edited

dhruv commented Mar 21, 2022 •

edited

laanwj Jun 2, 2022 •

edited

laanwj Jun 2, 2022 •

edited

laanwj Jun 2, 2022 •

edited

dhruv commented Jun 13, 2022 •

edited