This issue was the simplest to fix in the RavenDB security report finding. Here is what the code looks like:
And once it was pointed out, I was cringing inside because it was both obvious what was wrong and I knew it was wrong, and we still had this in the code (which is why audits are great).
What is the problem here? Conceptually, this is what this method does:
This works perfectly. Except that it leaks information to a potential attack. In particular, it leaks timing data. You might expect that the time spent comparing a couple of 32 bytes values wouldn’t matter, given that an attack is probably over a network connection, but it does. There is a great paper that shows how you can use such timing attacks to get private keys information.
In short, anything that uses security needs to use constant time comparisons. Again, conceptually, this means changing the code to be:
So regardless of the result, we’ll always do the same amount of work and won’t expose details through different processing times. In general, by the way, algorithms and execution of code in cryptography attempt to avoid anything that branches on the secret information, because that leaks details to attackers. This is also why AES cannot be securely implemented in software, you always leak in that mode.
The fix, by the way, was to use the sodium_memcmp, which ensures constant time operation.