Use constant-time comparison to avoid key timing attacks
anonimal opened this issue · 1 comments
By submitting this issue, I confirm the following:
- I have read and understood the developer guide in kovri-docs.
- I have checked that the issue I am reporting can be replicated or that the feature I am suggesting is not present.
- I have checked opened or recently closed pull requests for existing solutions/implementations to my issue/suggestion.
I should've opened this sooner but the fact remains the same: there are handful of areas which should use constant-time comparison in order to prevent key timing attacks (don't use std::memcmp). One of them being Tag which houses various sensitive crypto structures in AES/HMAC and identity. Constant-time was resolved in #153 for ed25519 but needs to be applied globally.
A quick check also shows that TunnelEndpoint::HandleDecryptedTunnelDataMsg() could be potentially vulnerable from a remote standpoint, as could I2NP's HandleBuildRequestRecords(). The list goes on, but basically any std::memcmp that is crypto-related and/or private-key related (which is 75% of all std::memcmp instances).
I intend to use CryptoPP::VerifyBufsEqual() which is designed to solve this issue. This function won't work out of the box with Tag's < comparator operator though, so that particular solution is TBD.
2018-06-29 00:04:03 @anonimal I was waiting for #909 to get merged before fixing #895 because nacl has a constant-time comparator.
2018-06-29 00:05:23 @anonimal but iirc it's only "null or not null" and doesn't return values like memcmp, which would be needed for < or >
s/not null/-1/ and we're limited by crypto++'s naclite to only 16 and 32 byte sized buffers.
I had messaged @noloader a few weeks ago asking for the possibility of a memcmp-like constant-time equivalent but have heard no response.
If I don't hear back soon, I'll move on the VerifyBufsEqual() solution and add a TODO to implement the rest.