erlang / otp

Erlang/OTP
http://erlang.org
Apache License 2.0
11.38k stars 2.95k forks source link

ERL-1385: Add a hash comparision function to OTP? #4441

Closed OTP-Maintainer closed 3 years ago

OTP-Maintainer commented 4 years ago

Original reporter: hans Affected version: OTP-24.0 Component: crypto Migrated from: https://bugs.erlang.org/browse/ERL-1385


Crypto has since a time had an undocumented function crypto:equal_const_time/2 with the purpose of comparing two binaries or two strings without revealing at which position  a possible miss-match is.

The Pull Request [PR-2749|https://github.com/erlang/otp/pull/2749] suggested a NIF implementation, and later the [PR-2778|https://github.com/erlang/otp/pull/2778] suggested the same as a BIF since crypto is not available if not a cryptolib is available.

At the OTP Technical Board 2020-10-15 both PRs have been rejected, and the following were noted for a future possible implementation of such a function as public:
{quote}If we add a function like this it should belong to {{crypto}} and should be named {{hash_equals/2}}.

The type spec should be {{hash_equals(Arg1::binary(), Arg2::binary() ) -> true | false}}.

If the size of Arg1 and Arg2 differs it should crash to make it obvious for the user that the function is intended to be used for comparing hash values where the length is known. The use for comparing for example plain text passwords of different lengths is not a supported use case and with a crash for unequal lengths we make that obvious. We also think the function should be implemented in Erlang (we already have such an implementation) as long as there is no public function in OpenSSLs crypto lib that can be used. We might change the implementation if such a function is made public in OpenSSL.

Before taking the final decision we want to know more about the use cases outside SSL and SSH where a function like this should be used. This in order to judge if the function and placement in crypto is the most useful one. The rationale to put the function in crypto is that if a user is dealing with something that is supposed to be safe then crypto is also needed. And if some other crypto solution is used then a "secure" compare should be available in that solution/library as well.
{quote}
Answers? Questions? Comments? Login to make a Comment.
OTP-Maintainer commented 4 years ago

starbelly said:

 Firstly let me state that I appreciate the time put into all this. Despite the PRs being rejected I learned a lot, in particularly around the ERTS, and that in and of itself made the attempt worth it. The commentary by members of the OTP team on the PRs were also very educational.
 - I did offer to do an erlang implementation as an alternative to the C implementation in the PRs FWIW. In testing I
 found more variance in the erlang implementation vs in C. Though, I think that should be checked by another party, as perhaps my tests were flawed or perhaps the code could be re-written to eliminate said variance. It also may be the
 variance is acceptable. After all, a little variance is better than returning early on the first difference between two binaries or lists.

 
 - One of the possible problems with having to go through a NIF implementation for such a function is a realloc that can
 occur in the face of unaligned sub binaries, AFAIK. Is there a way to avoid this currently in the context of a NIF? Could a way be made available?

 
 - An alternative to such a function existing in either crypto or binary might be a new module : crypto_lib. Functions written in erlang (possibly other langs) could live here and be made available even if you compile without openssl (or a compat lib).

 
 - Cases that live outside the context of ssl and ssh is any scenario where authentication must be performed using tokens. Said tokens might not be hashes (thus the name hash_equals I do not believe makes sense). Web applications are one such area where tokens must be verified securely as possible on each request.
OTP-Maintainer commented 4 years ago

john said:

{quote}I did offer to do an erlang implementation as an alternative to the C implementation in the PRs FWIW. In testing I
 found more variance in the erlang implementation vs in C. Though, I think that should be checked by another party, as perhaps my tests were flawed or perhaps the code could be re-written to eliminate said variance. It also may be the
 variance is acceptable. After all, a little variance is better than returning early on the first difference between two binaries or lists.
{quote}
Variance is not a problem per se, the important thing is that it never varies depending on how the terms compare.
{quote}One of the possible problems with having to go through a NIF implementation for such a function is a realloc that can occur in the face of unaligned sub binaries, AFAIK. Is there a way to avoid this currently in the context of a NIF? Could a way be made available?
{quote}
Even if you avoid reallocation, unaligned accesses are more expensive than aligned ones on all but the most exotic hardware, so you'll leak that either way.

Knowing whether it's byte-aligned in memory doesn't say anything about the secret itself though, so I think we can leave that alone.
{quote}An alternative to such a function existing in either crypto or binary might be a new module : crypto_lib. Functions written in erlang (possibly other langs) could live here and be made available even if you compile without openssl (or a compat lib).
{quote}
I don't think this is useful without {{crypto}}. You need encryption to begin with if you care about keeping things secret, and if you're using a different library for that then said library can expose a similar function.
{quote}Cases that live outside the context of ssl and ssh is any scenario where authentication must be performed using tokens. Said tokens might not be hashes (thus the name hash_equals I do not believe makes sense). Web applications are one such area where tokens must be verified securely as possible on each request.
{quote}
We're open to suggestions, but we'd like to avoid anything that implies that it's suitable for iffy uses like comparing passwords. 
OTP-Maintainer commented 4 years ago

starbelly said:

I think those are all fair points (y)