Disable PKCS#1 v1.5 padding

[timlegge]

Take a look at the pod info. Happy to revise ...

[ThomasLamprecht]

This breaks padding for RSA signatures, which do not support OAEP padding but only PSS padding for a modern and safe padding mode, excerpt from openssl's RSA signature implementation:

        case RSA_PKCS1_OAEP_PADDING:
            /*
             * OAEP padding is for asymmetric cipher only so is not compatible
             * with signature use.
             */
            err_extra_text = "OAEP padding not allowed for signing / verifying";
            goto bad_pad;

See: https://github.com/openssl/openssl/blob/ab021b624f1d09378bb5115ccfa01517a5ea0bdc/providers/implementations/signature/rsa_sig.c#L1614-L1620

I think also that might be the underlying reason for what resulted in the pre-existing FIXME comment added in PR #53 combined with ignoring result from the calling set_rsa_padding, i.e.:

Crypt-OpenSSL-RSA/RSA.xs

Lines 972 to 973 in dbf24f4

	/* FIXME: Issue setting padding in some cases */
	EVP_PKEY_CTX_set_rsa_padding(ctx, p_rsa->padding);

Crypt-OpenSSL-RSA/RSA.xs

Lines 1029 to 1030 in dbf24f4

	/* FIXME: Issue setting padding in some cases */
	EVP_PKEY_CTX_set_rsa_padding(ctx, p_rsa->padding);

As yes, trying to set padding modes for an operation that just cannot support them will return an error, nothing really unexpected here. Note, that this also means that the legacy PKCS1 padding will actually be used, as that is still the default in openssl, so failing to set it here will keep that default active, which makes this even more confusing for users.

That "not checking the result" pattern on setting the padding–introduced in #53–is also very problematic, especially as openssl has an error stack, so if setting the padding fails the next operation that actually checks the error stack might get confused. Our application does that, and as it's async we got sporadic errors in a completely unrelated part not using any of this module for errors that very much got caused by this module.

Besides, while I can see the good intentions behind this and making default behavior more secure, I find it highly questionable to straight out croak when one tries to use the older mode. What should one do if they need to verify older signatures? Or worse, decrypt data that was encrypted with an old version of this library?
Even policies like FIPS 140-3, which try to provide a strong and thought out protection, normally only disallow the creation of new signature with outdated modes or the like but not the decryption or verification of existing signatures for legacy purpose, so why should a Perl binding be more strict here? To be frank, it might make more sense to vet this for basic error checking of the binding code instead and improve overall security that way, while not artificially limiting users of this module in the name of security.
Disallowing a mode that is still not only available, but actually the default in openssl is really not the job of a binding library module.

Sorry to be a bit grumpy here, but we had some rather hard to trivial debug fallout from both, the new default and the pre-existing missing error checking, and looking at issue #61 it seems that we're not the only ones.

IMO, the steps to improvement might look like:

• allow using PKCS1_PADDING again. Being unable to verify old signatures and those create with current OpenSSL 3.5 defaults is far from ideal. Even creation should be allowed, with docs that it should be avoided if possible for security reason.
• Do not try to set padding modes that cannot work, like PKCS1_OAEP for signatures. Once that is done, re-enable error checking again to avoid keeping errors left on the openssl stack.
• Add support for PKCS1_PSS_PADDING, as that padding mode is modern and can be used for creating and verifying signatures (but not encryption/decryption).

A colleague of mine actually looked into this already, see the three commits on top of:
https://github.com/Blub/Crypt-OpenSSL-RSA/commits/padding-issues/

We will roll out parts of above points for a downstream build to fix using one of our core HTTP API daemons.
We'll also try to get around on opening an actual PR here, but I wanted to get this out so that any other one running into this after seing some weird breakage might save some time of theirs.

[timlegge]

At the time we were unable to find a way to protect against the marvin attack. I am away this week but I would be happy to review and test the marvin attack scripts against any PR you might generate. I will review the branch you mentioned.

I do agree disabling the padding mode completely is sub-optimal.