crypto: avoid potential buffer overread in ChaCha20::SetKey #25698

1. 
   theStack commented at 3:00 pm on July 25, 2022: contributor

   The current interface description of ChaCha20::SetKey suggests that literally any key-length is supported (comment set key with flexible keylength; 256bit recommended), while passing in a key with a length less than 16 bytes would lead to a buffer overread:

   https://github.com/bitcoin/bitcoin/blob/5057adf22fc4c3593e1e633defeda96be508f198/src/crypto/chacha20.cpp#L30-L33

   This PR adds an assert to only allowing sizes of 16 or 32 bytes (i.e. 128 and 256 bits, respectively) which seem what was intended originally, looking at the following piece of code:

   https://github.com/bitcoin/bitcoin/blob/5057adf22fc4c3593e1e633defeda96be508f198/src/crypto/chacha20.cpp#L34-L39

   The fuzz tests are adopted accordingly to pick either 16 or 32 rather than a length in the range of 16 to 32. An alternative would be to just assert for the minimum size of 16 (and a maximum size of 32), if for some reason supporting keys with a length between 17 and 31 bytes is important; I guess it isn’t though, it would also be confusing as the extra bytes wouldn’t be used.

2. MarcoFalke added the label Refactoring on Jul 25, 2022
3. in src/crypto/chacha20.cpp:11 in e941e92932 outdated

    7@@ -8,6 +8,7 @@
    8 #include <crypto/common.h>
    9 #include <crypto/chacha20.h>
   10 
   11+#include <assert.h>
   

   ---

   ---

   

   MarcoFalke commented at 3:06 pm on July 25, 2022:

   nit:

   0#include <cassert>
   

   ---

   

   theStack commented at 3:14 pm on July 25, 2022:

   Thanks, done. For some reason there is a strong tendency towards including .h headers from the C standard library in the crypto modules. Changed to cassert now (also from string.h to cstring).
4. MarcoFalke approved
5. 
   MarcoFalke commented at 3:06 pm on July 25, 2022: member

   lgtm
6. crypto: avoid potential buffer overread in `ChaCha20::SetKey`

   The current interface description of `ChaCha20::SetKey` suggests that
   literally any key-length is supported, while passing in a key with a
   length less than 16 bytes would lead to a buffer overread. Only allowing
   sizes of 16 or 32 bytes (i.e. 128 and 256 bits, respectively) seem to be
   what was intended originally, so assert to that and adapt the fuzz that
   accordingly to pick either 16 or 32 rather than a length in the range of
   16 to 32.

   132034dcae
7. theStack force-pushed on Jul 25, 2022
8. 
   MarcoFalke commented at 3:40 pm on July 25, 2022: member

   ref: https://github.com/bitcoin/bitcoin/pull/21781
9. 
   DrahtBot commented at 6:23 pm on July 25, 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)
   • #25712 (crypto: drop 16 byte key support for ChaCha20 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.

10. MarcoFalke approved
11. 
    sipa commented at 11:36 am on July 26, 2022: member

    To simplify things, I think we could also just drop support for 16-byte keys and only permit 32-byte ones.
12. 
    theStack commented at 1:45 pm on July 26, 2022: contributor

    To simplify things, I think we could also just drop support for 16-byte keys and only permit 32-byte ones.

    Good idea, opened another pull for that: #25712 I’m still keeping this PR open right now, for the unlikely case that we would still need to support 16-byte keys for some reason.

13. 
    theStack commented at 10:33 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. bitcoin locked this on Oct 2, 2023

Contributors
theStack MarcoFalke DrahtBot sipa

Labels
Refactoring