SRP: Use constant time comparisons of secrets

[jpgoldberg]

In srp/src/server.rs for example, we see

if user_proof == d.result().as_slice() {

where the types are byte slices, &[u8]. I suspect that the same kind of thing appears throughout the code (although I haven't checked).

That will result in a non-constant time comparison, and expose this to timing attacks.

I am new to Rust, so take my suggestion with a large grain of salt. It seems that if we create a trait for secrets and then implement comparison tests for that trait with constant time checks, we could use Rust's type system to enforce that we always have constant time comparisons.

[jpgoldberg]

Ah. I didn't see that the readme explicitly says this doesn't do constant time comparisons.

[jpgoldberg]

And so closing.

[newpavlov]

I will reopen this issue, as it's quite easy to fix by using subtle as we do in other crates.

[warner]

There's no strong reason to not use a constant-time comparison, but:

• the real threat is the time spent in the powm(b) function, where you're taking the secret server b value and taking a variable amount of time to convert it to the public b_pub value. Making that modular-exponentation take constant time is pretty difficult (the mitigation people usually try is to blind it with some random value, do the modexp, then remove the blinding)
  • but the higher-level protocol should never call SrpServer::new() more than once with a given secret b, so the attacker should never get more than one timing sample for a single value. The API might be safer to generate b internally rather than passing it in, to remove the opportunity for mistakes.
• there's no reason to allow more than one verify() call per SrpServer instance, so again the attacker should only get one timing sample
• the hash operation includes the secret key, so to do this right, you need a constant-time hash function, which is also non-trivial

I think PAKEs in general are safer against timing attacks because all the secrets tend to be single-use.

[jbis9051]

#79 adds timing safe comparisons for client and server proof verification. It does not fix the other timing safe issues described by @warner due to the complexity. Although I agree, I don't think a timing attack is actually feasible for the reasons @warner mentioned.