crypto: avoid potential buffer overread in ChaCha20::SetKey
#25698
Conversation
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.
e941e92 to
132034d
Compare
|
ref: #21781 |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
|
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 |
|
Closed in favour or #26153 (includes removing 16-byte key support). |
The current interface description of
ChaCha20::SetKeysuggests that literally any key-length is supported (commentset key with flexible keylength; 256bit recommended), while passing in a key with a length less than 16 bytes would lead to a buffer overread:bitcoin/src/crypto/chacha20.cpp
Lines 30 to 33 in 5057adf
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:
bitcoin/src/crypto/chacha20.cpp
Lines 34 to 39 in 5057adf
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.