+972 52-548-6969
RavenDB Security ReviewNon-Constant Time Secret Comparison
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.
More posts in "RavenDB Security Review" series:
- (27 Mar 2018) Non-Constant Time Secret Comparison
- (26 Mar 2018) Encrypt, don’t obfuscate
- (23 Mar 2018) Encrypting data on disk
- (22 Mar 2018) Nonce reuse
- (20 Mar 2018) Finding and details
Comments
C# version is not safe, because it could be potentially optimized by the compiler, so you have to use a precompiled binary or explicitly tell compiler not to optimize this method (potentially it can figure out that if the value comes false, then we can return).
Valery, That is interesting to me. Can you explain how the C# compiler can optimize this in a non safe way? We are actually using
sodium_memcmphere, so that is not an issue, but this is interesting.Since there are no restrictions, an optimizing compiler (created now, or in the future) might figure out that if we assign 'false' to the match variable, then it will not change and all other iterations have no side effects. But it's just a possibility that someone will implement such optimization in a compiler. Such optimization can potentially improve performance of some programs, but it will break the security. I don't know if any compiler has such optimization, but it seems to be not forbidden, thus possible.
Valery, Interesting point. I already know that crypto people are being very careful about what assembly is being generated, and ARM systems are just declared as "you can't do crypto" there because they don't have constant time multplication https://www.bearssl.org/constanttime.html
And here for more details: https://eprint.iacr.org/2016/1123.pdf
I wonder if the implementation above could also suffer from timing attacks because of branch prediction. If e.g. odd bytes are equal, and even bytes are not, then a simple branch predictor will guess wrong a lot, making the computation slower. If all bytes are either equal or not equal, then it will guess right almost all the time, which will be fast.
I think an attacker could still get some information from that.
Though this assumes that the
ifwill be compiled to a conditional jump and not e.g.cmov.It seems libsodium avoids this issue by using code like:
Svick, Just to point out, the code above is mostly meant as an example so people can reason about it, not as something to actually use.
Comment preview