Possible timing attack vulnerability

[Philipp91]

Just a quick heads-up from a passer-by: https://github.com/wisespace-io/yubico-rs/blob/master/src/lib.rs#L198 This comparison might be vulnerable to timing attacks because it's not constant-time. Not sure if that's relevant here at all. I'm reporting such issues to a couple of repositories, please just ignore/close if it isn't applicable here.

[wisespace-io]

Not sure if I understood, could you explain it? Or even better, you can raise a pull request if you already know how to fix it.

[Philipp91]

So the problem is that the comparison aborts early if the signatures don't match. The shorter the matching prefix, the earlier it aborts. If an attacker can run arbitrarily many of these signature checks (and I have no idea if that's the case in your scenario, anyway), the attacker can brute force the signature byte-by-byte, instead of all at once. There are only 256 possible values for each bytes and for the correct one, the comparison takes a tiny bit longer, which can be measurable or not.

Whether this applies to your concrete scenario or not, because these timing attacks exist, it is best practice to use a constant-time comparison function for all digest/signature comparisons. The one that comes with rust-crypto is here: https://github.com/DaGenix/rust-crypto/blob/master/src/util.rs#L43

I guess you could use it on your two &str directly by calling .as_bytes() on them.

A more clean design would compare the signatures before base64 encoding. So your build_signature would return a Vec<u8>, directly from .code(). The upper call site would do the encode() to get the String and the lower one would parse the base64 signature_response and then use crypto::util::fixed_time_eq() instead of ==.

Alternatively, you could return the MacResult from build_signature, construct one from the parsed signature_response with MacResult::new() and use == on the MacResults, which internally uses fixed_time_eq().

Again, I can't tell if there is a vulnerability in your scenario, but it's often much easier to just implement that best practice than to spot all exploit possibilities.

[wisespace-io]

@Philipp91 I implemented your suggestion, I return a MacResult and I use fixed_time_eq()

See https://github.com/wisespace-io/yubico-rs/commit/3b478702070f1a3d69357b80a60830c5128e2ec4

[Philipp91]

looks fine to me

[wisespace-io]

I will close the issue and prepare a new release. Thank you for pointing out this issue.