Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using checked_mul(8) instead of checked_shl(3) in SHA2 #376

Closed
briansmith opened this issue Mar 15, 2024 · 1 comment · Fixed by #377
Closed

Consider using checked_mul(8) instead of checked_shl(3) in SHA2 #376

briansmith opened this issue Mar 15, 2024 · 1 comment · Fixed by #377
Assignees
Labels
improvement General improvements to code

Comments

@briansmith
Copy link

I see in your SHA-2 implementation that you are using let len = length.checked_shl(3).unwrap(); to convert byte length to bit length. This doesn't make sense because x.checked_shl(s) only checks for overflow of s, not of the result, so checked_shl(3) isn't equivalent to checked_mul(8) w.r.t. overflow checking.

@briansmith briansmith added the bug Something isn't working label Mar 15, 2024
@brycx
Copy link
Member

brycx commented Mar 15, 2024

Thanks a lot for taking the time to report this, @briansmith.

I agree, this doesn't really make sense. An overflow should be unreachable on this path from user-input, so as you say it doesn't really make sense to use checked_shl(3) here. We could just use plain <<, but I think checked_mul(8) would better convey the implicit condition of unreachable overflow.

(In case "bug" label was added automatically, please ignore this.) I'm removing the bug-label, as a user cannot trigger an overflow on this path, as far as I'm aware.

@brycx brycx added improvement General improvements to code and removed bug Something isn't working labels Mar 15, 2024
@brycx brycx self-assigned this Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement General improvements to code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants