search

tel / saltine

Cryptography that's easy to digest (NaCl/libsodium bindings)
https://github.com/tel/saltine
MIT License
61 stars 29 forks source link

PublicKey comparison is insecure #28

Closed iphydf closed 3 years ago

iphydf commented 7 years ago

https://github.com/tel/saltine/blob/master/src/Crypto/Saltine/Core/Box.hs#L88

This comparison uses normal ByteString equality, which has a few shortcuts and finally resorts to memcmp from libc. This function is insecure for comparison of secret keys. This allows for easy timing attacks. See https://nacl.cr.yp.to/features.html and search for "timing attacks".

olorin commented 7 years ago

I agree that the SecretKey type uses normal ByteString equality, but where are sensitive comparisons performed using that type? I had assumed that any sensitive comparison operations would be performed on data derived from the secret key, but not on the secret key itself.

iphydf commented 7 years ago

Doesn't matter - why is there an Eq instance if it can't ever be used?

olorin commented 7 years ago

Doesn't matter - why is there an Eq instance if it can't ever be used?

Testing code is one example (e.g., I stored a secret key in the database and want to make sure I get the same secret key back out). There shouldn't be any instance in which an operation is dependent on comparing an attacker-controlled secret key to a known-valid secret key, that's why they're secret keys (as opposed to passwords). I don't see any issue with the instance as it stands.

OpenPGP: 6FB7 ED25 BFCF 3E22 72AE 6E8C 47D4 CE7F 6B9F DF57

iphydf commented 4 years ago

This is also relevant for public keys, because you'd want to avoid people figuring out what public keys a client/server/node/application knows about.

iphydf commented 4 years ago

Could bind https://github.com/jedisct1/libsodium/blob/master/src/libsodium/crypto_verify/sodium/verify.c#L90 and use that?

linearray commented 3 years ago

This should be fixed in master.