Deprecate the EVP_PKEY controls for CMS and PKCS#7

Improve the ossl_rsa_check_key() to prevent non-signature
operations with PSS keys.

Do not invoke the EVP_PKEY controls for CMS and PKCS#7 anymore
as they are not needed anymore and deprecate them.

Fixes #14276

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/14760)

11 files changed, 79 insertions, 175 deletions

diff --git a/CHANGES.md b/CHANGES.md
index 54fc6855f0..581fda0c96 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -31,6 +31,15 @@ OpenSSL 3.0
 
    *Shane Lontis*
 
+ * The EVP_PKEY_CTRL_PKCS7_ENCRYPT, EVP_PKEY_CTRL_PKCS7_DECRYPT,
+   EVP_PKEY_CTRL_PKCS7_SIGN, EVP_PKEY_CTRL_CMS_ENCRYPT,
+   EVP_PKEY_CTRL_CMS_DECRYPT, and EVP_PKEY_CTRL_CMS_SIGN control operations
+   are deprecated. They are not invoked by the OpenSSL library anymore and
+   are replaced by direct checks of the key operation against the key type
+   when the operation is initialized.
+
+   *Tomáš Mráz*
+
  * The EVP_PKEY_public_check() and EVP_PKEY_param_check() functions now work for
    more key types including RSA, DSA, ED25519, X25519, ED448 and X448.
    Previously (in 1.1.1) they would return -2. For key types that do not have
diff --git a/crypto/cms/cms_env.c b/crypto/cms/cms_env.c
index 494c2cc8fc..aa020cedfd 100644
--- a/crypto/cms/cms_env.c
+++ b/crypto/cms/cms_env.c
@@ -485,12 +485,6 @@ static int cms_RecipientInfo_ktri_encrypt(const CMS_ContentInfo *cms,
             goto err;
     }
 
-    if (EVP_PKEY_CTX_ctrl(pctx, -1, EVP_PKEY_OP_ENCRYPT,
-                          EVP_PKEY_CTRL_CMS_ENCRYPT, 0, ri) <= 0) {
-        ERR_raise(ERR_LIB_CMS, CMS_R_CTRL_ERROR);
-        goto err;
-    }
-
     if (EVP_PKEY_encrypt(pctx, NULL, &eklen, ec->key, ec->keylen) <= 0)
         goto err;
 
@@ -574,12 +568,6 @@ static int cms_RecipientInfo_ktri_decrypt(CMS_ContentInfo *cms,
     if (!ossl_cms_env_asn1_ctrl(ri, 1))
         goto err;
 
-    if (EVP_PKEY_CTX_ctrl(ktri->pctx, -1, EVP_PKEY_OP_DECRYPT,
-                          EVP_PKEY_CTRL_CMS_DECRYPT, 0, ri) <= 0) {
-        ERR_raise(ERR_LIB_CMS, CMS_R_CTRL_ERROR);
-        goto err;
-    }
-
     if (EVP_PKEY_decrypt(ktri->pctx, NULL, &eklen,
                          ktri->encryptedKey->data,
                          ktri->encryptedKey->length) <= 0)
diff --git a/crypto/cms/cms_sd.c b/crypto/cms/cms_sd.c
index c98d118f4b..287021fc21 100644
--- a/crypto/cms/cms_sd.c
+++ b/crypto/cms/cms_sd.c
@@ -749,24 +749,6 @@ int CMS_SignerInfo_sign(CMS_SignerInfo *si)
         si->pctx = pctx;
     }
 
-    /*
-     * TODO(3.0): This causes problems when providers are in use, so disabled
-     * for now. Can we get rid of this completely? AFAICT this ctrl has been
-     * present since CMS was first put in - but has never been used to do
-     * anything. All internal implementations just return 1 and ignore this ctrl
-     * and have always done so by the looks of things. To fix this we could
-     * convert this ctrl into a param, which would require us to send all the
-     * signer info data as a set of params...but that is non-trivial and since
-     * this isn't used by anything it may be better just to remove it.
-     */
-#if 0
-    if (EVP_PKEY_CTX_ctrl(pctx, -1, EVP_PKEY_OP_SIGN,
-                          EVP_PKEY_CTRL_CMS_SIGN, 0, si) <= 0) {
-        ERR_raise(ERR_LIB_CMS, CMS_R_CTRL_ERROR);
-        goto err;
-    }
-#endif
-
     alen = ASN1_item_i2d((ASN1_VALUE *)si->signedAttrs, &abuf,
                          ASN1_ITEM_rptr(CMS_Attributes_Sign));
     if (!abuf)
@@ -782,24 +764,6 @@ int CMS_SignerInfo_sign(CMS_SignerInfo *si)
     if (EVP_DigestSignFinal(mctx, abuf, &siglen) <= 0)
         goto err;
 
-    /*
-     * TODO(3.0): This causes problems when providers are in use, so disabled
-     * for now. Can we get rid of this completely? AFAICT this ctrl has been
-     * present since CMS was first put in - but has never been used to do
-     * anything. All internal implementations just return 1 and ignore this ctrl
-     * and have always done so by the looks of things. To fix this we could
-     * convert this ctrl into a param, which would require us to send all the
-     * signer info data as a set of params...but that is non-trivial and since
-     * this isn't used by anything it may be better just to remove it.
-     */
-#if 0
-    if (EVP_PKEY_CTX_ctrl(pctx, -1, EVP_PKEY_OP_SIGN,
-                          EVP_PKEY_CTRL_CMS_SIGN, 1, si) <= 0) {
-        ERR_raise(ERR_LIB_CMS, CMS_R_CTRL_ERROR);
-        goto err;
-    }
-#endif
-
     EVP_MD_CTX_reset(mctx);
 
     ASN1_STRING_set0(si->signature, abuf, siglen);
diff --git a/crypto/evp/ctrl_params_translate.c b/crypto/evp/ctrl_params_translate.c
index 4863b81db9..2d09f182cf 100644
--- a/crypto/evp/ctrl_params_translate.c
+++ b/crypto/evp/ctrl_params_translate.c
@@ -1432,34 +1432,6 @@ static int fix_hkdf_mode(enum state state,
     return 1;
 }
 
-static int hack_pkcs7_cms(enum state state,
-                          const struct translation_st *translation,
-                          struct translation_ctx_st *ctx)
-{
-    int ret = 1;
-
-    /* Make sure that this has no further effect */
-    ctx->action_type = 0;
-
-    switch (state) {
-    case PRE_CTRL_TO_PARAMS:
-        /* TODO (3.0) Temporary hack, this should probe */
-        if (EVP_PKEY_is_a(EVP_PKEY_CTX_get0_pkey(ctx->pctx), "RSASSA-PSS")) {
-            ERR_raise(ERR_LIB_EVP,
-                      EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE);
-            ret = -2;
-        }
-        break;
-    case POST_CTRL_TO_PARAMS:
-        break;
-    default:
-        ERR_raise(ERR_LIB_EVP, EVP_R_COMMAND_NOT_SUPPORTED);
-        ret = -2;
-        break;
-    }
-    return ret;
-}
-
 /*-
  * Payload getters
  * ===============
@@ -2121,16 +2093,6 @@ static const struct translation_st evp_pkey_ctx_translations[] = {
       EVP_PKEY_CTRL_RSA_KEYGEN_PRIMES, "rsa_keygen_primes", NULL,
       OSSL_PKEY_PARAM_RSA_PRIMES, OSSL_PARAM_UNSIGNED_INTEGER, NULL },
 
-    /* PKCS#7 and CMS hacks */
-    { SET, -1, -1, EVP_PKEY_OP_ENCRYPT,
-      EVP_PKEY_CTRL_PKCS7_ENCRYPT, NULL, NULL, NULL, 0, hack_pkcs7_cms },
-    { SET, -1, -1, EVP_PKEY_OP_DECRYPT,
-      EVP_PKEY_CTRL_PKCS7_DECRYPT, NULL, NULL, NULL, 0, hack_pkcs7_cms },
-    { SET, -1, -1, EVP_PKEY_OP_ENCRYPT,
-      EVP_PKEY_CTRL_CMS_ENCRYPT, NULL, NULL, NULL, 0, hack_pkcs7_cms },
-    { SET, -1, -1, EVP_PKEY_OP_DECRYPT,
-      EVP_PKEY_CTRL_CMS_DECRYPT, NULL, NULL, NULL, 0, hack_pkcs7_cms },
-
     /*-
      * TLS1-PRF
      * ========
diff --git a/crypto/pkcs7/pk7_doit.c b/crypto/pkcs7/pk7_doit.c
index c7a50ff57e..964b1367b2 100644
--- a/crypto/pkcs7/pk7_doit.c
+++ b/crypto/pkcs7/pk7_doit.c
@@ -122,12 +122,6 @@ static int pkcs7_encode_rinfo(PKCS7_RECIP_INFO *ri,
     if (EVP_PKEY_encrypt_init(pctx) <= 0)
         goto err;
 
-    if (EVP_PKEY_CTX_ctrl(pctx, -1, EVP_PKEY_OP_ENCRYPT,
-                          EVP_PKEY_CTRL_PKCS7_ENCRYPT, 0, ri) <= 0) {
-        ERR_raise(ERR_LIB_PKCS7, PKCS7_R_CTRL_ERROR);
-        goto err;
-    }
-
     if (EVP_PKEY_encrypt(pctx, NULL, &eklen, key, keylen) <= 0)
         goto err;
 
@@ -171,12 +165,6 @@ static int pkcs7_decrypt_rinfo(unsigned char **pek, int *peklen,
     if (EVP_PKEY_decrypt_init(pctx) <= 0)
         goto err;
 
-    if (EVP_PKEY_CTX_ctrl(pctx, -1, EVP_PKEY_OP_DECRYPT,
-                          EVP_PKEY_CTRL_PKCS7_DECRYPT, 0, ri) <= 0) {
-        ERR_raise(ERR_LIB_PKCS7, PKCS7_R_CTRL_ERROR);
-        goto err;
-    }
-
     if (EVP_PKEY_decrypt(pctx, NULL, &eklen,
                          ri->enc_key->data, ri->enc_key->length) <= 0)
         goto err;
@@ -932,30 +920,6 @@ int PKCS7_SIGNER_INFO_sign(PKCS7_SIGNER_INFO *si)
                               NULL) <= 0)
         goto err;
 
-    /*
-     * TODO(3.0): This causes problems when providers are in use, so disabled
-     * for now. Can we get rid of this completely? AFAICT this ctrl has never
-     * been used since it was first put in. All internal implementations just
-     * return 1 and ignore this ctrl and have always done so by the looks of
-     * things. To fix this we could convert this ctrl into a param, which would
-     * require us to send all the signer info data as a set of params...but that
-     * is non-trivial and since this isn't used by anything it may be better
-     * just to remove it. The original commit that added it had this
-     * justification in CHANGES:
-     *
-     * "During PKCS7 signing pass the PKCS7 SignerInfo structure to the
-     *  EVP_PKEY_METHOD before and after signing via the
-     *  EVP_PKEY_CTRL_PKCS7_SIGN ctrl. It can then customise the structure
-     *  before and/or after signing if necessary."
-     */
-#if 0
-    if (EVP_PKEY_CTX_ctrl(pctx, -1, EVP_PKEY_OP_SIGN,
-                          EVP_PKEY_CTRL_PKCS7_SIGN, 0, si) <= 0) {
-        ERR_raise(ERR_LIB_PKCS7, PKCS7_R_CTRL_ERROR);
-        goto err;
-    }
-#endif
-
     alen = ASN1_item_i2d((ASN1_VALUE *)si->auth_attr, &abuf,
                          ASN1_ITEM_rptr(PKCS7_ATTR_SIGN));
     if (!abuf)
@@ -972,30 +936,6 @@ int PKCS7_SIGNER_INFO_sign(PKCS7_SIGNER_INFO *si)
     if (EVP_DigestSignFinal(mctx, abuf, &siglen) <= 0)
         goto err;
 
-    /*
-     * TODO(3.0): This causes problems when providers are in use, so disabled
-     * for now. Can we get rid of this completely? AFAICT this ctrl has never
-     * been used since it was first put in. All internal implementations just
-     * return 1 and ignore this ctrl and have always done so by the looks of
-     * things. To fix this we could convert this ctrl into a param, which would
-     * require us to send all the signer info data as a set of params...but that
-     * is non-trivial and since this isn't used by anything it may be better
-     * just to remove it. The original commit that added it had this
-     * justification in CHANGES:
-     *
-     * "During PKCS7 signing pass the PKCS7 SignerInfo structure to the
-     *  EVP_PKEY_METHOD before and after signing via the
-     *  EVP_PKEY_CTRL_PKCS7_SIGN ctrl. It can then customise the structure
-     *  before and/or after signing if necessary."
-     */
-#if 0
-    if (EVP_PKEY_CTX_ctrl(pctx, -1, EVP_PKEY_OP_SIGN,
-                          EVP_PKEY_CTRL_PKCS7_SIGN, 1, si) <= 0) {
-        ERR_raise(ERR_LIB_PKCS7, PKCS7_R_CTRL_ERROR);
-        goto err;
-    }
-#endif
-
     EVP_MD_CTX_free(mctx);
 
     ASN1_STRING_set0(si->enc_digest, abuf, siglen);
diff --git a/include/openssl/evp.h b/include/openssl/evp.h
index ed54575e84..20fd70a7b6 100644
--- a/include/openssl/evp.h
+++ b/include/openssl/evp.h
@@ -1632,16 +1632,18 @@ int EVP_PKEY_CTX_set_mac_key(EVP_PKEY_CTX *ctx, const unsigned char *key,
 
 # define EVP_PKEY_CTRL_MD                1
 # define EVP_PKEY_CTRL_PEER_KEY          2
-# define EVP_PKEY_CTRL_PKCS7_ENCRYPT     3
-# define EVP_PKEY_CTRL_PKCS7_DECRYPT     4
-# define EVP_PKEY_CTRL_PKCS7_SIGN        5
 # define EVP_PKEY_CTRL_SET_MAC_KEY       6
 # define EVP_PKEY_CTRL_DIGESTINIT        7
 /* Used by GOST key encryption in TLS */
 # define EVP_PKEY_CTRL_SET_IV            8
-# define EVP_PKEY_CTRL_CMS_ENCRYPT       9
-# define EVP_PKEY_CTRL_CMS_DECRYPT       10
-# define EVP_PKEY_CTRL_CMS_SIGN          11
+# ifndef OPENSSL_NO_DEPRECATED_3_0
+#  define EVP_PKEY_CTRL_PKCS7_ENCRYPT     3
+#  define EVP_PKEY_CTRL_PKCS7_DECRYPT     4
+#  define EVP_PKEY_CTRL_PKCS7_SIGN        5
+#  define EVP_PKEY_CTRL_CMS_ENCRYPT       9
+#  define EVP_PKEY_CTRL_CMS_DECRYPT       10
+#  define EVP_PKEY_CTRL_CMS_SIGN          11
+# endif
 # define EVP_PKEY_CTRL_CIPHER            12
 # define EVP_PKEY_CTRL_GET_MD            13
 # define EVP_PKEY_CTRL_SET_DIGEST_SIZE   14
diff --git a/providers/common/include/prov/securitycheck.h b/providers/common/include/prov/securitycheck.h
index c322457fc8..7d163f70fa 100644
--- a/providers/common/include/prov/securitycheck.h
+++ b/providers/common/include/prov/securitycheck.h
@@ -10,7 +10,7 @@
 #include "crypto/types.h"
 
 /* Functions that are common */
-int ossl_rsa_check_key(const RSA *rsa, int protect);
+int ossl_rsa_check_key(const RSA *rsa, int operation);
 int ossl_ec_check_key(const EC_KEY *ec, int protect);
 int ossl_dsa_check_key(const DSA *dsa, int sign);
 int ossl_dh_check_key(const DH *dh);
diff --git a/providers/common/securitycheck.c b/providers/common/securitycheck.c
index 3f8a742286..08582d6346 100644
--- a/providers/common/securitycheck.c
+++ b/providers/common/securitycheck.c
@@ -13,6 +13,7 @@
 #include <openssl/dsa.h>
 #include <openssl/dh.h>
 #include <openssl/ec.h>
+#include <openssl/evp.h>
 #include <openssl/err.h>
 #include <openssl/proverr.h>
 #include <openssl/core_names.h>
@@ -25,14 +26,50 @@
  * Set protect = 1 for encryption or signing operations, or 0 otherwise. See
  * https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar2.pdf.
  */
-int ossl_rsa_check_key(const RSA *rsa, int protect)
+int ossl_rsa_check_key(const RSA *rsa, int operation)
 {
+    int protect = 0;
+
+    switch (operation) {
+        case EVP_PKEY_OP_SIGN:
+            protect = 1;
+            /* fallthrough */
+        case EVP_PKEY_OP_VERIFY:
+            break;
+        case EVP_PKEY_OP_ENCAPSULATE:
+        case EVP_PKEY_OP_ENCRYPT:
+            protect = 1;
+            /* fallthrough */
+        case EVP_PKEY_OP_VERIFYRECOVER:
+        case EVP_PKEY_OP_DECAPSULATE:
+        case EVP_PKEY_OP_DECRYPT:
+            if (RSA_test_flags(rsa,
+                               RSA_FLAG_TYPE_MASK) == RSA_FLAG_TYPE_RSASSAPSS) {
+                ERR_raise_data(ERR_LIB_PROV,
+                               PROV_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE,
+                               "operation: %d", operation);
+                return 0;
+            }
+            break;
+        default:
+            ERR_raise_data(ERR_LIB_PROV, ERR_R_INTERNAL_ERROR,
+                           "invalid operation: %d", operation);
+            return 0;
+    }
+
 #if !defined(OPENSSL_NO_FIPS_SECURITYCHECKS)
     if (ossl_securitycheck_enabled()) {
         int sz = RSA_bits(rsa);
 
-        return protect ? (sz >= 2048) : (sz >= 1024);
+        if (protect ? (sz < 2048) : (sz < 1024)) {
+            ERR_raise_data(ERR_LIB_PROV, PROV_R_INVALID_KEY_LENGTH,
+                           "operation: %d", operation);
+            return 0;
+        }
     }
+#else
+    /* make protect used */
+    (void)protect;
 #endif /* OPENSSL_NO_FIPS_SECURITYCHECKS */
     return 1;
 }
diff --git a/providers/implementations/asymciphers/rsa_enc.c b/providers/implementations/asymciphers/rsa_enc.c
index 621239d79e..ab84d53512 100644
--- a/providers/implementations/asymciphers/rsa_enc.c
+++ b/providers/implementations/asymciphers/rsa_enc.c
@@ -96,10 +96,13 @@ static int rsa_init(void *vprsactx, void *vrsa, const OSSL_PARAM params[],
 {
     PROV_RSA_CTX *prsactx = (PROV_RSA_CTX *)vprsactx;
 
-    if (!ossl_prov_is_running()
-            || prsactx == NULL
-            || vrsa == NULL
-            || !RSA_up_ref(vrsa))
+    if (!ossl_prov_is_running() || prsactx == NULL || vrsa == NULL)
+        return 0;
+
+    if (!ossl_rsa_check_key(vrsa, operation))
+        return 0;
+
+    if (!RSA_up_ref(vrsa))
         return 0;
     RSA_free(prsactx->rsa);
     prsactx->rsa = vrsa;
@@ -110,11 +113,8 @@ static int rsa_init(void *vprsactx, void *vrsa, const OSSL_PARAM params[],
         prsactx->pad_mode = RSA_PKCS1_PADDING;
         break;
     default:
-        ERR_raise(ERR_LIB_PROV, PROV_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE);
-        return 0;
-    }
-    if (!ossl_rsa_check_key(vrsa, operation == EVP_PKEY_OP_ENCRYPT)) {
-        ERR_raise(ERR_LIB_PROV, PROV_R_INVALID_KEY_LENGTH);
+        /* This should not happen due to the check above */
+        ERR_raise(ERR_LIB_PROV, ERR_R_INTERNAL_ERROR);
         return 0;
     }
     return rsa_set_ctx_params(prsactx, params);
diff --git a/providers/implementations/kem/rsa_kem.c b/providers/implementations/kem/rsa_kem.c
index 36bcea3506..3809bfb8b1 100644
--- a/providers/implementations/kem/rsa_kem.c
+++ b/providers/implementations/kem/rsa_kem.c
@@ -122,15 +122,17 @@ static int rsakem_init(void *vprsactx, void *vrsa,
 {
     PROV_RSA_CTX *prsactx = (PROV_RSA_CTX *)vprsactx;
 
-    if (prsactx == NULL || vrsa == NULL || !RSA_up_ref(vrsa))
+    if (prsactx == NULL || vrsa == NULL)
+        return 0;
+
+    if (!ossl_rsa_check_key(vrsa, operation))
+        return 0;
+
+    if (!RSA_up_ref(vrsa))
         return 0;
     RSA_free(prsactx->rsa);
     prsactx->rsa = vrsa;
 
-    if (!ossl_rsa_check_key(vrsa, operation == EVP_PKEY_OP_ENCAPSULATE)) {
-        ERR_raise(ERR_LIB_PROV, PROV_R_INVALID_KEY_LENGTH);
-        return 0;
-    }
     return rsakem_set_ctx_params(prsactx, params);
 }
 
diff --git a/providers/implementations/signature/rsa.c b/providers/implementations/signature/rsa.c
index f521f0190d..bfaa7b4e80 100644
--- a/providers/implementations/signature/rsa.c
+++ b/providers/implementations/signature/rsa.c
@@ -365,9 +365,14 @@ static int rsa_signverify_init(void *vprsactx, void *vrsa,
     if (!ossl_prov_is_running())
         return 0;
 
-    if (prsactx == NULL || vrsa == NULL || !RSA_up_ref(vrsa))
+    if (prsactx == NULL || vrsa == NULL)
         return 0;
 
+    if (!ossl_rsa_check_key(vrsa, operation))
+        return 0;
+
+    if (!RSA_up_ref(vrsa))
+        return 0;
     RSA_free(prsactx->rsa);
     prsactx->rsa = vrsa;
     prsactx->operation = operation;
@@ -375,11 +380,6 @@ static int rsa_signverify_init(void *vprsactx, void *vrsa,
     if (!rsa_set_ctx_params(prsactx, params))
         return 0;
 
-    if (!ossl_rsa_check_key(vrsa, operation == EVP_PKEY_OP_SIGN)) {
-        ERR_raise(ERR_LIB_PROV, PROV_R_INVALID_KEY_LENGTH);
-        return 0;
-    }
-
     /* Maximum for sign, auto for verify */
     prsactx->saltlen = RSA_PSS_SALTLEN_AUTO;
     prsactx->min_saltlen = -1;