erlang / otp

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

ERL-878: public_key:decrypt_private does not accept RSA padding options #4058

Closed OTP-Maintainer closed 3 years ago

OTP-Maintainer commented 5 years ago

Original reporter: victor.olinasc Affected version: OTP-21.3 Fixed in version: OTP-22.0 Component: public_key Migrated from: https://bugs.erlang.org/browse/ERL-878


`public_key:decrypt_private/3` is documented to accept all `crypto:pk_encrypt_decrypt_opts()` but it only uses the legacy rsa padding options. 

This line: https://github.com/erlang/otp/blob/maint-21/lib/public_key/src/public_key.erl#L409 makes it only accept `rsa_pkcs1_padding` where its delegated function `crypto:private_decrypt` accepts plenty of other options. 

I am not sure if there is a reason for this and so I haven't opened a PR to fix it (which would only remove the aforementioned line). If it must only accept pkcs1_padding (and not oaep_padding) then it should at least change the typespec and add some docs.

Thanks for all your work and I hope this helps.
OTP-Maintainer commented 5 years ago

ingela said:

That line does not do make it accept only that padding, it makes that old padding the default value
if no padding is supplied explicitly. The reason for choosing that default is  that it is used by all TLS versions except the latest, TLS -1.3, and yes it ought to have been changed earlier but standardizations takes time.  That said I do not think we have test vectors at the moment for other paddings in the public_key application.
OTP-Maintainer commented 5 years ago

victor.olinasc said:

Hi Ingela, thanks for looking into it. 

After that line it never passes the options to crypto. So, it only accepts the rsa_compat options. I can't pass rsa_mgf1_md and rsa_oaep_md for example.

A simple example would be:

{code:elixir}
priv_key = # fetch and parse private key
encrypted_bin = # fetch encrypted bin
:public_key.decrypt_private(encrypted_bin, priv_key, [rsa_padding: :rsa_pkcs1_oaep_padding, rsa_oaep_md: :sha256, rsa_mgf1_md: :sha256])
{code}

WHich fails with:

** (ErlangError) Erlang error: :decrypt_failed
    (crypto) crypto.erl:975: :crypto.pkey_crypt(:rsa, <<44, 236, ...>>, [many numbers from my private key], [rsa_padding: :rsa_pkcs1_padding])

The padding it is using is rsa_pkcs1_padding on the error message even though I have explicitily said rsa_padding: rsa_pkcs1_oaep_padding. Also, the other options are ignored. 

Even if I pass {rsa_pad, rsa_pkcs1_oaep_padding} I can't pass the the message digest options.
OTP-Maintainer commented 5 years ago

ingela said:

We willl probably get around to fixing this soonish as we are in the process of implementing TLS-1.3.  If you could generate some test data that we could use to reproduce the problem in the public_key test suite that could speed up the process. 
OTP-Maintainer commented 5 years ago

victor.olinasc said:

I will try to find some time later to post here some test data. Just noticed that `public_key:encrypt_public/3` has the same bug: it does not pass the options to `crypto:public_encrypt/4`.
OTP-Maintainer commented 5 years ago

victor.olinasc said:

Anything generated with:

{noformat}
echo "hello" | openssl pkeyutl -pubin -inkey mykey.pub -encrypt -pkeyopt rsa_padding_mode:oaep -pkeyopt rsa_mgf1_md:sha256 -pkeyopt rsa_oaep_md:sha256
{noformat}

Should be verified with:

{noformat}
public_key:decrypt_private(EncryptedBin, PrivKey, [{rsa_padding, rsa_pkcs1_oaep_padding}, {rsa_oaep_md, sha256}, {rsa_mgf1_md, sha256}])
{noformat}

It won't work. But can be verified with:

{noformat}
# Elixir syntax here... not familiar with the record syntax... sorry about that!
{:RSAPrivateKey, _, n, e, d, p1, p2, e1, e2, c, _} = priv_key
:crypto.private_decrypt(:rsa, encrypted_bin, [e, n, d, p1, p2, e1, e2, c], [rsa_padding: :rsa_pkcs1_oaep_padding, rsa_oaep_md: :sha256, rsa_mgf1_md: :sha256])
{noformat}
OTP-Maintainer commented 5 years ago

victor.olinasc said:

@Ingela Is that sufficient? 

Also, couldn't the fix be so that it just pass options as is to crypto?
OTP-Maintainer commented 5 years ago

ingela said:

I think it is sufficient, we will have a closer look. I think it could be as you say.
OTP-Maintainer commented 4 years ago

JIRAUSER12803 said:

This problem may still present depending on the version of OpenSSL, with the appearance of it still not being accepted if the OpenSSL version is less than 1.1.0.
OTP-Maintainer commented 4 years ago

hans said:

The support is disabled for versions less than 1.1.0 by an #ifdef.  Is it that you see as a problem or something else?

 
OTP-Maintainer commented 4 years ago

JIRAUSER12803 said:

Indeed that is the issue. In looking through the code, ifdefs and openssl implementation I found the feature is available in pre-1.1 openssl, but accept the difficulty in feature matching for the nif. The root problem is using long term support OS and the best solution is to build openssl specifically for erlang, though bundling with a release is a detail. My thanks for all the hard work here.