search

orion-rs / orion

Usable, easy and safe pure-Rust crypto
MIT License
545 stars 30 forks source link

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

Closed briansmith closed 8 months ago

briansmith commented 8 months ago

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.

brycx commented 8 months ago

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.