From 0cfbc828e03ad69c50ae51e0c88920d90906498a Mon Sep 17 00:00:00 2001
From: Tomas Mraz <tomas@openssl.org>
Date: Thu, 1 Apr 2021 17:14:43 +0200
Subject: [PATCH] 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)
---
CHANGES.md | 9 +++
crypto/cms/cms_env.c | 12 ----
crypto/cms/cms_sd.c | 36 -----------
crypto/evp/ctrl_params_translate.c | 38 ------------
crypto/pkcs7/pk7_doit.c | 60 -------------------
include/openssl/evp.h | 14 +++--
providers/common/include/prov/securitycheck.h | 2 +-
providers/common/securitycheck.c | 41 ++++++++++++-
.../implementations/asymciphers/rsa_enc.c | 18 +++---
providers/implementations/kem/rsa_kem.c | 12 ++--
providers/implementations/signature/rsa.c | 12 ++--
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;