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

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202207-crypto-avoid_potential_buffer_overread_in_chacha20_setkey changing 4 files +8 −6
  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
  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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me