Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BIP324: Cipher suite #25361

Closed
wants to merge 5 commits into from
Closed

BIP324: Cipher suite #25361

wants to merge 5 commits into from

Conversation

dhruv
Copy link
Contributor

@dhruv dhruv commented Jun 13, 2022

This PR supersedes #20962 and introduces a two-layered cipher suite used in the latest draft of BIP324.

  • Inner layer uses RFC8439 which comes with a formal security analysis so any novelty introduced by our cipher suite still offers a baseline confidence in confidentiality and authenticity. The RFC8439 instance is re-keyed every 256 messages for forward secrecy.
  • Outer layer uses a forward secure version of ChaCha20, FSChaCha20 which re-keys itself every 256 messages for forward secrecy. It is used to encrypt the message length resulting in a pseudorandom byte stream.

The dependency tree for BIP324 PRs is here.

@dhruv
Copy link
Contributor Author

dhruv commented Jun 25, 2022

Rebased

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 25, 2022

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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27479 (BIP324: ElligatorSwift integrations by sipa)
  • #26684 (bench: add readblock benchmark by andrewtoth)

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
Copy link
Contributor Author

dhruv commented Jun 30, 2022

Found necessary fixes from sanitizers, fuzz tests and unit tests in downstream branches. Pushed those changes. Ready for further review.

std::vector<unsigned char> keystream;
keystream.resize(CHACHA20_ROUND_OUTPUT);
c20.Keystream(keystream.data(), CHACHA20_ROUND_OUTPUT);
BOOST_CHECK_EQUAL(HexStr(keystream).substr(0, hex_expected_keystream.size()), hex_expected_keystream);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any particular reason why the entire CHACHA20_ROUND_OUTPUT(64 bytes of keystream) is not being compared? for example, the test vectors in L571-L576 compare only first 32 bytes of keystream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason other than I'm using the test vectors as-is from the hyperlink in the comment before those vectors.

@dhruv
Copy link
Contributor Author

dhruv commented Jul 6, 2022

Updated to correctly rekey using CSHA256 instead of CHash256. Thanks for the catch, @stratospher !

@dhruv
Copy link
Contributor Author

dhruv commented Sep 11, 2022

Updated to:

  • Expose the RFC8439 AAD via the BIP324CipherSuite interface.
  • Remove multiple plaintext interface from RFC8439 -- it ended up not being super helpful to get to the optimization I was trying
  • No longer expose the mac tag from RFC8439 - the tag is a detail internal to the AEAD and the ciphertext + mac is now treated as a blob

Ready for further review.

private:
ChaCha20 c20;
size_t rekey_interval;
uint32_t messages_with_key;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9d9ffc4: naming it as a counter would be more intuitive.

Suggested change
uint32_t messages_with_key;
uint32_t message_counter;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

{
// check buffer boundaries
if (
// if we encrypt, make sure the destination has the space for the length field, header, ciphertext and MAC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

361e971:

Suggested change
// if we encrypt, make sure the destination has the space for the length field, header, ciphertext and MAC
// if we encrypt, make sure the destination has the space for the length field, header, payload ciphertext and MAC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment on lines 75 to 76
std::vector<std::byte> input_vec;
input_vec.resize(BIP324_HEADER_LEN + input.size());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

361e971: input_vec can be defined directly with the required size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment on lines 47 to 48
rekey_ctr{0},
msg_ctr{0},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use class initializer instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

static constexpr size_t REKEY_INTERVAL = 256; // messages
static constexpr size_t NONCE_LENGTH = 12; // bytes

enum BIP324HeaderFlags : uint8_t {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use the scoped enum class instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's useful to have the implicit type conversion here for fuzz testing as the header flags are a byte that a peer could stuff with arbitrary bits.

Comment on lines 73 to 74
messages_with_key{0},
rekey_counter{0},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use class initializer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -5,6 +5,7 @@
#include <assert.h>
#include <bench/bench.h>
#include <crypto/rfc8439.h>
#include <span.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

361e971: changes in src/bench/rfc8439.cpp, src/crypto/rfc8439.h, src/test/fuzz/crypto_rfc8439.cpp can be moved to the previous previous commit. (a2a038a)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done where applicable.

@dhruv
Copy link
Contributor Author

dhruv commented Feb 2, 2023

Rebased. Brought in changes from #26153.

@dhruv
Copy link
Contributor Author

dhruv commented Feb 21, 2023

Rebased.

src/crypto/bip324_suite.cpp Outdated Show resolved Hide resolved
src/crypto/rfc8439.cpp Outdated Show resolved Hide resolved
@dhruv
Copy link
Contributor Author

dhruv commented Mar 21, 2023

Addressed review by @theStack.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed review by @theStack.

Thanks! Another (non-blocking) suggestion: considering that the keys for RFC8439 have a fixed size, would it be worth it to introduce a RFC8439Key type that uses std::array<std::byte, RFC8439_KEYLEN>? I feel that's the slightly cleaner interface with enforcing the length already at compile-time, and we could remove the asserts in the RFC8439Encrypt/RFC8439Decrypt functions. The only annoying part is that the test/benchmark/fuzz code gets a bit longer, as one needs to convert from vector to array, and for the zero_key, one can't use the comfortable constructor for initializing the object with a repeated count of items.

}

void ComputeRFC8439Tag(const std::array<std::byte, POLY1305_KEYLEN>& polykey,
Span<const std::byte> aad, Span<const std::byte> ciphertext,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const-correctness

Suggested change
Span<const std::byte> aad, Span<const std::byte> ciphertext,
const Span<const std::byte> aad, const Span<const std::byte> ciphertext,

return polykey;
}

void RFC8439Crypt(ChaCha20& c20, const Span<const std::byte> in_bytes, Span<std::byte> out_bytes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: seems like this function is never needed in another module, also not in follow-up PRs (checked at #24545)

Suggested change
void RFC8439Crypt(ChaCha20& c20, const Span<const std::byte> in_bytes, Span<std::byte> out_bytes)
static void RFC8439Crypt(ChaCha20& c20, const Span<const std::byte> in_bytes, Span<std::byte> out_bytes)

#ifndef RFC8439_TIMINGSAFE_BCMP
#define RFC8439_TIMINGSAFE_BCMP

int rfc8439_timingsafe_bcmp(const unsigned char* b1, const unsigned char* b2, size_t n)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 7a9d2fb:
It is fairly awkward to introduce a second copy of timingsafe_bcmp (one already in chacha_poly_aead), with a different name, and broken #ifdef logic, just to then rename it when you delete the original timingsafe_bcmp in a later commit. Although I guess re-using/extracting timingsafe_bcmp early from the to-be-deleted file is also a bit awkward.


void RFC8439Encrypt(const Span<const std::byte> aad, const Span<const std::byte> key, const std::array<std::byte, 12>& nonce, const Span<const std::byte> plaintext, Span<std::byte> output);

// returns false if authentication fails
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 7a9d2fb: Make these Doxygen comments?

#include <cstddef>
#include <vector>

constexpr static size_t RFC8439_KEYLEN = 32;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 7a9d2fb: These could be

Suggested change
constexpr static size_t RFC8439_KEYLEN = 32;
constexpr static size_t RFC8439_KEYLEN{32};

// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <assert.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 7a9d2fb: nit use c++ headers, i.e <cassert>

// Copyright (c) 2019-2020 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: additional newline

#include <assert.h>
#include <bench/bench.h>
#include <crypto/bip324_suite.h>
#include <crypto/rfc8439.h> // for the RFC8439_EXPANSION constant
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't add for xyz comments. They are unmaintainable, and if someone wants to know why an include is used, they can look at IWYU etc. Note that you could also add newly-added files to ci/test/06-script_b.sh to have the includes checked.


std::vector<std::byte> header_and_contents(BIP324_HEADER_LEN + input.size());

memcpy(header_and_contents.data(), &flags, BIP324_HEADER_LEN);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
memcpy(header_and_contents.data(), &flags, BIP324_HEADER_LEN);
std::memcpy(header_and_contents.data(), &flags, BIP324_HEADER_LEN);

Same for memset etc.

@@ -127,7 +142,7 @@ inline void ChaCha20Aligned::Keystream64(unsigned char* c, size_t blocks)
x15 += j15;

++j12;
if (!j12) ++j13;
if (!j12 && !is_rfc8439) ++j13;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a need for these is_rfc8439 values, because overflowing the block counter in RFC8439 mode is simply not allowed. If it happens, both incrementing j13 or not incrementing it are wrong. If you want to do anything, it should be an assertion failure.

static constexpr uint64_t PLAINTEXT_SIZE_SMALL = 256;
static constexpr uint64_t PLAINTEXT_SIZE_LARGE = 1024 * 1024;

static std::vector<std::byte> zero_key(32, std::byte{0x00});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this variable can be constant (and if it isn't, it probably shouldn't be a global). Also, globals should be upper case.


static std::vector<std::byte> zero_key(32, std::byte{0x00});
static std::vector<std::byte> aad(AAD_SIZE, std::byte{0x00});
std::array<std::byte, 12> nonce = {std::byte{0x00}, std::byte{0x01}, std::byte{0x02}, std::byte{0x03},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static const here as well?

@fanquake
Copy link
Member

fanquake commented May 6, 2023

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

@fanquake fanquake closed this May 6, 2023
@sipa sipa mentioned this pull request May 12, 2023
43 tasks
achow101 added a commit that referenced this pull request Jul 12, 2023
0bf8747 test: add ChaCha20 test triggering 32-bit block counter overflow (Sebastian Falbesoner)
7f2a985 tests: improve ChaCha20 unit tests (Pieter Wuille)
511a8d4 crypto: Implement RFC8439-compatible variant of ChaCha20 (Pieter Wuille)

Pull request description:

  Based on and replaces part of #25361, part of the BIP324 project (#27634). See also #19225 for background.

  There are two variants of ChaCha20 in use. The currently implemented one uses a 64-bit nonce and a 64-bit block counter, while the one used in RFC8439 (and thus BIP324) uses a 96-bit nonce and 32-bit block counter. This PR changes the logic to use the 96-bit nonce variant, though in a way that's compatible with >256 GiB output (by automatically incrementing the first 32-bit part of the nonce when the block counter overflows).

  For those who reviewed the original PR, the biggest change is here that the 96-bit nonce is passed as a Nonce96 type (pair of 32-bit + 64-bit integer) rather than a 12-byte array.

ACKs for top commit:
  achow101:
    ACK 0bf8747
  theStack:
    Code-review ACK 0bf8747

Tree-SHA512: 62e4cbd5388b8d50ef1a0dc99b6f4ad36c7b4419032035f8e622dda63a62311dd923032217e20054bcd836865d4be5c074f9e5538ca158f94f08eab75c5519c1
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 12, 2023
0bf8747 test: add ChaCha20 test triggering 32-bit block counter overflow (Sebastian Falbesoner)
7f2a985 tests: improve ChaCha20 unit tests (Pieter Wuille)
511a8d4 crypto: Implement RFC8439-compatible variant of ChaCha20 (Pieter Wuille)

Pull request description:

  Based on and replaces part of bitcoin#25361, part of the BIP324 project (bitcoin#27634). See also bitcoin#19225 for background.

  There are two variants of ChaCha20 in use. The currently implemented one uses a 64-bit nonce and a 64-bit block counter, while the one used in RFC8439 (and thus BIP324) uses a 96-bit nonce and 32-bit block counter. This PR changes the logic to use the 96-bit nonce variant, though in a way that's compatible with >256 GiB output (by automatically incrementing the first 32-bit part of the nonce when the block counter overflows).

  For those who reviewed the original PR, the biggest change is here that the 96-bit nonce is passed as a Nonce96 type (pair of 32-bit + 64-bit integer) rather than a 12-byte array.

ACKs for top commit:
  achow101:
    ACK 0bf8747
  theStack:
    Code-review ACK 0bf8747

Tree-SHA512: 62e4cbd5388b8d50ef1a0dc99b6f4ad36c7b4419032035f8e622dda63a62311dd923032217e20054bcd836865d4be5c074f9e5538ca158f94f08eab75c5519c1
achow101 added a commit that referenced this pull request Jul 17, 2023
4e5c933 Switch all callers from poly1305_auth to Poly1305 class (Pieter Wuille)
8871f7d tests: add more Poly1305 test vectors (Pieter Wuille)
40e6c5b crypto: add Poly1305 class with std::byte Span interface (Pieter Wuille)
50269b3 crypto: switch poly1305 to incremental implementation (Pieter Wuille)

Pull request description:

  Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see #27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces:
  * The additionally authenticated data (AAD), padded to 16 bytes.
  * The ciphertext, padded to 16 bytes.
  * The length of the AAD and the length of the ciphertext, together another 16 bytes.

  Implementing RFC8439 using the existing `poly1305_auth` function requires creating a temporary copy with all these pieces of data concatenated just for the purpose of computing the tag (the approach used in #25361).

  This PR replaces the poly1305 code with new code from https://github.com/floodyberry/poly1305-donna (with minor adjustments to make it match our coding style and use our utility functions, documented in the commit) which supports incremental operation, and then adds a C++ wrapper interface using std::byte Spans around it, and adds tests that incremental and all-at-once computation match.

ACKs for top commit:
  achow101:
    ACK 4e5c933
  theStack:
    ACK 4e5c933
  stratospher:
    tested ACK 4e5c933.

Tree-SHA512: df6e9a2a4a38a480f9e4360d3e3def5311673a727a4a85b008a084cf6843b260dc82cec7c73e1cecaaccbf10f3521a0ae7dba388b65d0b086770f7fbc5223e2a
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 19, 2023
…modernize

4e5c933 Switch all callers from poly1305_auth to Poly1305 class (Pieter Wuille)
8871f7d tests: add more Poly1305 test vectors (Pieter Wuille)
40e6c5b crypto: add Poly1305 class with std::byte Span interface (Pieter Wuille)
50269b3 crypto: switch poly1305 to incremental implementation (Pieter Wuille)

Pull request description:

  Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see bitcoin#27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces:
  * The additionally authenticated data (AAD), padded to 16 bytes.
  * The ciphertext, padded to 16 bytes.
  * The length of the AAD and the length of the ciphertext, together another 16 bytes.

  Implementing RFC8439 using the existing `poly1305_auth` function requires creating a temporary copy with all these pieces of data concatenated just for the purpose of computing the tag (the approach used in bitcoin#25361).

  This PR replaces the poly1305 code with new code from https://github.com/floodyberry/poly1305-donna (with minor adjustments to make it match our coding style and use our utility functions, documented in the commit) which supports incremental operation, and then adds a C++ wrapper interface using std::byte Spans around it, and adds tests that incremental and all-at-once computation match.

ACKs for top commit:
  achow101:
    ACK 4e5c933
  theStack:
    ACK 4e5c933
  stratospher:
    tested ACK 4e5c933.

Tree-SHA512: df6e9a2a4a38a480f9e4360d3e3def5311673a727a4a85b008a084cf6843b260dc82cec7c73e1cecaaccbf10f3521a0ae7dba388b65d0b086770f7fbc5223e2a
fanquake added a commit that referenced this pull request Aug 10, 2023
1c7582e tests: add decryption test to bip324_tests (Pieter Wuille)
990f0f8 Add BIP324Cipher, encapsulating key agreement, derivation, and stream/AEAD ciphers (Pieter Wuille)
c91cedf crypto: support split plaintext in ChaCha20Poly1305 Encrypt/Decrypt (Pieter Wuille)
af2b44c bench: add benchmark for FSChaCha20Poly1305 (Pieter Wuille)
aa8cee9 crypto: add FSChaCha20Poly1305, rekeying wrapper around ChaCha20Poly1305 (Pieter Wuille)
0fee267 crypto: add FSChaCha20, a rekeying wrapper around ChaCha20 (Pieter Wuille)
9ff0768 crypto: add the ChaCha20Poly1305 AEAD as specified in RFC8439 (Pieter Wuille)
9fd085a crypto: remove outdated variant of ChaCha20Poly1305 AEAD (Pieter Wuille)

Pull request description:

  Depends on #27985 and #27993, based on and partially replaces #25361, part of #27634. Draft while dependencies are not merged.

  This adds implementations of:
  * The ChaCha20Poly1305 AEAD from [RFC8439 section 2.8](https://datatracker.ietf.org/doc/html/rfc8439#section-2.8), including test vectors.
  * The FSChaCha20 stream cipher as specified in [BIP324](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#rekeying-wrappers-fschacha20poly1305-and-fschacha20), a rekeying wrapper around ChaCha20.
  * The FSChaCha20Poly1305 AEAD as specified in [BIP324](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#rekeying-wrappers-fschacha20poly1305-and-fschacha20), a rekeying wrapper around ChaCha20Poly1305.
  * A BIP324Cipher class that encapsulates key agreement, key derivation, and stream ciphers and AEADs for [BIP324 packet encoding](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#overall-packet-encryption-and-decryption-pseudocode).

  The ChaCha20Poly1305 and FSChaCha20Poly1305 implementations are new, taking advance of the improvements in #27993.

ACKs for top commit:
  jamesob:
    reACK 1c7582e
  theStack:
    ACK 1c7582e
  stratospher:
    tested ACK 1c7582e.

Tree-SHA512: 06728b4b95b21c5b732ed08faf40e94d0583f9d86ff4db3b92dd519dcd9fbfa0f310bc66ef1e59c9e49dd844ba8c5ac06e2001762a804fb5aa97027816045a46
@bitcoin bitcoin locked and limited conversation to collaborators May 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

None yet

7 participants