Comparing signatures with `==` is bad security practice due to byte-wise short-circuit being a potential source of subtle timing attack.
sporkmonger opened this issue · 4 comments
If you compare user supplied signature to calculated signature using ==
, Ruby will helpfully abort the comparison early if you have a mismatch between the signatures. There's at least a few places in Signet that do this, with the OAuth 1 server code that @mechazoidal contributed being the most exposed to the issue. In a nutshell, you probably want a non-short-circuited equality check for that.
Realistically, I doubt anyone could actually build an exploit for this in large part due to Ruby's wildly varying runtimes for web requests, but I don't really want to find out the hard way, so this should probably get eliminated as even a possibility, just in case.
In terms of non-short-circuited equality operations you could use, assuming a signature of 'pretendsignature', you can safely abort if the supplied signature is the wrong length (invalid signature, no information revealed), and zip
on the pair of 'pretendsignature'.unpack('C*')
and supplied_signature.unpack('C*')
, doing a bitwise |
reduce
on the output of a ^ b
, and indicating a match if the final output is zero.
The other, possibly easier and more readable option is to do a double HMAC – of both the calculated value and then the attacker supplied value.
If:
HMAC(m, k) == their_signature
Then the following is also true:
HMAC(HMAC(m, k), k) == HMAC(their_signature, k)
But in the second scenario, the attacker no longer has any control whatsoever over the timing of the thing being compared and should be unable to take advantage of the short circuit, while also maintaining readability of the code.
def safe_compare a, b
check = a.bytesize ^ b.bytesize
a.bytes.zip(b.bytes) { |x, y| check |= x ^ y.to_i }
check == 0
end
Yup, that looks right to me. Also more readable than what I was suggesting by virtue of not using the inscrutable unpack
.