Skip to content

Conversation

@sipa
Copy link
Member

@sipa sipa commented Jun 29, 2023

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, including test vectors.
  • The FSChaCha20 stream cipher as specified in BIP324, a rekeying wrapper around ChaCha20.
  • The FSChaCha20Poly1305 AEAD as specified in BIP324, a rekeying wrapper around ChaCha20Poly1305.
  • A BIP324Cipher class that encapsulates key agreement, key derivation, and stream ciphers and AEADs for BIP324 packet encoding.

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 29, 2023

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
ACK jamesob, theStack, 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:

  • #28100 (crypto: more Span<std::byte> modernization & follow-ups by sipa)

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.

@sipa sipa mentioned this pull request Jun 29, 2023
43 tasks
@sipa sipa force-pushed the 202306_bip324_ciphersuite branch from 143dd16 to 1b1ae6f Compare June 30, 2023 04:58
@sipa sipa force-pushed the 202306_bip324_ciphersuite branch from 1b1ae6f to ac7483f Compare June 30, 2023 18:06
@sipa sipa force-pushed the 202306_bip324_ciphersuite branch 2 times, most recently from ac43237 to bc9fecf Compare July 7, 2023 18:54
@sipa sipa force-pushed the 202306_bip324_ciphersuite branch 2 times, most recently from 08fe653 to ac6e198 Compare July 7, 2023 21:54
@sipa sipa changed the title BIP324 ciphers: FSChaCha20 and FSChaCha20Poly1305 BIP324 ciphersuite Jul 7, 2023
@sipa sipa force-pushed the 202306_bip324_ciphersuite branch from ac6e198 to 4c1874d Compare July 8, 2023 04:04
@DrahtBot DrahtBot removed the CI failed label Jul 8, 2023
@sipa sipa force-pushed the 202306_bip324_ciphersuite branch 5 times, most recently from 4ff7db7 to 5f9705f Compare July 11, 2023 02:46
@sipa sipa force-pushed the 202306_bip324_ciphersuite branch from 5f9705f to 7714328 Compare July 12, 2023 18:58
@sipa sipa force-pushed the 202306_bip324_ciphersuite branch from 7714328 to d15c57b Compare July 13, 2023 18:09
@sipa sipa marked this pull request as ready for review July 17, 2023 22:43
@sipa sipa force-pushed the 202306_bip324_ciphersuite branch 2 times, most recently from 0b0b07c to b0b6d11 Compare July 17, 2023 22:53
@sipa sipa mentioned this pull request Aug 1, 2023
m_chacha20.SetKey32(UCharCast(new_key));
// Wipe the key (a copy remains inside m_chacha20, where it'll be wiped on the next rekey
// or on destruction).
memory_cleanse(new_key, sizeof(new_key));
Copy link
Member

Choose a reason for hiding this comment

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

In 0fee267 "crypto: add FSChaCha20, a rekeying wrapper around ChaCha20"

We have secure_allocator which handles this cleaning when the object is destroyed, could we use that instead of having to remember to explicitly memory_cleanse secret data?

Copy link
Member Author

@sipa sipa Aug 13, 2023

Choose a reason for hiding this comment

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

@achow101 We have a zero_after_free_allocator, which sounds like what you're describing. The secure_allocator goes a step further an allocates from a pool of non-swappable memory pages on supporting systems.

  • The secure_allocator's behavior does sound overkill for the BIP324 purpose, I feel, and it may be counterproductive even because non-swappable memory is limited, and we really want it for our high-quality RNG and wallet keys.
  • The zero_after_free_allocator's behavior is what we want, but actually using an allocator still comes with the additional cost that it would mean FSChaCha20 objects would require an additional heap allocation every time the keys are cycled. I feel that doesn't weigh up against the added ease of understanding that approach brings.

Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

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

tested ACK 1c7582e.

tested it using this branch which uses random inputs from FuzzedDataProvider to check whether the ciphertext produced by this PR and the python BIP code produce the same output.

I also liked the simpler current approach for streaming API changes. the interfaces of the cipher look more efficient and cleaner compared to the previous implementations!

// - Bit 0: whether the ignore bit is set in message
// - Bit 1: whether the responder (0) or initiator (1) sends
// - Bit 2: whether this ciphertext will be corrupted (making it the last sent one)
// - Bit 3-4: controls the maximum aad length (max 511 bytes)
Copy link
Contributor

Choose a reason for hiding this comment

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

990f0f8: aad in the BIP allows upto 4095 max bytes (max garbage length). is this a performance saver too?

Copy link
Member Author

Choose a reason for hiding this comment

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

The (FS)ChaCha20Poly1305 cipher really supports up to $2^{64}-1$ bytes of AAD. It just so happens that we only use it for garbage, and the garbage is limited to 4095 bytes. We need some kind of limit anyway, but it's a fair point that for our purposes, 4095 would be a more natural limit.

Copy link
Contributor

Choose a reason for hiding this comment

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

done in #28267.

@fanquake
Copy link
Member

We can track and deal with all remaining comments in one of the followup PRs.

@fanquake fanquake merged commit b2ec032 into bitcoin:master Aug 10, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 10, 2023
@stratospher
Copy link
Contributor

stratospher commented Aug 13, 2023

we'd want fuzz tests for AEADChaCha20Poly1305 and FSChaCha20Poly1305 right?
wrote one in #28263 and picked up some review suggestions from here in #28267 in case it helps.

stratospher added a commit to stratospher/bitcoin that referenced this pull request Aug 14, 2023
follow-up to bitcoin#28008.
* move `dummy_tag` variable in FSChaCha20Poly1305 crypto_tests
outside of the loop to be reused every time
* use easy to read `cipher.last()` in `AEADChaCha20Poly1305::Decrypt()`
* comment for initiator in `BIP324Cipher::Initialize()`
* systematically damage ciphertext with bit positions in bip324_tests
* use 4095 max bytes for aad in bip324 fuzz test
fanquake added a commit that referenced this pull request Aug 15, 2023
93cb8f0 refactor: add missing headers for BIP324 ciphersuite (stratospher)
d22d5d9 crypto: BIP324 ciphersuite follow-up (stratospher)

Pull request description:

  follow-up to #28008.
  * move `dummy_tag` variable in FSChaCha20Poly1305 crypto_tests outside of the loop to be reused every time
  * use easy to read `cipher.last()` in `AEADChaCha20Poly1305::Decrypt()`
  * comment for initiator in `BIP324Cipher::Initialize()`
  * systematically damage ciphertext with bit positions in bip324_tests
  * use 4095 max bytes for `aad` in bip324 fuzz test

ACKs for top commit:
  fanquake:
    ACK 93cb8f0 - thanks for following up here.

Tree-SHA512: 361f3e226d3168fdef69a2eebe6092cfc04ba14ce009420222e762698001eaf8be69a1138dab0be237964509c2b96a41a0b4db5c1df43ef75062f143c5aa741a
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 17, 2023
93cb8f0 refactor: add missing headers for BIP324 ciphersuite (stratospher)
d22d5d9 crypto: BIP324 ciphersuite follow-up (stratospher)

Pull request description:

  follow-up to bitcoin#28008.
  * move `dummy_tag` variable in FSChaCha20Poly1305 crypto_tests outside of the loop to be reused every time
  * use easy to read `cipher.last()` in `AEADChaCha20Poly1305::Decrypt()`
  * comment for initiator in `BIP324Cipher::Initialize()`
  * systematically damage ciphertext with bit positions in bip324_tests
  * use 4095 max bytes for `aad` in bip324 fuzz test

ACKs for top commit:
  fanquake:
    ACK 93cb8f0 - thanks for following up here.

Tree-SHA512: 361f3e226d3168fdef69a2eebe6092cfc04ba14ce009420222e762698001eaf8be69a1138dab0be237964509c2b96a41a0b4db5c1df43ef75062f143c5aa741a
kwvg added a commit to kwvg/dash that referenced this pull request Mar 5, 2024
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Mar 6, 2024
, bitcoin#28267, bitcoin#28374, bitcoin#28100 (p2p primitives)

b60c493 merge bitcoin#28100: more Span<std::byte> modernization & follow-ups (Kittywhiskers Van Gogh)
c2aa01c merge bitcoin#28374: python cryptography required for BIP 324 functional tests (Kittywhiskers Van Gogh)
7c5edf7 merge bitcoin#28267: BIP324 ciphersuite follow-up (Kittywhiskers Van Gogh)
1b1924e merge bitcoin#28008: BIP324 ciphersuite (Kittywhiskers Van Gogh)
ff54219 merge bitcoin#27993: Make poly1305 support incremental computation + modernize (Kittywhiskers Van Gogh)
d7482eb merge bitcoin#27985: Add support for RFC8439 variant of ChaCha20 (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #5900

  * Dependent on #5901

  * Without modifications, tests introduced in [bitcoin#28008](bitcoin#28008) will fail due to salt comprising of a fixed string (`bitcoin_v2_shared_secret`) and network bytes ([source](https://github.com/sipa/bitcoin/blob/1c7582ead6e1119899922041c1af2b4169b0bc74/src/bip324.cpp#L39-L40)). Bitcoin uses [`{0xf9, 0xbe, 0xb4, 0xd9}`](https://github.com/sipa/bitcoin/blob/1c7582ead6e1119899922041c1af2b4169b0bc74/src/kernel/chainparams.cpp#L114-L117) for mainnet while Dash uses [`{0xbf, 0x0c, 0x6b, 0xbd}`](https://github.com/dashpay/dash/blob/37f43e4e56de0d510110a7f790df34ea77748dc9/src/chainparams.cpp#L238-L241).
    * The replacement parameters are generated by:
      * Cloning https://github.com/bitcoin/bips (as of this writing, at bitcoin/bips@b3701fa)
      * Editing `bip-0324/reference.py`
        * Changing `NETWORK_MAGIC` to Dash's network magic
      * Running `gen_test_vectors.py` to get a new `packet_encoding_test_vectors.csv`
      * Using [this python script](https://github.com/bitcoin/bitcoin/blob/1c7582ead6e1119899922041c1af2b4169b0bc74/src/test/bip324_tests.cpp#L174-L196) mentioned in a comment in `src/test/bip324_tests.cpp`, generate the values that will be used to replace the ones in `bip324_tests.cpp` (it will print to `stdout` so it's recommended to pipe it to a file)
      * Paste the new values over the old ones

  ## Breaking Changes

  None. Changes are restricted to BIP324 cryptosuite, tests and associated logic.

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: bb056de8588026adae63e78d56878274ff3934a439e2eae81606fa9c0a37dab432a129315bb9c1b754400b5c883bf460eea3a0c857a3be0816c8fbf55c479843
fanquake added a commit that referenced this pull request Jul 16, 2024
8607773 Add fuzz test for FSChaCha20Poly1305 (stratospher)
c807f33 Add fuzz test for AEADChacha20Poly1305 (stratospher)

Pull request description:

  This PR adds fuzz tests for `AEADChaCha20Poly1305` and `FSChaCha20Poly1305` introduced in #28008.

  Run using:
  ```
  $ FUZZ=crypto_aeadchacha20poly1305 src/test/fuzz/fuzz
  $ FUZZ=crypto_fschacha20poly1305 src/test/fuzz/fuzz
  ```

ACKs for top commit:
  dergoegge:
    tACK 8607773
  marcofleon:
    Tested ACK 8607773. Ran both targets for ~200 CPU hours. Coverage of intended targets looks good to me. The simulation of damaged keys and checks that follow seem useful as well.

Tree-SHA512: b6b85661d896e653caeed330f941fde665fc2bbd97ecd340808a3f365c469fe9134aa77316569a771dc36d1158cac1a5f76700bcfc45fff12aef07562e48feb9
@bitcoin bitcoin locked and limited conversation to collaborators Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants