Add crypto:hash_equals/2

[gearnode]

I propose to add secure_compare/2 function on the crypto module.

Constant time memory comparison is a common mechanism for
applications/libraries dealing with crypto logic. It is easy to code
this function in Erlang[1], but the OpenSSL one is safe and fast.

It's why I suggest enriching the crypto module with this function.

I tried as much as possible to follow the contribution guide. Don't
hesitate to tell me if I made a mistake somewhere.

[1] https://github.com/exograd/erl-pkcs5/blob/master/src/pkcs5.erl#L34

[jhogberg]

but the OpenSSL one is safe and fast.

... but undocumented. :-(

We've discussed this previously in #2749, #2778, #4441, and a technical board meeting. We're open to adding this, and decided that:

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.

[gearnode]

The OpenSSL `CRYPTO_memcmp` is documented[1] and is part of their public API. The naming `hash_equals` makes no sense to me, as this function can be used to compare any binary not only `HMAC` result. And this name does not hint about the main characteristic of this function (comparing binary in a constant time). [1] https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_memcmp.html

[jhogberg]

The OpenSSL CRYPTO_memcmp is documented[1] and is part of their public API.

To quote #2749 (comment):

However, the CRYPTO_memcmp is not documented in other OpenSSL versions than 1.1.1, not even the 3.0 that is soon in beta state. LibreSSL documents it. The function is available in all versions we have to support, but relying on a not documented function is not good.

Emphasis mine. They may have added it since that was posted however.

The naming hash_equals makes no sense to me, as this function can be
used to compare any binary not only HMAC result. And this name does
not hint about the main characteristic of this function (comparing
binary in a constant time).

We want to discourage using it to compare plaintext passwords and the like, and figured hash_equals/2 was a decent enough name for that. We're open to suggestions.

[gearnode]

> The OpenSSL `CRYPTO_memcmp` is documented[1] and is part of their public API. To quote #2749 (comment): > However, the CRYPTO_memcmp is not documented in other OpenSSL versions > than 1.1.1, **not even the 3.0 that is soon in beta state.** LibreSSL > documents it. The function is available in all versions we have to > support, but relying on a not documented function is not good. (Emphasis mine)

The function is documented in the OpenSSL 3.0[1].

> The naming `hash_equals` makes no sense to me, as this function can be used to compare any binary not only `HMAC` result. And this name does not hint about the main characteristic of this function (comparing binary in a constant time). We want to discourage using it to compare plaintext passwords and the like, and figured `hash_equals/2` was a decent enough name for that. We're open to suggestions.

Can you explain why you would discourage the usage of plain text? [1] https://github.com/openssl/openssl/blob/openssl-3.0.0-alpha14/doc/man3/CRYPTO_memcmp.pod

[gearnode]

I have updated the function name.

I propose to replace the call to the equal_const_time with the new
function. And mark the equal_const_time as deprecated. What do you
think?

[jhogberg]

The function is documented in the OpenSSL 3.0[1].

Sweet, then it should no longer be an issue. :-)

Can you explain why you would discourage the usage of plain text?

To put it differently, when would you want this for something that is not a hash?

We haven't found a good answer to that question, the only cases we could think of involved known-bad practices. Plaintext password comparison is bad for several reasons but to keep it brief:

1. The timing-safety of the comparison function is the only line of defense.
2. It implies having stored (or made it easy to retrieve) plain passwords, which is very bad when the database leaks.

I propose to replace the call to the equal_const_time with the new
function. And mark the equal_const_time as deprecated. What do you
think?

equal_const_time/2 is undocumented so we could yank it right away, but I'll defer to @HansN. I don't maintain this part of OTP, I've just taken part in past discussions on this subject.

[josevalim]

We haven't found a good answer to that question, the only cases we could think of involved known-bad practices.

Personal access tokens should still be secure compared to avoid timing attacks and they don't necessarily need to be hashed. Hashing would be better - sure - but not as bad as keeping a password plaintext. I would also use secure compare when comparing short-lived one-time passwords from time-based or SMS auth.

Sometimes you also want to use "secure_compare" to avoid leaking information. For example, imagine you are creating an email provider. As part of signup, you need to list an "email recovery address" (or a phone number) in case you forget your credentials. Now, if you forget your credentials, you need to two inputs to trigger recovery: your current e-mail address and the recovery phone number / e-mail address. You want to use secure_compare when verifying the latter, otherwise someone can retrieve that information out of the DB.

[HansN]

The purpose of the crypto app in OTP has so far been to provide functions common for the needs of the OTP apps SSL, SSH and public_key. There are however some old exceptions to this present in crypto.

If we aim for a more general cryptographic app, we need to at least investigate the future maintenance workload and needed competence, and also decide for some more general design.

To add one simple function like the subject for this PR must also take care of all supported cryptolib versions. OTP is required to support from OpenSSL 0.9.8c and later. The function called by this PR is documented for 1.1.1 and also in 3.0 as the PR author kindly remarked. It is also present in earlier versions, but not documented. A PR must therefor handle the case that the function is absent in the linked library. Note that if cryptographic code is provided in for example crypto, the juridical aspects might change which in some countries could affect companies and persons delivering Erlang/OTP or clones of it.

[jhogberg]

Personal access tokens should still be secure compared to avoid timing attacks and they don't necessarily need to be hashed. Hashing would be better - sure - but not as bad as keeping a password plaintext. I would also use secure compare when comparing short-lived one-time passwords from time-based or SMS auth.

Good point!

Now, if you forget your credentials, you need to two inputs to trigger recovery: your current e-mail address and the recovery phone number / e-mail address. You want to use secure_compare when verifying the latter, otherwise someone can retrieve that information out of the DB.

This strikes me as very similar to logging in. We already have best practices for that, so why build something ad-hoc for account recovery when it's nearly the same thing?

In one case you want to keep a password secret, in the other a phone number, and I don't think we should be less careful with the latter. It's very sensitive information for many people.

[josevalim]

This strikes me as very similar to logging in. We already have best practices for that, so why build something ad-hoc for account recovery when it's nearly the same thing?

The difference is that you want to hash a password but you encrypt PII. So in those cases you often have the data encrypted at rest (either directly in the database, with pgcrypto or similar, or as an abstraction in your "ORM"). This gives you the safety but still allows your application to see the stored e-mail/phone number (after all, it has to). So you have to be careful inside your application to not leak the information by using secure compare.

[jhogberg]

The difference is that you want to hash a password but you encrypt PII. So in those cases you often have the data encrypted at rest (either directly in the database, with pgcrypto or similar, or as an abstraction in your "ORM"). This gives you the safety but still allows your application to see the stored e-mail/phone number (after all, it has to). So you have to be careful inside your application to not leak the information by using secure compare.

There's nothing preventing us from doing both. Safety at rest is only one reason why passwords need to be hashed, another reason is that comparing raw strings leaks their lengths and puts us entirely at the mercy of the comparison function. We want to avoid that in this case too, and there's nothing saying that we can't store a hash next to the recovery information (both encrypted at rest).

Either way I'm bowing out, this is a fun tangent but I don't want to derail the thread further. We can move it to the EEF slack instead. :)

[gearnode]

The purpose of the crypto app in OTP has so far been to provide functions common for the needs of the OTP apps SSL, SSH and public_key. There are however some old exceptions to this present in crypto.

The proposed change does not change the primary role of the crypto module, as the ssh module uses the function.

If we aim for a more general cryptographic app, we need to at least investigate the future maintenance workload and needed competence, and also decide for some more general design.

I agree. And the function is already exported by the crypto module with this comment: "Candidate for a NIF".

To add one simple function like the subject for this PR must also take care of all supported cryptolib versions. OTP is required to support from OpenSSL 0.9.8c and later. The function called by this PR is documented for 1.1.1 and also in 3.0 as the PR author kindly remarked.

The OpenSSL team itself does not support OpenSSL 0.9.8 since the January 1st, 2016[1].

It is also present in earlier versions, but not documented. A PR must therefor handle the case that the function is absent in the linked library.

I can update the patch to fallback to an erlang implementation when the OpenSSL function is not provided.

Note that if cryptographic code is provided in for example crypto, the juridical aspects might change which in some countries could affect companies and persons delivering Erlang/OTP or clones of it.

The proposed patch does not add anything new. The compare algorithm is already shipped by the OTP since January 2018. And as far I know, it's a compare algorithm, not a cryptographic one. Can this patch be merged if I add an Erlang fallback? [1] https://www.openssl.org/news/vulnerabilities-0.9.8.html

[HansN]

Can this patch be merged if I add an Erlang fallback?

It depends on if it fulfills the comments in the previous attempts ( #2749, #2778 ), the OTP Technical Board (OTB) decision referenced above ( #4441 ) and a decision in a new OTB meeting.

[galdor]

I'm not sure having two implementations, Erlang and C, is a good idea for a function which is quite important in many crypto-related applications. CRYPTO_memcmp is as far as I know available in all supported OpenSSL versions; is there a precise combination of platform and supported Erlang version which does not have this version ?

@HansN #2749 is closed in favor of #2778 which is itself closed in favor of #4441, which does not really help here: it does not seem to be aware of CRYPTO_memcmp and focuses on how this kind of function would be written from scratch.

[HansN]

@galdor It's true that CRYPTO_memcmp is supported in all OpenSSL supported versions of cryptolib. We were well aware of that long before this question arose. The problem is that OTP has to support older versions - now unsupported by OpenSSL - in which this function exists, but is not documented.

That's the reason why a replacement was discussed.

[galdor]

This is exactly what I'm asking about :) I cannot find any precise information about which OTP versions are officially considered as "supported". I may be missing something, but I'm not even sure how it relates to the situation: I imagine several old OTP versions are supported for security fixes, but they would not be affected since a new function would obviously not be backported in an old stable branch.

[lhoguin]

Use autotools to only provide this function if CRYPTO_memcmp is detected maybe? That way even if users don't have it in their OpenSSL (because they removed it, it happens), then you can still build like before at the very least.

[HansN]

@galdor Supported OTP versions are described here

This is not to be confused with the versions of OpenSSL libcrypto that OpenSSL supports. They support now only 1.1.1 and later also the yet unreleased 3.0 afaik.

In every supported OTP version we support OpenSSL 0.9.8c and later. That has nothing to do with which OTP versions we support, nor with which OpenSSL versions they support.

Hope this clarifies.

[galdor]

The page you linked indicates that "bugs are only fixed on the latest release", but immediately after adds that "we, due to internal reasons, fix bugs on older releases". So there is no such thing as an officially supported version beyond the latest one (not that I'm complaining, it is just an observation).

That being said, if every supported OTP version (which therefore means pretty much any version) supports 0.9.8.c, so be it, but I cannot find any public information about that. And his is very surprising given how insecure this is, and this might be a good time to bump the minimal supported version.

Unless of course Ericsson has contracts enforcing this requirement, which I would understand of course. But it would be nice to clearly document this kind of thing so that in the future, potential contributors do not waste their time.

[HansN]

@gearnode, @lhoguin, @jhogberg What about modifying/extending this PR by :

1. have a configure check for CRYPTO_memcmp as @lhoguin proposed
2. if the check indicates that CRYPTO_memcmp is not available, raise an exception or return an error
3. find a good name (the hardest part)

The configure is being re-written now for 24.0, so we have to wait till after the release with changes

[gearnode]

have a configure check for CRYPTO_memcmp as @lhoguin proposed

Yes, I can try to do this.

if the check indicates that CRYPTO_memcmp is not available, raise an exception or return an error

Would you say a compile error or runtime error?

find a good name (the hardest part)

For sure!

[HansN]

@gearnode Great!

if the check indicates that CRYPTO_memcmp is not available, raise an exception or return an error

Would you say a compile error or runtime error?

Well, most tests in crypto are reported as exceptions in runtime (since code on top of crypto must be able to work even if certain functions are not available), like this:

#include "common.h"
...
#ifdef SOME_FLAG
   ....
   return result;
#else
    return EXCP_NOTSUP(env, "Unsupported XXX");
#endif

The test for CRYPTO_memcmp should be done in the lib/crypto/configure script.

Or is there a better solution?

[gearnode]

I'm far from being an autoconf expert, but I have to do something that works.

@HansN If you don't have any other feedback, I can squash my commits :)

[gearnode]

Another point, should I replace the compare function in the SSH module with the new one?

[HansN]

Please go ahead and squash the commits. Leave ssh outside of this, that's a separate task I think.

[gearnode]

@HansN Done! I can open a dedicated pull request for the SSH module if you want :)

[HansN]

Thanks for the squash, but I had expected a commit message more like "Add crypto:hash_equals/2" :)

[gearnode]

@HansN fixed!

[gearnode]

Thanks for the patch @rickard-green :)

@HansN I've re-squash

[rickard-green]

I've pushed another adjustment of the configure test that was needed

[HansN]

To avoid a failed test in case of missing CRYPTO_memcmp one can change the test into something like:

try
       true = crypto:hash_equals(<<>>, <<>>),
       true = crypto:hash_equals(<<"abc">>, <<"abc">>),
       false = crypto:hash_equals(<<"abc">>, <<"abe">>)
catch
       error: {notsup,{"hash_equals.c",_Line},"Unsupported CRYPTO_memcmp"} -> {skip, "No CRYPTO_memcmp"}
end

[gearnode]

Fixed and I've squashed all the commits.

[HansN]

Thanks for the PR!