Skip to content

crypto: drop 16 byte key support for ChaCha20 #25712

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

crypto: drop 16 byte key support for ChaCha20 #25712

wants to merge 1 commit into from

Conversation

theStack
Copy link
Contributor

This PR is an alternative to #25698; as suggested in comment #25698 (comment), the support for 16 byte keys for ChaCha20 can likely be dropped in order to simplify the implementation, i.e. only 32 bytes keys are supported then. To keep the diff minimal and to avoid having to change the callers, the length parameter is still kept right now; one option would be to change the parameter to a fixed-size type, e.g. something like std::array<std::byte, 32>.

@theStack theStack mentioned this pull request Jul 26, 2022
Closed
@fanquake
Copy link
Member

To keep the diff minimal and to avoid having to change the callers,

I don't think there's any point keeping redundant code for the sake of a smaller diff. Someone is just going to open a PR to remove the code later on. Might as well remove it here.

@theStack
Copy link
Contributor Author

To keep the diff minimal and to avoid having to change the callers,

I don't think there's any point keeping redundant code for the sake of a smaller diff. Someone is just going to open a PR to remove the code later on. Might as well remove it here.

Sure, but before tackling that I'd want to 1) have consensus first that there's really no need for 16-byte keys and 2) discuss which exact interface would make sense (e.g. only removing the size parameter and just keeping the pointer as input parameter seems to be fishy and dangerous; the burden of the length check would just move to all callers).

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 26, 2022 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26153 (Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa)
  • #25903 (Use static member functions from class instead of instances by aureleoules)
  • #25698 (crypto: avoid potential buffer overread in ChaCha20::SetKey by theStack)
  • #25361 (BIP324: Cipher suite by dhruv)
  • #25172 (refactor: use std:: prefix for std lib funcs by fanquake)
  • #23561 (BIP324: Handshake prerequisites by dhruv)
  • #23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)
  • #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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.

This was referenced Jul 26, 2022
Closed
Closed
Closed
Closed
Closed
@DrahtBot DrahtBot mentioned this pull request Aug 23, 2022
Closed
sipa
sipa reviewed Aug 23, 2022
View reviewed changes
@theStack theStack force-pushed the 202207-crypto-remove_16byte_key_support_for_chacha20 branch from 88d2b51 to 586330e Compare August 23, 2022 23:36
@theStack
Copy link
Contributor Author

Force-pushed, reduced the input array size from 16 to 12 elements by using the sigma constants directly where previously input[0..3] was accessed, as suggested by sipa. As a consequence, the indices of all other input accesses are reduced by 4 (s/input[4]/input[0]/, s/input[5]/input[1]/ ... s/input[15]/input[11]/).

@maflcko
Copy link
Member

maflcko commented Aug 24, 2022 

fuzz: test/fuzz/crypto_diff_fuzz_chacha20.cpp:317: auto crypto_diff_fuzz_chacha20_fuzz_target(FuzzBufferType)::(anonymous class)::operator()() const: Assertion `output == djb_output' failed.

@theStack theStack force-pushed the 202207-crypto-remove_16byte_key_support_for_chacha20 branch from 586330e to 0f17395 Compare August 25, 2022 01:17
@theStack
Copy link
Contributor Author 

fuzz: test/fuzz/crypto_diff_fuzz_chacha20.cpp:317: auto crypto_diff_fuzz_chacha20_fuzz_target(FuzzBufferType)::(anonymous class)::operator()() const: Assertion `output == djb_output' failed.

This seems to be triggered on the empty ctor-case, when no key has been set yet. Since I don't want to fiddle around too much in the differential fuzz test (setting ctx.input[0..3] to the sigma constant values would probably do the trick), I reverted to a simpler version without input array size change in the latest force-push.

@DrahtBot DrahtBot mentioned this pull request Sep 13, 2022
Closed
@sipa sipa mentioned this pull request Sep 21, 2022
Merged
@theStack
Copy link
Contributor Author

theStack commented Oct 2, 2022

Closed in favour or #26153 (includes removing 16-byte key support).

@theStack theStack closed this Oct 2, 2022
fanquake added a commit that referenced this pull request Feb 15, 2023
@fanquake
Merge #26153: Reduce wasted pseudorandom bytes in ChaCha20 + various …
1e0198b 
…improvements

511aa4f Add unit test for ChaCha20's new caching (Pieter Wuille)
fb243d2 Improve test vectors for ChaCha20 (Pieter Wuille)
93aee8b Inline ChaCha20 32-byte specific constants (Pieter Wuille)
62ec713 Only support 32-byte keys in ChaCha20{,Aligned} (Pieter Wuille)
f21994a Use ChaCha20Aligned in MuHash3072 code (Pieter Wuille)
5d16f75 Use ChaCha20 caching in FastRandomContext (Pieter Wuille)
38eaece Add fuzz test for testing that ChaCha20 works as a stream (Pieter Wuille)
5f05b27 Add xoroshiro128++ PRNG (Martin Leitner-Ankerl)
12ff724 Make unrestricted ChaCha20 cipher not waste keystream bytes (Pieter Wuille)
6babf40 Rename ChaCha20::Seek -> Seek64 to clarify multiple of 64 (Pieter Wuille)
e37bcaa Split ChaCha20 into aligned/unaligned variants (Pieter Wuille)

Pull request description:

  This is an alternative to #25354 (by my benchmarking, somewhat faster), subsumes #25712, and adds additional test vectors.

  It separates the multiple-of-64-bytes-only "core" logic (which becomes simpler) from a layer around which performs caching/slicing to support arbitrary byte amounts. Both have their uses (in particular, the MuHash3072 code can benefit from multiple-of-64-bytes assumptions), plus the separation results in more readable code. Also, since FastRandomContext effectively had its own (more naive) caching on top of ChaCha20, that can be dropped in favor of ChaCha20's new built-in caching.

  I thought about rebasing #25712 on top of this, but the changes before are fairly extensive, so redid it instead.

ACKs for top commit:
  ajtowns:
    ut reACK 511aa4f
  dhruv:
    tACK crACK 511aa4f

Tree-SHA512: 3aa80971322a93e780c75a8d35bd39da3a9ea570fbae4491eaf0c45242f5f670a24a592c50ad870d5fd09b9f88ec06e274e8aa3cefd9561d623c63f7198cf2c7
@bitcoin bitcoin locked and limited conversation to collaborators Oct 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sipa sipa sipa left review comments

Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@theStack @fanquake @DrahtBot @maflcko @sipa