crypto: drop 16 byte key support for ChaCha20 #25712

1. 
   theStack commented at 1:44 pm on July 26, 2022: 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>.
2. 
   fanquake commented at 1:54 pm on July 26, 2022: 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.

3. 
   theStack commented at 2:16 pm on July 26, 2022: contributor

   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).

4. DrahtBot added the label Utils/log/libs on Jul 26, 2022
5. 
   DrahtBot commented at 5:27 pm on July 26, 2022: contributor

   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.

6. in src/crypto/chacha20.cpp:30 in 88d2b510fa outdated

   27 void ChaCha20::SetKey(const unsigned char* k, size_t keylen)
   28 {
   29-    const unsigned char *constants;
   30+    assert(keylen == 32);
   31 
   32+    input[0] = ReadLE32(sigma + 0);
   

   ---

   ---

   

   sipa commented at 5:57 pm on August 23, 2022:

   Could use

   0input[0] = 0x61707865;
   1input[1] = 0x3320646e;
   2input[2] = 0x79622d32;
   3input[3] = 0x6b206574;
   

   Or even better: make the input variable only 12 long, and replace accesses to input 0..3 with these constants.

   ---

   

   theStack commented at 11:48 pm on August 23, 2022:

   Or even better: make the input variable only 12 long, and replace accesses to input 0..3 with these constants.

   Thanks, done.

7. theStack force-pushed on Aug 23, 2022
8. 
   theStack commented at 11:48 pm on August 23, 2022: contributor

   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]/).
9. 
   MarcoFalke commented at 5:48 am on August 24, 2022: member

   0fuzz: 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.
   

10. crypto: drop 16 byte key support for ChaCha20 0f17395123
11. theStack force-pushed on Aug 25, 2022
12. 
    theStack commented at 1:19 am on August 25, 2022: contributor

    0fuzz: 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.

13. 
    theStack commented at 10:34 pm on October 2, 2022: contributor

    Closed in favour or #26153 (includes removing 16-byte key support).
14. theStack closed this on Oct 2, 2022

    ---

15. fanquake referenced this in commit 1e0198b6c1 on Feb 15, 2023
16. bitcoin locked this on Oct 2, 2023

Contributors
theStack fanquake DrahtBot sipa MarcoFalke

Labels
Utils/log/libs