Make RSA decryption API safe to use with PKCS#1 v1.5 padding

[tomato42]

fixes #13421

The code is functional (behaves the same as the implementation in NSS and tlslite-ng).

We need to decide if we want to cache the d_hash value or not.

Checklist

• [x] documentation is added or updated
• [x] tests are added or updated
  • the test vectors are verified in tlslite-ng
• [ ] decide if the the hash of the private exponent should live in the RSA structure or not
• [x] make sure we still handle the mismatch between buffer size and key size the same way

[tomato42]

@beldmit thanks, updated

[beldmit]

Build failures seems relevant

[tomato42]

@beldmit updated

[beldmit]

@tomato42 build failures still persist :(

[tomato42]

@beldmit @t8m PR updated

[t8m]

This looks good to me now, however given this is an API break we should discuss it within OTC at least.

[tomato42]

This looks good to me now, however given this is an API break we should discuss it within OTC at least.

I don't think it is, PKCS#1 v1.5 decryption never guaranteed that an invalid ciphertext wouldn't decrypt to some message

[t8m]

Sure, but at least for some known broken messages the failure was returned, now it will not be. I personally think this is a very beneficial change and should be accepted in 3.0 however it needs to be at least discussed within OTC to see whether there is no major objection to it.

[tomato42]

thanks, but I'm insisting on the "no API break" as I'd like to see it in 1.1.1 too

and there are still ciphertexts that will return errors on decryption: basically for any key a ciphertext starting with 0xff will cause a failure, so it's not like no ciphertext of size matching the bit size of the key will return errors

[kaduk]

thanks, but I'm insisting on the "no API break" as I'd like to see it in 1.1.1 too

and there are still ciphertexts that will return errors on decryption: basically for any key a ciphertext starting with 0xff will cause a failure, so it's not like no ciphertext of size matching the bit size of the key will return errors

The aspect that I would like the OTC (which does not include me) to discuss is that with this change it is trivial for an attacker to generate a ciphertext that will expose "nonsense" payload to the application. Yes, a properly written application should be dealing with that anyway, but without this change the application had a rough filter (on return value) that would catch many cases. I did not attempt to ascertain whether it would also be trivial for an attacker to do so in the absence of this change, which may well be the key technical question.

[tomato42]

thanks, but I'm insisting on the "no API break" as I'd like to see it in 1.1.1 too and there are still ciphertexts that will return errors on decryption: basically for any key a ciphertext starting with 0xff will cause a failure, so it's not like no ciphertext of size matching the bit size of the key will return errors

The aspect that I would like the OTC (which does not include me) to discuss is that with this change it is trivial for an attacker to generate a ciphertext that will expose "nonsense" payload to the application. Yes, a properly written application should be dealing with that anyway, but without this change the application had a rough filter (on return value) that would catch many cases. I did not attempt to ascertain whether it would also be trivial for an attacker to do so in the absence of this change, which may well be the key technical question.

that is trivial for the attacker to do; the attacker just needs to take the public key and use it to encrypt exactly the payload it wants the application to process (and access to the public key is a prerequisite for Bleichenbacher attack anyway)

so, no, this doesn't make it easier for the attacker to expose application to nonsense payloads. To be more precise, neither does RSA-OAEP; with access to public key the attacker can make application process any payload of their choice.

[beldmit]

Ping @openssl/otc to take a look at this issue

[tomato42]

Ping @openssl/otc

[tomato42]

@t8m can you rise it for discussion on an OTC meeting?

[openssl-machine]

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

[t-j-h]

Current OTC vote on placing this as post-3.0 is in progress and not yet complete.

[openssl-machine]

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

[t8m]

The vote was closed:

topic: Set PR 13817 milestone to Post 3.0 Proposed by Tim Hudson Public: yes opened: 2021-04-20 closed: 2021-05-21 accepted: yes (for: 2, against: 0, abstained: 7, not voted: 2)

[t8m]

Summary for the OTC decision

This PR changes the RSA_private_decrypt() API in a way that makes it no longer possible to mount a Bleichenbacher attack on the private key based on the errors reported by the API call.

The PR causes for invalid ciphertext input to always return a plaintext that is pseudorandomly generated from the ciphertext. As the callers always have to cope with arbitrary plaintexts (given the attacker has an access to public key he can create valid ciphertext from an arbitrary plaintext), this does not give an attacker any new attack vector against the caller.

The question for OTC is:

1. Is the inherent vulnerability of the RSA_private_decrypt() API a bug or something we do not consider a bug?
2. Which releases would this change be acceptable for, if any?

[mattcaswell]

OTC: Not a bug. This is ok for master subject to normal review process.

[t-j-h]

I think this should have an option to be able to disable it - and accessible at the EVP_PKEY level so if this causes an issue in a particular context it can be turned off.

[tomato42]

@t-j-h I think this should have an option to be able to disable it - and accessible at the EVP_PKEY level so if this causes an issue in a particular context it can be turned off.

Can you provide an example of such a context?

While I agree that it would be nice to have this behaviour configurable, this is security critical code, so the fewer places to make it go wrong the better. People that really need to inspect the raw values from the RSA decryption can always use the API to ask for no padding decryption.

[beldmit]

Can you provide an example of such a context?

Smth like CMS decryption when we may want to get an exact error using -debug_decrypt command line option

[tomato42]

Smth like CMS decryption when we may want to get an exact error using -debug_decrypt command line option

That option already is documented as vulnerable to Bleichenbacher, so if it really is needed, it can be modified to do NO_PADDING decryption.

[openssl-machine]

This PR is in a state where it requires action by @openssl/otc but the last update was 54 days ago

[openssl-machine]

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

[openssl-machine]

This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago

[beldmit]

@t-j-h are you satisfied with https://github.com/openssl/openssl/pull/13817#issuecomment-1016609594 ?

[openssl-machine]

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

[bernd-edlinger]

Could it be that this will code will break with this PR? https://github.com/openssl/openssl/blob/4f4942a133bd57c4940fb1bc6ed7c8b67da4d8f0/crypto/cms/cms_smime.c#L718-L764

I mean in the case cms_pkey_ri_type == CMS_RECIPINFO_TRANS this tries to decode with all keys, and uses the first which succeeds, not in constant time, but this is just not an issue for smime.