Correct documentation of integer_division_remainder_used

[felix91gr]

1. Barrett Reduction isn't constant-time, so that was eliminated.
2. The "use instead" example didn't solve the timing attack, so it was eliminated as well.
3. Added some context and recommendations for the user.

changelog: [integer_division_remainder_used]: corrected documentation errors

[rustbot]

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[felix91gr]

Notes for the reviewer:

1. I really really wanted to add an example of a proper "use instead" example, but timing attacks are so hard to avoid, especially in non-assembly code, that I added a more realistic recommendation instead.
2. This post might help understand the context of this lint a little bit better: https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html

[felix91gr]

@tarcieri you opened the issue that encouraged this lint to exist :3 can I ask you for feedback on this?

Like... well, basically on the changes proposed:

1. As far as I'm aware, Barrett Reduction isn't constant-time. I know you know your cryptography (understatement), so: did I get it right? 😄
   • What would you suggest to the reader, instead of Barrett Reduction?
   • The shifting example in the original docs is not guaranteed to be constant-time either. I'm guessing there's no "simple" suggestion we could put in the lint, but if you have feedback on what the lint could suggest, please let me know.
2. Do the recommendations I added seem reasonable?

[tarcieri]

Barrett Reduction isn't constant-time, so that was eliminated.

While an implementation of a Barrett Reduction won't necessarily be constant-time, it's certainly possible to implement a Barrett Reduction in constant-time, which is an extremely common technique in cryptography, and such implementations run in a fixed number of CPU cycles. See Algorithm 6) Barrett Reduction modulo p from this paper:

https://csrc.nist.gov/csrc/media/events/workshop-on-elliptic-curve-cryptography-standards/documents/papers/session6-adalier-mehmet.pdf

You can read about our constant-time implementation of a Barrett Reduction for the P-256 scalar field here: https://reports.zksecurity.xyz/reports/near-p256/#analysis-of-the-bound-of-barrett-reduction

And generally it's covered for cryptographic applications in HAC. See section 14.3.3: https://cacr.uwaterloo.ca/hac/about/chap14.pdf

The shifting example in the original docs is not guaranteed to be constant-time either.

...if you're talking about CPUs from the '80s like 80286 that predate barrel shifters, sure. These days even a Cortex-M0 has a barrel shifter. Are there any targets Rust actually supports that have non-constant-time bit shift operations?

FWIW, on the @RustCrypto crates we do include a caveat about non-constant-time multipliers on e.g. PPC32 targets that short circuit on zero. I am not aware of any other specific cases of CPUs that are tier 1-3 rustc targets where operations like addition, multiplication, and bitwise operations including shifts are not constant-time, though looking over the table of Rust Tier 3 targets, I guess there's a msp430-none-elf target that maybe it's worth making a note about.

[felix91gr]

@tarcieri nice! I think I'll be adding a lot of those references to the lint in my next push :3

But first.. I have more questions! If you'd have me ^^

1. How can one defend against compiler optimizations messing up the constant-time characteristics of the algorithm? I'm hesitant to leave recommendations in the lint that could lead to someone implementing something that should be constant-time, but gets wrecked by the compiler. Whatever recommendations it makes, I want to make sure they don't mislead.

Well, you know in cryptography, so you're more aware than I am about the importance of these nuances (and how misinformation has been used in the past to lower cryptographic defenses at the implementor's level)

2. Re:

Are there any targets Rust actually supports that have non-constant-time bit shift operations?

I don't want the constant-timeness of something be guaranteed by happenstance. The compiler and the language don't guarantee it, so it's currently a sort of implicit guarantee? But not even that... it's more happenstance than anything else. That's why I still hesitate.

Maybe the lint should just say all the caveats and not recommend any particular operation 100%

3. Let's assume that shifting is constant time for this question.

The original example worked because it was replacing a division by 4 with a shift by 2. But:

* This would've been done by the compiler anyway (div by 2^n replaced by a shift is one of the most basic optimizations).
* How would one go about it if the division was by another number? The third reason I'm iffy about that example is that it only makes divisions by 2^n constant-time, and not all of them. What if the lint picks up a division by 29? Or some other weird prime? You know what I mean?

Given those two things, I wonder how would a more general example look like. One for divisions other than by 2, 4, 8, etc.

Or maybe one does not divide by numbers other than 2^n in current cryptography, and I shouldn't worry about it here.

[tarcieri]

How can one defend against compiler optimizations messing up the constant-time characteristics of the algorithm? I'm hesitant to leave recommendations in the lint that could lead to someone implementing something that should be constant-time, but gets wrecked by the compiler. Whatever recommendations it makes, I want to make sure they don't mislead.

Short of implementing the algorithm in architecture-specific assembly you can't, or rather mechanisms for ensuring such optimizations won't be applied are nascent.

However, the use of division is highly problematic. This lint was motivated by a real-world example, KyberSlash, where a similar vulnerability was seen across multiple implementations in many languages impacted by many compiler versions (including the @RustCrypto ml-kem crate, where we have the integer_division_remainder_used lint now enabled).

The specific mitigation to use is going to be highly algorithm-dependent (the Barrett reduction example comes from Kyberslash FWIW), so perhaps the clippy lint itself shouldn't be prescriptive about the solution, but rather focus on the problem. Seems you were on the same page with that here:

Maybe the lint should just say all the caveats and not recommend any particular operation 100%

As for this:

Given those two things, I wonder how would a more general example look like. One for divisions other than by 2, 4, 8, etc.

Or maybe one does not divide by numbers other than 2^n in current cryptography, and I shouldn't worry about it here.

Actually cryptography deals quite a bit with dividing by things which aren't powers of two: many asymmetric cryptographic algorithms are built around modular arithmetic where the modulus is something like a very large prime number.

The general strategy is to replace the division by algorithms that can be expressed in terms of additions/subtractions, multiplications, and bitwise operations, without data-dependent branches, pointer calculations, or table lookups. A Barrett reduction is one example. Several others are covered in that HAC chapter I linked earlier.

Another general approach is to turn division into multiplication using e.g. a(n X)GCD algorithm and/or modular inversions.

[felix91gr]

Actually cryptography deals quite a bit with dividing by things which aren't powers of two: many asymmetric cryptographic algorithms are built around modular arithmetic where the modulus is something like a very large prime number.

Nice! I thought so. Since I've been out of the loop in state-of-the-art of asymmetric cryptography for a good while, I had to ask. Modular arithmetic is so cool, man.

---

Okay, I think I'm content with how this has ended up.

@tarcieri I've pushed my latest changes. Can I bother you one more time, and ask you to proof-read the new documentation?

[felix91gr]

@Alexendoo lemme know if I should change anything

[Alexendoo]

I don't think we should be offering advice on how to implement constant time algorithms, pointing out that it needs to be replaced with one is enough for the purpose of this lint IMO

Mentioning KyberSlash as a specific example is nice though, I'd say drop the Final Notes section and move that into the Why restrict this? paragraph