Replace in-tree MD5 with OpenSSL

[bgermann]

This is a follow-up for #8272 with a different OpenSSL interface used.
The original PR was rejected because it used a deprecated interface.
The performance properties should be the same but I have not measured them.
My main motivation for this is getting rid of the RSA-MD code.

[vtnerd]

There should be calls to free the ctx? Ideally using a std::unique_ptr with custom deleter.

[hyc]

The original problem with #8272 remains - MD5 is deprecated since OpenSSL 3.0 and there are no guarantees that it will be present at any particular site. If you're going to go ahead and remove the embedded MD5 code, you should modify all its callers to use a modern/supported hash algorithm instead.

[bgermann]

I have addressed that concern in my commit message by writing "non-dreprecated EVP facility". In fact, only the md5.h interface is deprecated in OpenSSL 3+. The documentation reads: "All of the functions described on this page are deprecated. Applications should instead use EVP_DigestInit_ex(3), EVP_DigestUpdate(3) and EVP_DigestFinal_ex(3)." And I am using these (actually not the _ex version but I can change this easily).

MD5 being deprecated by RFC 6151 for some applications is a different issue but replacing it with something else is a design decision that was asked for in other issues but not solved for now.

[bgermann]

EVP_MD-MD5 is still in the default OpenSSL algorithm provider.

[iamamyth]

You can allocate all of these digest instances on the stack (they don't require much space), that's what the prior implementation did and that's what the OpenSSL examples do. In theory, it's a good idea to reset the stack-allocated context when you're done with it (via EVP_MD_CTX_reset), but EVP_DigestFinal already does this.

[vtnerd]

The original problem with #8272 remains - MD5 is deprecated since OpenSSL 3.0 and there are no guarantees that it will be present at any particular site. If you're going to go ahead and remove the embedded MD5 code, you should modify all its callers to use a modern/supported hash algorithm instead.

I think the primary blocker is no real support for other hash algorithms. We're probably better off nudging people towards SSL cert checking.

[bgermann]

You cannot allocate the EVP types on the stack since OpenSSL 1.1.

[sorenstoutner]

To expand a bit on what @bgermann said in his initial comment as to the motivation for this change, as described in feather-wallet/feather#218, the code that @bgermann is proposing to replace is licensed under the RSA-MD, which is incompatible with the GPL. Feather Wallet includes other code licensed under the GPL, meaning that currently it is not legal to distribute the feather binary. Switching to a different implementation of this feature based on code that isn't licensed RSA-MD resolves the problem.

[vtnerd]

You cannot allocate the EVP types on the stack since OpenSSL 1.1
.

Still need to free the memory.

[iamamyth]

You cannot allocate the EVP types on the stack since OpenSSL 1.1.

You definitely can, and they work fine, otherwise EVP_MD_CTX_init wouldn't exist. What the docs actually say is that stack allocations might break the code when linking to a new major version of OpenSSL (https://docs.openssl.org/1.0.2/man3/EVP_DigestInit/#notes):

Stack allocation of EVP_MD_CTX structures is common, for example:

EVP_MD_CTX mctx;
EVP_MD_CTX_init(&mctx);

This will cause binary compatibility issues if the size of EVP_MD_CTX structure changes (this will only happen with a major release of OpenSSL). Applications wishing to avoid this should use EVP_MD_CTX_create() instead

I don't have a strong view on cross-major-revision compatibility, though I suppose it doesn't hurt; but then, if you want it, the allocations need to be paired with free.

[bgermann]

It is not helpful to point to a function that was removed in 1.1 to counter a point that was made about 1.1 or later. Yes, I know that OpenSSL supported it in the old days.

I am going to free the mem when I have time for it.

[bgermann]

Freeing the mem with a unique_ptr is done. I think, I have addressed every comment.

[0xFFFC0000]

LGTM. I am usually in support of using well audited third party libraries instead of our own. I was afraid of MD5 deprecation issue in OpenSSL.

[iamamyth]

Agreed that support was removed for stack allocation in OpenSSL 3.0 (https://docs.openssl.org/master/man7/ossl-guide-migration/#upgrading-from-openssl-111), which is the relevant minimum version (not 1.1).

[sorenstoutner]

Are there any other blockers to getting this merged?