Open jpgoldberg opened 5 years ago
Ah. I didn't see that the readme explicitly says this doesn't do constant time comparisons.
And so closing.
I will reopen this issue, as it's quite easy to fix by using subtle
as we do in other crates.
There's no strong reason to not use a constant-time comparison, but:
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)
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.verify()
call per SrpServer
instance, so again the attacker should only get one timing sampleI think PAKEs in general are safer against timing attacks because all the secrets tend to be single-use.
https://github.com/RustCrypto/PAKEs/pull/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.
In
srp/src/server.rs
for example, we seewhere 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.