-
Notifications
You must be signed in to change notification settings - Fork 38.6k
BIP324 ciphersuite #28008
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 ciphersuite #28008
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
143dd16 to
1b1ae6f
Compare
1b1ae6f to
ac7483f
Compare
ac43237 to
bc9fecf
Compare
08fe653 to
ac6e198
Compare
ac6e198 to
4c1874d
Compare
4ff7db7 to
5f9705f
Compare
5f9705f to
7714328
Compare
7714328 to
d15c57b
Compare
0b0b07c to
b0b6d11
Compare
| 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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
stratospher
left a comment
There was a problem hiding this 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in #28267.
|
We can track and deal with all remaining comments in one of the followup PRs. |
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
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
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
, 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
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
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 and FSChaCha20Poly1305 implementations are new, taking advance of the improvements in #27993.