Ensure that rc5 doesn't try to use a key longer than 2040 bits #8834
Conversation
mattcaswell
added
branch: master
|
Confirmed that this resolves the issue. |
|
Silent truncation seems like the wrong choice to me. It should fail. |
That isn't possible to do without an API break because this is a void function. I did consider that, but it doesn't seem appropriate for a stable branch since this is targeting 1.1.1. Silent truncation seemed like the lesser of two evils. |
| =head1 BUGS | ||
|
|
||
| Currently the number of rounds in RC5 can only be set to 8, 12 or 16. | ||
| This is a limitation of the current RC5 code rather than the EVP interface. |
There was a problem hiding this comment.
This still seems to be the case.
There was a problem hiding this comment.
Yes, but I documented the restriction of 8, 12 or 16 in the section on the EVP_CTRL_SET_RC5_ROUNDS ctrl. I didn't see this as a "bug" so the writing it in a bugs section seemed misplaced. The fact that its a limitation of the rc5 code rather than the evp interface is an implementation detail that doesn't seem relevant to the end user.
|
In 3.0 you could at least change it to return something, that is API compatible. I doubt it's ABI compatible on all our platforms to suddenly return something, so for 1.1 either silent truncation, or just returning would be fine for me. |
Yes, ok. That does make sense. |
|
Will this get merged at some point? https://oss-fuzz.com/testcase-detail/5750176758628352 is still active. |
|
Also see https://oss-fuzz.com/testcase-detail/5672061294346240 (RC5 integer overflow) which might be worth fixing. |
The maximum key length for rc5 is 2040 bits so we should not attempt to use keys longer than this. Issue found by OSS-Fuzz and Guido Vranken.
If the key is too long we now return an error.
|
Thanks for the reminder. I've reworked and rebased this. There are now 2 commits:
The second change should be API compatible but is not ABI compatible. I plan to push both commits to master and only the first commit to the 1.1.1 branch. This means that, in 1.1.1, using RC5 via EVP will correctly fail if the key is too long. Using the low level RC5 functions directly with a key that is too long will still crash (as it does now). |
|
|
||
| =item EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GET_RC5_ROUNDS, 0, &rounds) | ||
|
|
||
| Stores the number of rounds currently configured in B<*rounds> where B<rounds> |
There was a problem hiding this comment.
Shouldn't the second be *rounds too?
Alternatively, say it is a pointer to an int.
|
(side note: I'm unsure if changing a void function to a function returning int is ABI incompatible. Such a change only means that old code ignores the return value, right?) |
|
(ah, except letter downgrades, which is permissible. Never mind) |
|
The return value can actually have an effect on the ABI (not API).
|
|
Pushed with the documentation fixup to master. Also cherry-picked the first commit only to 1.1.1. |
The maximum key length for rc5 is 2040 bits so we should not attempt to use keys longer than this. Issue found by OSS-Fuzz and Guido Vranken. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #8834)
If the key is too long we now return an error. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #8834)
The maximum key length for rc5 is 2040 bits so we should not attempt to
use keys longer than this. We truncate them to 2040 bits instead.
Issue found by OSS-Fuzz and Guido Vranken.