Add support for RFC8439 variant of ChaCha20 #27985
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. |
|
#19225 :-) |
|
@Sjors Thanks, I had forgotten about that. Added a reference. |
sipa
force-pushed
the
202306_chacha20_rfc8439
branch
2 times, most recently
from
eb62589 to
6bb1a89
Compare
|
Made some changes:
|
sipa
force-pushed
the
202306_chacha20_rfc8439
branch
from
6bb1a89 to
200e1a2
Compare
|
I've encapsulated the 96-bit nonce into a |
There was a problem hiding this comment.
Code-review ACK 200e1a2
Verified that the implementation matches the RFC8439 specification (https://datatracker.ietf.org/doc/html/rfc8439#section-2.3) and checked that the test vectors match the ones from the linked sources (https://tools.ietf.org/html/draft-agl-tls-chacha20poly1305-04#section-7 and https://datatracker.ietf.org/doc/html/rfc8439#page-30).
Note for other reviewers (that get as easily confused as me): we don't store the constants in the input array, so our indices are 4 less, e.g. input[8] in our code corresponds to Word 12 in the RFC.
|
Is the current ChaCha20 implementation used for anything that needs backwards compatibility? |
|
@achow101 Good question! It's used by FastRandomContext and MuHash. I haven't investigated whether there are any uses of FastRandomContext that could generate more than 256 GiB of data, but even if there are there it's possible to at least use the new seeking style (the current implementation, when using the 96/32 split, when the block counter overflows, just increments the nonce, which is exactly what we'd want for an RNG anyway). So no - I think all existing uses could be converted to use the 96/32 split, if the overflow behavior is kept. I'm happy to include that in this PR if reviewers prefer that, or I could leave it for a follow-up. |
|
Assuming it doesn't break a whole lot of tests, I would prefer to change it always use the new version as it removes the need to check that everything is using the right variant. |
There are two variants of ChaCha20 in use. The original one uses a 64-bit nonce and a 64-bit block counter, while the one used in RFC8439 uses a 96-bit nonce and 32-bit block counter. This commit changes the interface to use the 96/32 split (but automatically incrementing the first 32-bit part of the nonce when the 32-bit block counter overflows, so to retain compatibility with >256 GiB output). Simultaneously, also merge the SetIV and Seek64 functions, as we almost always call both anyway. Co-authored-by: dhruv <856960+dhruv@users.noreply.github.com>
sipa
force-pushed
the
202306_chacha20_rfc8439
branch
from
200e1a2 to
7f2a985
Compare
|
ACK 7f2a985 This PR fundamentally doesn't change our implementation of ChaCha20, just its interface so that it matches RFC8439. It otherwise behaves in the same way as previously. The new test vectors match those of the RFC. |
There was a problem hiding this comment.
Nice and indeed much simpler now, the actual diff is smaller than I expected. Will re-review tomorrow.
Meanwhile, I thought it would be nice to have a test case triggering the 32-bit block counter overflow to check for >256 GiB output compatibility and wrote one (test data is created with PyCryptodome, see commit message), maybe it's worthwhile to include it here: theStack@61b41ad (the next commit theStack@ff28930 goes one step further and checks if the overflow really happened by inspecting the input state before/after test case execution, but not sure if it's worth it to pollute the ChaCha20 class with test-only methods).
Verify that our ChaCha20 implementation using the 96/32 split interface
is compatible with >256 GiB outputs by triggering a 32-bit block counter
overflow and checking that the keystream matches one created with an
alternative implementation using a 64/64 split interface with the
corresponding input data. The test case data was generated with the
following Python script using the PyCryptodome library (version 3.15.0):
----------------------------------------------------------------------------------------------
from Crypto.Cipher import ChaCha20
key = bytes(list(range(32))); nonce = 0xdeadbeef12345678; pos = 2**32 - 1
c = ChaCha20.new(key=key, nonce=nonce.to_bytes(8, 'little'))
c.seek(pos * 64); stream = c.encrypt(bytes([0])*128)
print(f"Key: {key.hex()}\nNonce: {hex(nonce)}\nPos: {hex(pos)}\nStream: {stream.hex()}")
----------------------------------------------------------------------------------------------
|
@theStack Cherrypicked the first commit. I don't think the second one adds that much, as presumably there is no way the output can be correct if the internal state is wrong. FWIW, the crypto_diff_fuzz_chacha20 fuzz test also tests the overflowing behavior (but still good to have a unit test for it too). |
|
ACK 0bf8747 |
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
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#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
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.